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

core/types: create block's bloom by merging receipts' bloom #31129

Merged
merged 3 commits into from
Feb 13, 2025

Conversation

minh-bq
Copy link
Contributor

@minh-bq minh-bq commented Feb 5, 2025

Currently, when calculating block's bloom, we loop through all the receipt logs to calculate the hash value. However, normally, after going through applyTransaction, the receipt's bloom is already calculated based on the receipt log, so the block's bloom can be calculated by just ORing these receipt's blooms.

goos: darwin
goarch: arm64
pkg: github.com/ethereum/go-ethereum/core/types
cpu: Apple M1 Pro
BenchmarkCreateBloom
BenchmarkCreateBloom/small
BenchmarkCreateBloom/small-10             810922              1481 ns/op             104 B/op          5 allocs/op
BenchmarkCreateBloom/large
BenchmarkCreateBloom/large-10               8173            143764 ns/op            9614 B/op        401 allocs/op
BenchmarkCreateBloom/small-mergebloom
BenchmarkCreateBloom/small-mergebloom-10                 5178918               232.0 ns/op             0 B/op          0 allocs/op
BenchmarkCreateBloom/large-mergebloom
BenchmarkCreateBloom/large-mergebloom-10                   54110             22207 ns/op               0 B/op          0 allocs/op

Currently, when calculating block's bloom, we loop through all the receipt logs
to calculate the hash value. However, normally, after going through
applyTransaction, the receipt's bloom is already calculated based on the
receipt log, so the block's bloom can be calculated by just ORing these
receipt's blooms.

goos: darwin
goarch: arm64
pkg: github.com/ethereum/go-ethereum/core/types
cpu: Apple M1 Pro
BenchmarkCreateBloom
BenchmarkCreateBloom/small
BenchmarkCreateBloom/small-10             810922              1481 ns/op             104 B/op          5 allocs/op
BenchmarkCreateBloom/large
BenchmarkCreateBloom/large-10               8173            143764 ns/op            9614 B/op        401 allocs/op
BenchmarkCreateBloom/small-mergebloom
BenchmarkCreateBloom/small-mergebloom-10                 5178918               232.0 ns/op             0 B/op          0 allocs/op
BenchmarkCreateBloom/large-mergebloom
BenchmarkCreateBloom/large-mergebloom-10                   54110             22207 ns/op               0 B/op          0 allocs/op
rjl493456442
rjl493456442 previously approved these changes Feb 5, 2025
@rjl493456442
Copy link
Member

I think it’s a solid idea. I would prefer including it in version 1.15.1 while simultaneously running a full sync for validation.

@fjl fjl added this to the 1.15.1 milestone Feb 5, 2025
@fjl
Copy link
Contributor

fjl commented Feb 11, 2025

We've been discussing in team. The average mainnet block will save computing ~1.2k sha3 hashes with this change. It's not going to make a significant difference in performance, even though the change is a nice idea. On the flip side, it introduces a dependency on Receipt.Bloom that wasn't there before, so we would have to check the whole codebases for areas where that could become a problem.

@fjl fjl removed the status:triage label Feb 11, 2025
@fjl fjl modified the milestones: 1.15.1, 1.15.2 Feb 13, 2025
Copy link
Contributor

@zsfelfoldi zsfelfoldi left a comment

Choose a reason for hiding this comment

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

I did some measurements and calculations based on real mainnet blocks and my rough estimate for the savings in total block processing time was about 0.3-0.5% which is not huge but also not totally negligible considering that there are no really low hanging fruits in terms of performance optimization any more. Small improvements might add up over time. Also the change makes the code somewhat nicer and it's not a big change, I did not find any dangers in relying on Receipt.Bloom so all things considered I'm in favor of merging it.
One nitpick about the benchmark tests: I felt it's weird to have CreateBlooms for the sole purpose of benchmarking it. We usually benchmark the stuff that is used in production code. Measuring the removed method is good for proving your point but once it's not in production code it shouldn't be in the benchmarks either. So I changed it so that we have "createbloom" and "mergebloom" benchmarks, both creating individual receipt blooms and then merging them and checking the results, but the first one puts the creation in the measured loop, the second does this with the merge operation. It gives pretty much the same results btw, as I expected.

@zsfelfoldi zsfelfoldi merged commit 68de26e into ethereum:master Feb 13, 2025
1 of 4 checks passed
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