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

trie: parallel commit (alternative version) #30545

Merged
merged 15 commits into from
Oct 14, 2024

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Oct 3, 2024

Alternative to #30461

I've attempted to do it differently.

  • Get rid of the wrapped node struct
  • On top level, spin out new committers, one per goroutine. Each committer has it's own internal nodeset. After finishing it's work, each goroutine merges it's own nodeset with the parent nodeset.

With this approach, I get

BenchmarkCommit/commit-5000nodes-single-8                    129           9849412 ns/op         4620280 B/op      55223 allocs/op
BenchmarkCommit/commit-5000nodes-parallel-8                  313           3974443 ns/op         5592933 B/op      55789 allocs/op

So the 9ms -> 10ms for single-thread, and 4ms for multi-threaded approach. The mem usage is only marginally higher than current master.

trie/committer.go Outdated Show resolved Hide resolved
@holiman holiman force-pushed the parallel_commit_alt branch 3 times, most recently from e6591dd to 5d09de1 Compare October 3, 2024 20:06
@holiman
Copy link
Contributor Author

holiman commented Oct 3, 2024

Running this on 07 vs master on 08 now

@holiman
Copy link
Contributor Author

holiman commented Oct 4, 2024

Here's the same:ish chart as @rjl493456442 provided for #30461

Screenshot 2024-10-04 at 16-35-30 Dual Geth - Grafana

The previous one here

It's a bit odd -- the previous PR did showed a clear difference, but this PR does not. I have no idea why that is.

@holiman
Copy link
Contributor Author

holiman commented Oct 7, 2024

I will now switch it a bit, so that:

07 switches from this PR to running #30461 (rebased on master)
08 switches from master to running this PR

@holiman
Copy link
Contributor Author

holiman commented Oct 7, 2024

I will now switch it a bit, so that:

07 switches from this PR to running #30461 (rebased on master) 08 switches from master to running this PR

So, apparently, this made a difference. I don't know why, but now it does show a difference. The only meaningful change I can think of, is that I now set back the minumum mutations from 400 to 100. For account commit, a block's worth of changes should easily surpass 400, but I don't have any better theory. I have double-checked that the commit-hash of the code that was running on 07 was indeed this PR.

So, new charts:

Screenshot 2024-10-07 at 15-31-30 Dual Geth - Grafana

On the left: master then this PR, going from ~5 to ~2ms
On the right: this PR then #30461, going from ~5 to ~3ms

So, now it's showing what I had expected from the start. Here's a zoom in on the last 30 minutes, again this PR on the left, ancestor PR on the right:

Screenshot 2024-10-07 at 15-35-48 Dual Geth - Grafana

trie/committer.go Outdated Show resolved Hide resolved
@holiman holiman added this to the 1.14.12 milestone Oct 13, 2024
@holiman holiman merged commit f4dc753 into ethereum:master Oct 14, 2024
3 checks passed
holiman added a commit that referenced this pull request Nov 19, 2024
This change makes the trie commit operation concurrent, if the number of changes exceed 100. 

Co-authored-by: stevemilk <wangpeculiar@gmail.com>
Co-authored-by: Gary Rong <garyrong0905@gmail.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.

4 participants