perf: do less allocs in hasher #287
Conversation
WalkthroughThe pull request focuses on optimizing the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| 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): |
There was a problem hiding this comment.
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.
| @@ -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()) | |||
There was a problem hiding this comment.
We're appending hash to the res so adding place for hash.
|
|
||
| return digest | ||
| digest := make([]byte, zeroSize, fullSize) |
There was a problem hiding this comment.
Literally what comment above says: we need zeroed 2 n.NamespaceLen + space for hash.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hasher.go (1)
308-314: Good optimization with room for minor improvementThe changes effectively reduce allocations through pre-allocation and direct writes. However, we could further optimize the slice allocation.
Consider this minor optimization to reduce the number of append operations:
-res := make([]byte, 0, len(minNs)+len(maxNs)+h.Size()) -res = append(res, minNs...) -res = append(res, maxNs...) +totalSize := len(minNs)+len(maxNs)+h.Size() +res := make([]byte, len(minNs)+len(maxNs), totalSize) +copy(res, minNs) +copy(res[len(minNs):], maxNs)This approach:
- Pre-allocates the slice with the correct size for minNs and maxNs
- Uses copy instead of append, which can be more efficient
- Maintains the same capacity for the final hash
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hasher.go(3 hunks)
🔇 Additional comments (2)
hasher.go (2)
157-162: Excellent optimization of slice allocation!The changes effectively reduce memory allocations by:
- Pre-allocating the slice with exact capacity
- Utilizing Go's zero-value initialization
- Eliminating unnecessary append operations
This aligns well with the PR's performance objectives.
195-196: Efficient optimization of hash computation!Direct writes to the hasher eliminate unnecessary slice allocations while maintaining the same hashing semantics. This is a clean optimization that contributes to the observed performance improvements.
There was a problem hiding this comment.
LGTM with two optional changes (or FLUPs):
- Refactored
EmptyRootunit test to confirm that the empty root remains exactly the same before and after this PR test: empty root #288 - Remove all the
nolint:errcheck
Thanks for including the perf analysis in the PR description 👍 great work
|
|
||
| return digest | ||
| digest := make([]byte, zeroSize, fullSize) | ||
| return n.baseHasher.Sum(digest) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Done. Have included your test, thank you!
hasher.go
Outdated
| h.Write([]byte{LeafPrefix}) //nolint:errcheck | ||
| h.Write(ndata) //nolint:errcheck |
There was a problem hiding this comment.
nit: there's a lint disable on line 180. Can we remove all the lint disables and actually check for errors?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hasher_test.go (1)
904-905: Fix variable naming inconsistencies.There are two naming issues:
nIDSziehas a typo (should benIDSize)ignoreMaxNSis inconsistent with the codebase style (should beignoreMaxNs)Apply this diff to fix the naming:
- nIDSzie := 1 - ignoreMaxNS := true + nIDSize := 1 + ignoreMaxNs := true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hasher.go(3 hunks)hasher_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hasher.go
🔇 Additional comments (1)
hasher_test.go (1)
903-927: Well-structured test implementation!The test function is well-designed with:
- Clear sub-tests for verifying both correctness and consistency
- Good edge case coverage testing hasher state independence
- Clear comments explaining the test purposes
Have found few places after #285 that can be improved a bit more without going into enigma mode™:
Summary by CodeRabbit
TestEmptyRootfunction with sub-tests for better validation.