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.

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

Problem: memiavl restore not fast enough #1241

Merged
merged 4 commits into from
Nov 15, 2023

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Nov 14, 2023

Solution:

  • increase channel buffer size to improve parallisation

it reduce restore time by 44% when testing with testnet snapshot, thanks @yzang2019 for sharing the finding.

👮🏻👮🏻👮🏻 !!!! 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! :)

Summary by CodeRabbit

  • Performance Improvements

    • Enhanced the parallelization process for data restoration to improve efficiency.
    • Refactored the websocket/subscription system for better performance and stability.
  • Refactor

    • Adjusted internal processing order in test functions for consistency and maintainability.

Solution:
- increase channel buffer size to improve parallisation

it reduce restore time by 44% when testing with testnet, thanks @yzang2019 for sharing the finding.
@yihuang yihuang requested a review from a team as a code owner November 14, 2023 11:14
@yihuang yihuang requested review from calvinaco and leejw51crypto and removed request for a team November 14, 2023 11:14
Copy link
Contributor

coderabbitai bot commented Nov 14, 2023

Walkthrough

The recent update brings a significant enhancement to the system's performance and stability, focusing on two main areas: the websocket/subscription system and the memiavl restoration process. The websocket system has been refactored for better efficiency, while memiavl restoration now benefits from improved parallelization, ensuring faster and more reliable data handling.

Changes

File Path Change Summary
CHANGELOG.md Refactored websocket/subscription system; improved parallelization of memiavl restoration.
memiavl/import.go Introduced NodeChannelBuffer constant; initialized nodesChan with buffer size.
memiavl/snapshot_test.go Altered the order of exporter.Close() and importer.Close() function calls.

🐇 In the code's woven tapestry, threads align anew, 🍂
🌟 Performance leaps, as autumn's breeze whispers through.
🌐 Websockets dance, memiavl's trees in swift array,
🎉 On this day in history, our code hops forward, hooray!


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: yihuang <huang@crypto.com>
@yihuang yihuang requested a review from mmsqe November 14, 2023 11:15
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 54e4d9c and 38fde43.
Files selected for processing (2)
  • CHANGELOG.md (1 hunks)
  • memiavl/import.go (2 hunks)
Additional comments: 2
memiavl/import.go (2)
  • 11-11: The introduction of a new constant NodeChannelBuffer is a good practice for managing magic numbers and improving code readability. Ensure that the value of 2048 is appropriate for all deployment scenarios, as a buffer that is too large may consume unnecessary memory, while one that is too small may not provide the intended performance benefits.

  • 104-105: Changing from an unbuffered to a buffered channel with a size of 2048 is a significant change that should improve the throughput of concurrent operations. However, it's important to ensure that the rest of the system can handle the increased concurrency without introducing bottlenecks elsewhere. Additionally, consider monitoring memory usage, as larger buffers can lead to increased memory consumption.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 38fde43 and 11959f5.
Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
  • 17-21: The changelog entries for the improvements are clear and concise, providing direct links to the pull requests for users to find more detailed information. It's good practice to include such references for transparency and traceability.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@yzang2019
Copy link

LGTM, I think you might also want to fix your unit test like: sei-protocol/sei-db#33

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 11959f5 and 7ed0fbe.
Files selected for processing (1)
  • memiavl/snapshot_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • memiavl/snapshot_test.go

@yihuang
Copy link
Collaborator Author

yihuang commented Nov 15, 2023

LGTM, I think you might also want to fix your unit test like: sei-protocol/sei-db#33

thanks, added.

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #1241 (7ed0fbe) into main (607fa83) will increase coverage by 20.22%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #1241       +/-   ##
===========================================
+ Coverage   16.67%   36.89%   +20.22%     
===========================================
  Files          79      115       +36     
  Lines        5786    10255     +4469     
===========================================
+ Hits          965     3784     +2819     
- Misses       4743     6097     +1354     
- Partials       78      374      +296     
Files Coverage Δ
memiavl/import.go 70.55% <100.00%> (ø)

... and 53 files with indirect coverage changes

@yihuang yihuang added this pull request to the merge queue Nov 15, 2023
Merged via the queue into crypto-org-chain:main with commit cdfb5de Nov 15, 2023
32 checks passed
@yihuang yihuang deleted the import-perf branch November 15, 2023 05:28
yihuang added a commit to yihuang/cronos that referenced this pull request Nov 15, 2023
)

* Problem: memiavl restore not fast enough

Solution:
- increase channel buffer size to improve parallisation

it reduce restore time by 44% when testing with testnet, thanks @yzang2019 for sharing the finding.

* Update CHANGELOG.md

Signed-off-by: yihuang <huang@crypto.com>

* fix unit test

---------

Signed-off-by: yihuang <huang@crypto.com>
yihuang added a commit that referenced this pull request Nov 15, 2023
* Problem: memiavl restore not fast enough

Solution:
- increase channel buffer size to improve parallisation

it reduce restore time by 44% when testing with testnet, thanks @yzang2019 for sharing the finding.

* Update CHANGELOG.md



* fix unit test

---------

Signed-off-by: yihuang <huang@crypto.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants