perf(iavlx): use NodeID and absolute offsets in branch layout #25482
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## aaronc/iavlx #25482 +/- ##
================================================
- Coverage 52.88% 52.83% -0.05%
================================================
Files 797 798 +1
Lines 64903 64953 +50
================================================
- Hits 34322 34317 -5
- Misses 30581 30636 +55 see 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
iavl/branch_layout.go
Outdated
| SizeBranch = 88 | ||
| ) | ||
|
|
||
| type BranchLayout struct { | ||
| Id NodeID | ||
| Left NodeRef | ||
| Right NodeRef | ||
| Left NodeID | ||
| LeftOffset uint32 // absolute offset | ||
| Right NodeID | ||
| RightOffset uint32 // absolute offset |
There was a problem hiding this comment.
Ah so I can see how this snippet looks totally equivalent to what I posted on slack! But the order actually matters and affects the struct size. So when we place NodeID (8 bytes) next to a uint32 (4 bytes), the compiler actually makes it take up 16 bytes with 4 bytes of padding. And if we do this twice, we use 32 bytes of space. If we put the offsets together, there is no extra padding and the same data only uses 24 bytes. If you're not too familiar with struct alignment and padding, there a bunch of articles you can find by googling.
| SizeBranch = 88 | |
| ) | |
| type BranchLayout struct { | |
| Id NodeID | |
| Left NodeRef | |
| Right NodeRef | |
| Left NodeID | |
| LeftOffset uint32 // absolute offset | |
| Right NodeID | |
| RightOffset uint32 // absolute offset | |
| SizeBranch = 80 | |
| ) | |
| type BranchLayout struct { | |
| Id NodeID | |
| Left NodeID | |
| Right NodeID | |
| LeftOffset uint32 // absolute offset | |
| RightOffset uint32 // absolute offset |
There was a problem hiding this comment.
iavl/compactor.go
Outdated
| branchesWriter: NewStructWriter[BranchLayout](newFiles.branchesFile), | ||
| versionsWriter: NewStructWriter[VersionInfo](newFiles.versionsFile), | ||
| keyCache: make(map[string]uint32), | ||
| leafOffsetRemappings: make(map[uint32]uint32), |
There was a problem hiding this comment.
| leafOffsetRemappings: make(map[uint32]uint32), |
We no longer need this
There was a problem hiding this comment.
iavl/changeset.go
Outdated
| leftPtr := cr.resolveNodeID(layout.Left) | ||
| rightPtr := cr.resolveNodeID(layout.Right) | ||
|
|
||
| return &BranchPersisted{ | ||
| layout: layout, | ||
| store: cr, | ||
| selfIdx: actualIdx, | ||
| leftPtr: leftPtr, | ||
| rightPtr: rightPtr, |
There was a problem hiding this comment.
We actually can delete the leftPtr, rightPtr and I think even selfIdx fields from BranchPersisted now. They're longer needed because BranchLayout now includes everything we need.
There was a problem hiding this comment.
removed the fields, and updated resolve to match new data
|
@technicallyty your pull request is missing a changelog! |
| defer cs.Unpin() | ||
|
|
||
| // get version data | ||
| version := uint32(nodeId.Version()) |
There was a problem hiding this comment.
This should be at the top of the func, no need to call Version() twice
Description
Closes: #25474
updates iavlx to use absolute offsets in branch layout