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

core: speed up header import #21967

Merged
merged 4 commits into from
Jan 26, 2021
Merged

core: speed up header import #21967

merged 4 commits into from
Jan 26, 2021

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Dec 7, 2020 •
edited
Loading

This is a follow up to #21471.

Charts and niceties can be found here: https://github.com/holiman/headerimport/blob/main/README.md

This PR implements the following modifications

  • 1M blocks without header-shortcut (mod1)
    • Do not shortcut check if block is present, thus avoid disk lookup
  • 1M blocks with less hashing (mod2)
    • Don't check hash ancestry in early-check (it's still done in parallel checker)
  • 1M blocks with less syncronization (mod3)
    • Don't check for abort between every header EDIT: this optimization was reverted
  • 1M blocks without Now
    • Don't check time.Now for every single header

TLDR: With these changes, writing 1M headers goes down to 62s for 1M headers, from 106s on (old) master, and 80s on PR 21471.

@holiman holiman force-pushed the validation_speedup branch 2 times, most recently from 3ba14c9 to 90be6be Compare December 9, 2020 12:14
@holiman holiman marked this pull request as ready for review December 9, 2020 12:18
holiman
holiman commented Dec 9, 2020
View reviewed changes
core/headerchain.go Outdated Show resolved Hide resolved
@holiman holiman modified the milestone: 1.10.0 Dec 10, 2020
ucwong
ucwong reviewed Dec 18, 2020
View reviewed changes
consensus/ethash/consensus.go
@@ -180,10 +181,7 @@ func (ethash *Ethash) verifyHeaderWorker(chain consensus.ChainHeaderReader, head
if parent == nil {
return consensus.ErrUnknownAncestor
}
if chain.GetHeader(headers[index].Hash(), headers[index].Number.Uint64()) != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this check ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://github.com/holiman/headerimport/blob/main/README.md#1m-blocks-on-21471

@holiman holiman force-pushed the validation_speedup branch from b5aa900 to 6bce7c4 Compare January 11, 2021 10:42
@holiman
Copy link
Contributor Author

holiman commented Jan 11, 2021

Rebased on master. Master:
total-times_master-2021-01-11_1M

This PR:
total-times_21967-2021-01-11_1M

TLDR: 52+22 = 74s vs 38+23=61s

karalabe
karalabe reviewed Jan 26, 2021
View reviewed changes
consensus/ethash/consensus.go Outdated
@@ -130,11 +130,12 @@ func (ethash *Ethash) VerifyHeaders(chain consensus.ChainHeaderReader, headers [
done = make(chan int, workers)
errors = make([]error, len(headers))
abort = make(chan struct{})
utcNow = time.Now().Unix()
Copy link
Member

Choose a reason for hiding this comment

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

unix :)

karalabe
karalabe approved these changes Jan 26, 2021
View reviewed changes
Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

Pls fix utx->unix, othewise SGTM

@holiman holiman force-pushed the validation_speedup branch from d06d950 to 92a53c9 Compare January 26, 2021 10:16
@holiman holiman changed the title core: header validation speedup core: speed up header import Jan 26, 2021
@holiman holiman merged commit 6816182 into ethereum:master Jan 26, 2021
@quorumbot quorumbot mentioned this pull request Sep 3, 2021
Merged
9 tasks
@baptiste-b-pegasys baptiste-b-pegasys mentioned this pull request Sep 3, 2021
Closed
9 tasks
This was referenced Sep 23, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MariusVanDerWijden MariusVanDerWijden MariusVanDerWijden left review comments

@ucwong ucwong ucwong left review comments

@karalabe karalabe karalabe approved these changes

@rjl493456442 rjl493456442 Awaiting requested review from rjl493456442 rjl493456442 is a code owner

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

Successfully merging this pull request may close these issues.
4 participants
@holiman @karalabe @MariusVanDerWijden @ucwong