Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Problem: memiavl background snapshot writing don't limit concurrency level #1169

Closed
wants to merge 40 commits into from
Closed

Problem: memiavl background snapshot writing don't limit concurrency level #1169

wants to merge 40 commits into from

Conversation

jacksoom
Copy link
Contributor

@jacksoom jacksoom commented Sep 15, 2023 •
edited
Loading

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

  • Have you read the CONTRIBUTING.md?
  • Does your PR follow the C4 patch requirements?
  • Have you rebased your work on top of the latest master?
  • Have you checked your code compiles? (make)
  • Have you included tests for any non-trivial functionality?
  • Have you checked your code passes the unit tests? (make test)
  • Have you checked your code formatting is correct? (go fmt)
  • Have you checked your basic code style is fine? (golangci-lint run)
  • If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
  • If your changes affect the client infrastructure, have you run the integration test?
  • If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
  • If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
  • If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

mmsqe and others added 7 commits September 13, 2023 05:29
@mmsqe @yihuang
Problem: no log converted from native event (#1159)
5ad3299 
* Problem: no log converted from native event

* mv doc to script

* Apply suggestions from code review

* refactoring

* revert

* more decoders

* cleanup

* rename

* cleanup

* fix errors

* fix

* fix lint

* fix statedb interface

* skip msg type event

* fix attr handle

* fix lint

* fix contract adr

* add makeFilter

* rm toMixedCase

* Apply suggestions from code review

* fix packetData

* clean up

* decode packetDataHex as data

* add gen bindings

* point to fill blk height

* Apply suggestions from code review

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>
Co-authored-by: HuangYi <[email protected]>
@mmsqe
Problem: event converter don't have to be global (#1164)
48dbe14
@yihuang
Problem: memiavl performance regression when cache miss (#1166)
580a5ac 
* Problem: memiavl performance regression when cache miss

Solution:
- make the cache optional

* cleanup
@yihuang
Problem: erc20 benchmarking fails (#1168)
f5e30b0 
Solution:
- fix it
@yihuang @mmsqe
Problem: statedb balance is not handled consistently (#1167)
e6ca45f 
Solution:
- integration latest ethermint

Co-authored-by: mmsqe <[email protected]>
@mmsqe
Problem: ica in ibc v7.2.0 is not adjust correctly (#1165)
ae58c99 
* Problem: icaauth route is not registered correctly

test icaauth

* rm dup claim capability

handle in https://github.com/cosmos/ibc-go/blob/v7.2.0/modules/apps/27-interchain-accounts/controller/ibc_middleware.go#L60

* fix submit tx

https://github.com/cosmos/ibc-go/blob/v7.2.0/modules/apps/27-interchain-accounts/controller/keeper/relay.go#L28

* Apply suggestions from code review

* cleanup test
@jacksoom
feat(memiavl): Add snapshot writer goroutine limit
a67c654
yihuang
yihuang reviewed Sep 15, 2023
View reviewed changes
memiavl/multitree.go Outdated Show resolved Hide resolved
yihuang
yihuang reviewed Sep 15, 2023
View reviewed changes
memiavl/db.go Outdated Show resolved Hide resolved
@yihuang yihuang changed the title feat(memiavl): Add snapshot writer goroutine limit Problem: memiavl background snapshot writing don't limit concurrency level Sep 15, 2023
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 15, 2023
View reviewed changes
memiavl/export.go Fixed Show fixed Hide fixed
@jacksoom jacksoom marked this pull request as ready for review September 15, 2023 08:18
@jacksoom jacksoom requested a review from a team as a code owner September 15, 2023 08:18
@jacksoom jacksoom requested review from mmsqe and calvinaco and removed request for a team September 15, 2023 08:18
mmsqe
mmsqe reviewed Sep 15, 2023
View reviewed changes
Copy link
Collaborator

@mmsqe mmsqe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe could limit with CPU-bound like nProcs := runtime.GOMAXPROCS(0)

mmsqe
mmsqe reviewed Sep 15, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
jacksoom and others added 2 commits September 15, 2023 16:45
@jacksoom @mmsqe
Update CHANGELOG.md
73515fc 
Co-authored-by: mmsqe <[email protected]>
Signed-off-by: jacksoom <[email protected]>
@jacksoom @mmsqe
Update go.mod
5106e2e 
Co-authored-by: mmsqe <[email protected]>
Signed-off-by: jacksoom <[email protected]>
@jacksoom
Copy link
Contributor Author

jacksoom commented Sep 15, 2023 •
edited
Loading

maybe could limit with CPU-bound like nProcs := runtime.GOMAXPROCS(0)

Just prevent affecting other goroutine

yihuang
yihuang reviewed Sep 15, 2023
View reviewed changes
Copy link
Collaborator

@yihuang yihuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Minor suggestions.

memiavl/db.go Outdated Show resolved Hide resolved
memiavl/db.go Show resolved Hide resolved
mmsqe
mmsqe approved these changes Sep 16, 2023
View reviewed changes
Copy link
Collaborator

@mmsqe mmsqe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, would you mind update gomod2nix.toml and sign the commits?

@yihuang
Copy link
Collaborator

yihuang commented Sep 18, 2023

@jacksoom you need to sign all commits, you can squash the commits instead.

yihuang
yihuang reviewed Sep 18, 2023
View reviewed changes
memiavl/db_test.go Outdated Show resolved Hide resolved
yihuang
yihuang approved these changes Sep 18, 2023
View reviewed changes
Copy link
Collaborator

@yihuang yihuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor suggestions to void adding default option to the test.

mmsqe and others added 20 commits September 18, 2023 10:39
@mmsqe @yihuang @jacksoom
Problem: no log converted from native event (#1159)
08ef66e 
* Problem: no log converted from native event

* mv doc to script

* Apply suggestions from code review

* refactoring

* revert

* more decoders

* cleanup

* rename

* cleanup

* fix errors

* fix

* fix lint

* fix statedb interface

* skip msg type event

* fix attr handle

* fix lint

* fix contract adr

* add makeFilter

* rm toMixedCase

* Apply suggestions from code review

* fix packetData

* clean up

* decode packetDataHex as data

* add gen bindings

* point to fill blk height

* Apply suggestions from code review

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>
Co-authored-by: HuangYi <[email protected]>
@mmsqe @jacksoom
Problem: event converter don't have to be global (#1164)
18a1281
@yihuang @jacksoom
Problem: memiavl performance regression when cache miss (#1166)
00197d1 
* Problem: memiavl performance regression when cache miss

Solution:
- make the cache optional

* cleanup
@yihuang @jacksoom
Problem: erc20 benchmarking fails (#1168)
97db489 
Solution:
- fix it
@yihuang @mmsqe @jacksoom
Problem: statedb balance is not handled consistently (#1167)
26bb535 
Solution:
- integration latest ethermint

Co-authored-by: mmsqe <[email protected]>
@mmsqe @jacksoom
Problem: ica in ibc v7.2.0 is not adjust correctly (#1165)
f588059 
* Problem: icaauth route is not registered correctly

test icaauth

* rm dup claim capability

handle in https://github.com/cosmos/ibc-go/blob/v7.2.0/modules/apps/27-interchain-accounts/controller/ibc_middleware.go#L60

* fix submit tx

https://github.com/cosmos/ibc-go/blob/v7.2.0/modules/apps/27-interchain-accounts/controller/keeper/relay.go#L28

* Apply suggestions from code review

* cleanup test
@jacksoom
fix comments
406a5c8
@jacksoom
simple
70d2e40
@jacksoom
add Improvements CHANGELOG
1289624
@jacksoom @mmsqe
Update CHANGELOG.md
0cf0d0a 
Co-authored-by: mmsqe <[email protected]>
Signed-off-by: jacksoom <[email protected]>
@jacksoom @mmsqe
Update go.mod
c0a48bf 
Co-authored-by: mmsqe <[email protected]>
Signed-off-by: jacksoom <[email protected]>
@jacksoom
simple fix
fa5f9e7
@jacksoom
replace raw goroutine to worker pool
71b1fb0
@jacksoom
update gomod2nix
b60b411
@jacksoom
add annotation
1e72328
@jacksoom
simple fix
de93057 
simple fix
@jacksoom
add annotation
8b172ea
@jacksoom
add annotation
b4f9293
@jacksoom
Merge branch 'main' of https://github.com/jacksoom/cronos
734357f
@jacksoom
remove duplicate option
4290bd5
@yihuang yihuang enabled auto-merge September 18, 2023 02:49
auto-merge was automatically disabled September 18, 2023 04:42

Head branch was pushed to by a user without write access

@jacksoom jacksoom closed this by deleting the head repository Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmsqe mmsqe mmsqe approved these changes

@yihuang yihuang yihuang approved these changes

@calvinaco calvinaco Awaiting requested review from calvinaco calvinaco is a code owner automatically assigned from crypto-org-chain/cronos-dev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jacksoom @yihuang @mmsqe