Skip to content

Conversation

@aaronc
Copy link
Member

@aaronc aaronc commented Dec 8, 2025

Description

Whenever we memory map a file we obtain a virtual memory address which becomes invalid when we unmap the file. Mapping and unmapping happen continously throughout the lifecycle of IAVLX changesets and if we reference an unmapped memory address there will be a system fault.

In order to allow for concurrent access to memory mapped files in IAVLX (a critical optimization), we use a reference counting system to make sure that the cleanup thread doesn't unmap files when the mapped memory is still in use.

In the original IAVLX implementation, this reference counting happened internally within the Changeset type in every single getter method. This was pretty messy and verbose and I wouldn't want to subject anyone to reviewing that code the way it is. In addition, it was sub-optimal performance wise because we were forced to copy any transient memory buffers which increases allocation which increases pressure on the garbage collector.

This redesign moves the responsibility for pinning and unpinning mapped memory out to the edges. While this does mean that there are a bunch of defer pin.Unpin calls whenever we access a node, the pattern itself is simpler and more like opening and closing a file. Essentially, whenever you're acess a Node you should expect that you have file like access to it and must effectively "close" it, which is sort of the case if it is mapped to a file. At the edges, however, this allows us to directly access memory mapped buffers via the UnsafeBytes abstraction which should make it clear that if you want to retain a non-transient reference to this "memory" you must copy it. This means that in query, mutation and hashing code we can now do key comparison and hash accumulation without extra allocation.

In memiavl, they had Hash and SafeHash methods on the Node interface itself to account for this behavior, but the meaning wasn't very clear or obvious. I think assuming these bytes are unsafe to retain is a better starting point. Thus the UnsafeBytes wrapper is used for returning bytes from Key, Value and Hash methods because these []byte arrays may reference mmaps.

@codecov
Copy link

codecov bot commented Dec 8, 2025

Codecov Report

❌ Patch coverage is 89.18919% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.22%. Comparing base (f2d4a98) to head (02f25c9).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
iavl/internal/mem_node.go 82.35% 3 Missing ⚠️
iavl/internal/node_pointer.go 66.66% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #25657      +/-   ##
==========================================
- Coverage   70.42%   70.22%   -0.21%     
==========================================
  Files         833      835       +2     
  Lines       54395    54414      +19     
==========================================
- Hits        38310    38212      -98     
- Misses      16085    16202     +117     
Files with missing lines Coverage Δ
iavl/internal/pin.go 100.00% <100.00%> (ø)
iavl/internal/unsafe_bytes.go 100.00% <100.00%> (ø)
iavl/internal/node_pointer.go 90.90% <66.66%> (ø)
iavl/internal/mem_node.go 94.44% <82.35%> (+0.21%) ⬆️

... and 3 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aaronc aaronc changed the title feat(iavlx): add Pin and UnsafeBytes design for managing mmaps feat(iavl): add Pin and UnsafeBytes design for managing mmaps Dec 8, 2025
@aaronc aaronc marked this pull request as ready for review December 9, 2025 00:41
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2025

@aaronc your pull request is missing a changelog!

@technicallyty technicallyty self-requested a review December 9, 2025 00:47
@aaronc aaronc enabled auto-merge December 11, 2025 02:33
@aaronc aaronc added this pull request to the merge queue Dec 11, 2025
Merged via the queue into main with commit 6065146 Dec 11, 2025
41 checks passed
@aaronc aaronc deleted the aaronc/iavlx-part7 branch December 11, 2025 02:46
warpbuild-benchmark-bot bot added a commit to WarpBuilds/cosmos-sdk that referenced this pull request Dec 11, 2025
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.

4 participants