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

perf: do less allocs in hasher #287

Merged
merged 3 commits into from
Jan 20, 2025
Merged

perf: do less allocs in hasher #287

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0103f9
perf: do less allocs in hasher
cristaloleg Jan 17, 2025
192f6d2
review suggestions
cristaloleg Jan 18, 2025
7301b8c
rabbit suggestions
cristaloleg Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 11 additions & 18 deletions hasher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,12 @@ func (n *NmtHasher) BlockSize() int {

func (n *NmtHasher) EmptyRoot() []byte {
n.baseHasher.Reset()
h := n.baseHasher.Sum(nil)
digest := append(make([]byte, int(n.NamespaceLen)*2), h...)
// make returns a zeroed slice, exactly what we need for the (nID || nID)
zeroSize := int(n.NamespaceLen) * 2
fullSize := zeroSize + n.baseHasher.Size()

return digest
digest := make([]byte, zeroSize, fullSize)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Literally what comment above says: we need zeroed 2 n.NamespaceLen + space for hash.

return n.baseHasher.Sum(digest)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wrote a unit test to verify this refactor doesn't modify the returned empty root: #288

Didn't want to push to your branch without permission but feel free to include in this PR if it helps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Have included your test, thank you!

}

// ValidateLeaf verifies if data is namespaced and returns an error if not.
Expand Down Expand Up @@ -190,11 +192,8 @@ func (n *NmtHasher) HashLeaf(ndata []byte) ([]byte, error) {
minMaxNIDs = append(minMaxNIDs, nID...) // nID
minMaxNIDs = append(minMaxNIDs, nID...) // nID || nID

// add LeafPrefix to the ndata
leafPrefixedNData := make([]byte, 0, len(ndata)+1)
leafPrefixedNData = append(leafPrefixedNData, LeafPrefix)
leafPrefixedNData = append(leafPrefixedNData, ndata...)
h.Write(leafPrefixedNData)
h.Write([]byte{LeafPrefix}) //nolint:errcheck
h.Write(ndata) //nolint:errcheck
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: there's a lint disable on line 180. Can we remove all the lint disables and actually check for errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. TBH there is no sense to check 'cause hasher never returns any errors. It's even stated in crypto.Hash doc:

type Hash interface {
	// Write (via the embedded io.Writer interface) adds more data to the running hash.
	// It never returns an error.
	io.Writer

So we can drop this for sure (including line 180).


// compute h(LeafPrefix || ndata) and append it to the minMaxNIDs
nameSpacedHash := h.Sum(minMaxNIDs) // nID || nID || h(LeafPrefix || ndata)
Expand Down Expand Up @@ -306,19 +305,13 @@ func (n *NmtHasher) HashNode(left, right []byte) ([]byte, error) {
// compute the namespace range of the parent node
minNs, maxNs := computeNsRange(lRange.Min, lRange.Max, rRange.Min, rRange.Max, n.ignoreMaxNs, n.precomputedMaxNs)

res := make([]byte, 0, len(minNs)*2)
res := make([]byte, 0, len(minNs)+len(maxNs)+h.Size())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're appending hash to the res so adding place for hash.

res = append(res, minNs...)
res = append(res, maxNs...)

// Note this seems a little faster than calling several Write()s on the
// underlying Hash function (see:
// https://github.com/google/trillian/pull/1503):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment is outdated. For the last ~5 years compiler became better and doing a few .Write() is cheaper then allocating a big slice and copying into it.

data := make([]byte, 0, 1+len(left)+len(right))
data = append(data, NodePrefix)
data = append(data, left...)
data = append(data, right...)
//nolint:errcheck
h.Write(data)
h.Write([]byte{NodePrefix}) //nolint:errcheck
h.Write(left) //nolint:errcheck
h.Write(right) //nolint:errcheck
return h.Sum(res), nil
}

Expand Down
Loading