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/state/snapshot: use AddHash/ContainHash instead of Hasher interface #28849

Merged
merged 5 commits into from
Jan 23, 2024

Conversation

MariusVanDerWijden
Copy link
Member

This PR switches from using the Hasher interface to add/query the bloomfilter to implementing it as methods.
This significantly reduces the allocations for Search and Rebloom.
The allocations for the storageBloom alone was ~7% of all allocations (in a quite contrived scenario)
26666581 26666581 243: dl.diffed.Add(storageBloomHasher{accountHash, storageHash})
14812039 14812039 375: hit := dl.diffed.Contains(storageBloomHasher{accountHash, storageHash})
14828423 14849723 (flat, cum) 2.50% of Total
26666581 26672121 (flat, cum) 4.49% of Total

The added benefit of reduced allocations also surfaces in the benchmark:

Master:
BenchmarkSearchSlot-8   	 4042929	       341.6 ns/op	     160 B/op	       3 allocs/op
PR:
BenchmarkSearchSlot-8   	 6573976	       180.8 ns/op	      64 B/op	       1 allocs/op

Reduces the allocations quite significantly, before:
  26666581   26666581    243:			dl.diffed.Add(storageBloomHasher{accountHash, storageHash})
core/state/snapshot/difflayer.go Outdated Show resolved Hide resolved
core/state/snapshot/difflayer.go Outdated Show resolved Hide resolved
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM (with minor nit)

func (f stateBloomHasher) BlockSize() int { panic("not implemented") }
func (f stateBloomHasher) Size() int { return 8 }
func (f stateBloomHasher) Sum64() uint64 { return binary.BigEndian.Uint64(f) }
// stateBloomHasher is used to convert a trie hash or contract code hash into a 64 bit mini hash.
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that these no longer are types (objects), but rather functions (verbs), perhaps they should be renamed from stateBloomHasher into stateBloomHash ?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@holiman holiman added this to the 1.13.11 milestone Jan 23, 2024
@holiman holiman merged commit c89a3da into ethereum:master Jan 23, 2024
2 of 3 checks passed
Dergarcon pushed a commit to specialmechanisms/mev-geth-0x2mev that referenced this pull request Jan 31, 2024
…ace (ethereum#28849)

This change switches from using the `Hasher` interface to add/query the bloomfilter to implementing it as methods.
This significantly reduces the allocations for Search and Rebloom.
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.

3 participants