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

Fix spurious allocation in hasher.Merkleize #171

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

karalabe
Copy link
Contributor

This PR fixed a leaky allocation that happens in merkleizeImpl.

Specifically, https://github.com/ferranbt/fastssz/blob/main/hasher.go#L379 expands the input slice:

		if oddNodeLength {
			// is odd length
			input = append(input, zeroHashes[i][:]...)
			layerLen++
		}

but the problem is that the input slice is a sub-slice of h.buf, so if the expansion needs to allocate a new buffer because we're at capacity, then the newly allocated memory area gets thrown away and will not be piled into h.buf to be used subsequently. The result is that calls in a loop will allocate in a loop.

You can verify this by hashing DepositData for example, where the last field is 96 bytes, requiring a 32 byte expansion, but since it's at the end of the buffer, it will trigger the case.

@karalabe
Copy link
Contributor Author

Here's a repro for the bug and the fix.

func BenchmarkHashTreeRootAllocBug(b *testing.B) {
	obj := new(DepositData)
	readValidGenericSSZ(nil, "../eth2.0-spec-tests/tests/mainnet/deneb/ssz_static/DepositData/ssz_random/case_4", obj)

	b.ReportAllocs()
	b.ResetTimer()

	hh := ssz.NewHasherWithHashFn(gohashtree.HashByteSlice)
	for i := 0; i < b.N; i++ {
		obj.HashTreeRootWith(hh)
		hh.Reset()
	}
}

Without the PR:

go test ./spectests --run=NONE --bench=BenchmarkHashTreeRootAllocBug

BenchmarkHashTreeRootAllocBug-12    	 3209919	       358.4 ns/op	     192 B/op	       1 allocs/op

With the PR:

go test ./spectests --run=NONE --bench=BenchmarkHashTreeRootAllocBug

BenchmarkHashTreeRootAllocBug-12    	 3425565	       335.1 ns/op	       0 B/op	       0 allocs/op

@ferranbt ferranbt merged commit 8e1c57a into ferranbt:main Jul 10, 2024
1 check passed
Copy link

@Pjrich1313 Pjrich1313 left a comment

Choose a reason for hiding this comment

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

Please help support

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