refactor(perf): replace sync struct HeaderView with HeaderIndexView #3970
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
quake
requested review from
doitian,
zhangsoledad and
eval-exec
and removed request for
a team
quake
commented
May 6, 2023
| @@ -101,7 +101,7 @@ bootnode_mode = false | |||
| support_protocols = ["Ping", "Discovery", "Identify", "Feeler", "DisconnectMessage", "Sync", "Relay", "Time", "Alert", "LightClient", "Filter"] | |||
|
|
|||
| # [network.sync.header_map] | |||
| # memory_limit = "600MB" | |||
| # memory_limit = "256MB" | |||
There was a problem hiding this comment.
Changed the default memory limit to smaller one, it can keep about the same amount of header data in memory as before.
quake
commented
May 6, 2023
| Some(status.contains(BlockStatus::BLOCK_STORED)), | ||
| )? | ||
| .into_inner(); | ||
| .get_header_index_view(&parent_hash, false)?; |
There was a problem hiding this comment.
we have checked the status if contains BLOCK_STORED in line 148 and returned early, no need to check it again here.
eval-exec
approved these changes
May 6, 2023
eval-exec
reviewed
May 6, 2023
driftluo
approved these changes
May 10, 2023
zhangsoledad
approved these changes
May 12, 2023
quake
force-pushed
the
quake/header-index-view
branch
from
e29af19 to
0c8e91e
Compare
quake
force-pushed
the
quake/header-index-view
branch
from
0c8e91e to
62e3454
Compare
doitian
reviewed
May 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
resolve #3940
What is changed and how it works?
replace the struct
HeaderViewof sync mod with a more memory compact structHeaderIndexView, and avoid holding headers messages in memory (remove bytes slice clone).Compare with v0.109.0, this PR reduced the peak memory usage from 3.8G to 2.8G during IBD sync on my local PC. @jiangxianliang007 also did test on an aws c5.xlarge instance (4C8G), it reduced from 3.2G to 2.3G, sync performance has also improved, from 0 to 9 million, dropping from 19 hours and 25 minutes to 17 hours and 30 minutes.
please not that this PR didn't replace the HeaderMap storage sled with other solution, there is another PR in testing
Check List
Tests
Release note