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

asynchronous update snap in updatetrie #924

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

flywukong
Copy link
Contributor

@flywukong flywukong commented May 23, 2022

Description

the updateTrie function need to update trie node and snap in a for loop, since the snap updating need wait lock , it may make update trie slower , this PR use a groutine to update snap to make update trie more efficient

Rationale

When chasing blocks in batches, the time overhead of volidation is reduced by 10%,
The overall efficiency of batch sync blocks has been improved, the follow lines represents the delay of volidation with pipeline commit
image

When executing the latest block, the latency of updateTrie is reduced by 30%-40%, the follow lines represents the delay of an updateTrie function call.

image

Example

add an example CLI or API response...

Changes

Notable changes:

  • add each change in a bullet point here
  • ...

yutianwu
yutianwu previously approved these changes May 27, 2022
Copy link
Contributor

@setunapo setunapo left a comment

Choose a reason for hiding this comment

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

LGTM

core/state/state_object.go Outdated Show resolved Hide resolved
for key, value := range dirtyStorage {
// If state snapshotting is active, cache the data til commit
if s.db.snap != nil {
s.db.snapMux.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

as mentioned above, could be better to hold Lock out of the for loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Contributor

Choose a reason for hiding this comment

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

whole for loop including storage update

core/state/state_object.go Outdated Show resolved Hide resolved
core/state/state_object.go Show resolved Hide resolved
core/state/state_object.go Outdated Show resolved Hide resolved
Copy link
Contributor

@setunapo setunapo left a comment

Choose a reason for hiding this comment

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

As noticed in previous comments

qinglin89
qinglin89 previously approved these changes Jun 2, 2022
Copy link
Contributor

@qinglin89 qinglin89 left a comment

Choose a reason for hiding this comment

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

LGTM

core/state/state_object.go Outdated Show resolved Hide resolved
core/state/state_object.go Outdated Show resolved Hide resolved
setunapo
setunapo previously approved these changes Jun 2, 2022
@flywukong flywukong changed the base branch from master to develop June 7, 2022 02:06
@flywukong flywukong dismissed stale reviews from setunapo, qinglin89, and yutianwu June 7, 2022 02:06

The base branch was changed.

setunapo
setunapo previously approved these changes Jun 7, 2022
qinglin89
qinglin89 previously approved these changes Jun 7, 2022
@flywukong flywukong merged commit aa3cc82 into bnb-chain:develop Jun 7, 2022
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.

6 participants