Intern tree values in a content-addressed store (ENG-5178) - #247
Intern tree values in a content-addressed store (ENG-5178)#247oschwald wants to merge 43 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces hash-based ChangesValue storage and tree integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Tree
participant storeDecoder
participant valueStore
participant Inserter
participant dataWriter
Tree->>storeDecoder: Decode MMDB record
storeDecoder->>valueStore: Intern decoded nodes
Tree->>Inserter: Pass materialized views
Inserter-->>Tree: Return updated value
Tree->>valueStore: Retain or release valueRef
Tree->>dataWriter: Serialize valueRef
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Modver resultThis report was generated by Modver, This PR requires an increase in your module’s major version number. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@audit_test.go`:
- Around line 46-69: Extend TestValueStoreAuditRejectsInvalidPathOwnership with
table-driven subtests that directly corrupt the value-store structures and
invoke auditValueStore. Cover invalid bucket membership, bucket-chain cycles,
duplicate freelist entries, invalid child references, and the full
auditCallerIdentity LRU validation, asserting each case’s specific error text.
Reuse the existing tree setup and corruption patterns where applicable, and
ensure the caller-identity cases exercise all validation branches.
- Around line 13-32: In TestValueStoreRefcountAudit, replace the broad
require.ErrorContains assertion on auditValueStore with an exact error-message
assertion using the expected formatted message, adding fmt to construct it if
needed. Keep the refCount restoration and subsequent successful audit unchanged.
- Around line 34-44: Extend TestRefcountAuditMode after the two Insert calls to
query the combined 1.2.3.0/24 range and assert that its returned record contains
the expected "same" value, verifying that the second insert actually merges the
two half networks rather than only completing without error.
In `@audit.go`:
- Around line 86-90: Update the audit logic around callerIdentity to validate
materializedByIdentity entries without adding them to expected. For each map
entry, verify the mapped ref is live and that its stored identity matches the
map key, while preserving expected accounting only for owning references.
In `@data_section.go`:
- Around line 67-95: Remove the unused remember parameter from
dataWriter.writeValue, delete its conditional rememberOffset block, and update
both callers—maybeWrite and writeOrWritePointer—to invoke writeValue with only
the value reference.
- Around line 128-142: Remove the compatibility methods
dataWriter.WriteOrWritePointer and dataWriter.WriteOrWritePointerString, along
with their associated compatibility comment, since production writes use
maybeWrite and writeValue instead. Ensure no callers or interface requirements
still depend on these methods.
In `@golden_test.go`:
- Around line 63-67: Replace the direct require.Equal comparison in the golden
test with a comparison of the decoded output bytes and enterprise golden
fixture, locating the first differing byte offset and reporting it in the
failure assertion. Preserve the existing tree.WriteTo error check and ensure
matching decoded bytes still pass.
In `@store_decoder.go`:
- Around line 26-33: Update UnmarshalMaxMindDB to release any existing d.result
before assigning the newly decoded ref, while preserving the existing decodeRef
error path. Ensure the stale result is released only when present so repeated
Decode calls balance references correctly.
- Around line 148-230: Update decodeMap to detect duplicate keys after sorting
pairs by key and before building or passing children to internOwnedChildren.
Return a decoding error when adjacent sorted pairs have equal key values,
releasing all retained key and value references first; preserve the existing
successful path for unique keys.
In `@tree_test.go`:
- Around line 89-139: Add successful-insert mutation tests alongside
TestFailedInsertDoesNotCacheCallerIdentity and
TestFailedInsertRangeDoesNotCacheCallerIdentity. For both Insert and
InsertRange, insert a value successfully, mutate the same object in place
without changing its length, insert it again, and assert the lookup returns the
mutated value rather than stale cached data.
In `@value_store_benchmark_test.go`:
- Around line 51-71: Pre-warm the caller-identity cache in the
“equal-shared-nested” benchmark by interning each shallow copy and calling
rememberCallerIdentity before b.ResetTimer, retaining the references needed for
cleanup. Ensure the timed loop reuses those cached identities and only measures
the cache-hit path, while preserving release and allocation reporting behavior.
In `@value_store.go`:
- Around line 647-678: The caller-identity cache assumes inserted values are not
mutated in place, so document this contract in the public Insert and InsertRange
documentation in value_store.go, or change the cache key to include content that
detects same-size mutations. Add coverage in tree_test.go for inserting a value,
mutating it in place without changing its length, and inserting it again,
asserting the second insert stores the mutated content; the existing
failure-path tests do not cover this successful path.
- Around line 385-393: Update the release method around the children snapshot to
obtain the slice from the existing takeChildScratch pool instead of allocating
with append, then return it with putChildScratch after cascading through every
child. Preserve the current snapshot-before-node-reset behavior and
stack-ordered take/put discipline so nested release calls remain safe.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1928c0ef-f664-4837-ad93-5cb28d17a120
📒 Files selected for processing (21)
.github/workflows/go.ymlCHANGELOG.mdaudit.goaudit_test.godata_key.godata_key_test.godata_map.godata_map_test.godata_section.godata_section_test.gogolden_test.gommdbtype/types.gonode.gonode_test.gostore_decoder.gotree.gotree_benchmark_test.gotree_test.govalue_store.govalue_store_benchmark_test.govalue_store_test.go
💤 Files with no reviewable changes (4)
- data_map.go
- data_key.go
- data_key_test.go
- data_map_test.go
| children := append([]valueRef(nil), s.childRefs(node)...) | ||
| s.payloads.release(node.payloadOffset, node.payloadLen) | ||
| s.children.release(node.childrenOffset, node.childrenLen) | ||
| *node = valueNode{} | ||
| s.freeRefs = append(s.freeRefs, ref) | ||
|
|
||
| for _, child := range children { | ||
| s.release(child) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Reuse the child scratch pool for the cascading release.
release allocates a new slice for every container node it frees. Large builds free many containers, so this allocation appears on a hot path. The store already pools child slices through takeChildScratch and putChildScratch, and the take/put discipline is stack-ordered, so a nested release can borrow safely.
♻️ Proposed refactor to pool the released child list
- children := append([]valueRef(nil), s.childRefs(node)...)
+ children := append(s.takeChildScratch(), s.childRefs(node)...)
s.payloads.release(node.payloadOffset, node.payloadLen)
s.children.release(node.childrenOffset, node.childrenLen)
*node = valueNode{}
s.freeRefs = append(s.freeRefs, ref)
for _, child := range children {
s.release(child)
}
+ s.putChildScratch(children)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| children := append([]valueRef(nil), s.childRefs(node)...) | |
| s.payloads.release(node.payloadOffset, node.payloadLen) | |
| s.children.release(node.childrenOffset, node.childrenLen) | |
| *node = valueNode{} | |
| s.freeRefs = append(s.freeRefs, ref) | |
| for _, child := range children { | |
| s.release(child) | |
| } | |
| children := append(s.takeChildScratch(), s.childRefs(node)...) | |
| s.payloads.release(node.payloadOffset, node.payloadLen) | |
| s.children.release(node.childrenOffset, node.childrenLen) | |
| *node = valueNode{} | |
| s.freeRefs = append(s.freeRefs, ref) | |
| for _, child := range children { | |
| s.release(child) | |
| } | |
| s.putChildScratch(children) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@value_store.go` around lines 385 - 393, Update the release method around the
children snapshot to obtain the slice from the existing takeChildScratch pool
instead of allocating with append, then return it with putChildScratch after
cascading through every child. Preserve the current snapshot-before-node-reset
behavior and stack-ordered take/put discipline so nested release calls remain
safe.
0e65802 to
9cd7b87
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
audit_test.go (1)
111-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the LRU link branches of
auditCallerIdentity.This case only triggers the length mismatch at
audit.goline 192. The LRU validation branches remain untested: the non-empty head/tail check, theentry.prev != previousmismatch, theseen[index]duplicate link, and theprevious != s.callerIdentityTailincomplete chain. That logic is the most complex part of the audit.Add subtests that corrupt
callerIdentityHead,callerIdentityTail, and an entryprevvalue, and assert the matching error text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@audit_test.go` around lines 111 - 120, Extend the auditCallerIdentity tests with subtests that independently corrupt callerIdentityHead, callerIdentityTail, and an entry’s prev link, covering the non-empty head/tail validation, prev mismatch, duplicate seen index, and incomplete-chain branches; assert each subtest’s corresponding audit error text while preserving the existing index-mismatch case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@audit_test.go`:
- Around line 101-110: Update the “dead materialized identity” corruption
fixture to derive its invalid valueRef from len(tree.valueStore.nodes), ensuring
the reference remains out of range as the store grows while preserving the
existing dead materialized ref assertion.
- Around line 60-69: Remove the unused ref assignment and `_ = ref` from the
“live ref missing from its bucket” corruption setup; call `requireDataRef(t,
tree)` only if its fixture assertion is required, otherwise remove that call as
well, while preserving `clear(tree.valueStore.buckets)`.
In `@audit.go`:
- Around line 89-93: Add complete bounds validation in audit.go at lines 89-93
and 53-56: in the callerIdentity loop, validate int(entry.ref) is below
len(s.nodes) before indexing expected and return the existing diagnostic error
style; before t.nodeAt(index), validate signed index is non-negative as well as
within the existing upper bound. Update both sites to prevent corrupted values
from causing panics.
In `@golden_test.go`:
- Around line 63-76: Update the golden comparison failure in the test around
tree.WriteTo to report the first differing expected and actual bytes, along with
both hex string lengths. Handle truncation or length mismatches explicitly so
the message remains accurate when the common prefix reaches the shorter string.
In `@mmdbtype/types.go`:
- Around line 440-442: Update the documentation comment for the exported Pointer
type to explicitly name the mmdbwriter.Tree insertion method that returns an
error when Pointer values reach tree storage, replacing the ambiguous “Values
that reach tree storage” wording while preserving the existing contract.
In `@store_decoder.go`:
- Around line 230-257: Cap the initial capacity hint derived from size in
decodeSlice before allocating children, using the same bounded approach as
decodeMap; preserve the existing iterator error cleanup and decoding behavior.
Prefer reusing d.store.takeChildScratch() for the child buffer if that is
decodeMap’s established pattern, avoiding an unbounded per-slice allocation.
In `@value_store_test.go`:
- Around line 430-440: Update TestPutPairScratchClearsEntries to assert that the
pairs returned by takePairScratch have non-zero capacity before iterating over
pairs[:cap(pairs)], ensuring the zero-value checks cannot pass vacuously when
the scratch pool is empty.
---
Duplicate comments:
In `@audit_test.go`:
- Around line 111-120: Extend the auditCallerIdentity tests with subtests that
independently corrupt callerIdentityHead, callerIdentityTail, and an entry’s
prev link, covering the non-empty head/tail validation, prev mismatch, duplicate
seen index, and incomplete-chain branches; assert each subtest’s corresponding
audit error text while preserving the existing index-mismatch case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 106e2774-7996-4fa7-8f49-f0e8f1c165ff
📒 Files selected for processing (14)
CHANGELOG.mdaudit.goaudit_test.godata_section.godata_section_test.gogolden_test.gommdbtype/types.gonode.gostore_decoder.gotree.gotree_test.govalue_store.govalue_store_benchmark_test.govalue_store_test.go
| { | ||
| name: "live ref missing from its bucket", | ||
| corrupt: func(t *testing.T, tree *Tree) { | ||
| t.Helper() | ||
| ref := requireDataRef(t, tree) | ||
| clear(tree.valueStore.buckets) | ||
| _ = ref | ||
| }, | ||
| want: "in 0 hash buckets", | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove the unused ref variable.
The corruption for this case is clear(tree.valueStore.buckets). The ref value is not used, and _ = ref is a leftover artifact. Call requireDataRef for its fixture assertion only, or drop it.
♻️ Proposed change
corrupt: func(t *testing.T, tree *Tree) {
t.Helper()
- ref := requireDataRef(t, tree)
+ requireDataRef(t, tree)
clear(tree.valueStore.buckets)
- _ = ref
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| name: "live ref missing from its bucket", | |
| corrupt: func(t *testing.T, tree *Tree) { | |
| t.Helper() | |
| ref := requireDataRef(t, tree) | |
| clear(tree.valueStore.buckets) | |
| _ = ref | |
| }, | |
| want: "in 0 hash buckets", | |
| }, | |
| { | |
| name: "live ref missing from its bucket", | |
| corrupt: func(t *testing.T, tree *Tree) { | |
| t.Helper() | |
| requireDataRef(t, tree) | |
| clear(tree.valueStore.buckets) | |
| }, | |
| want: "in 0 hash buckets", | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@audit_test.go` around lines 60 - 69, Remove the unused ref assignment and `_
= ref` from the “live ref missing from its bucket” corruption setup; call
`requireDataRef(t, tree)` only if its fixture assertion is required, otherwise
remove that call as well, while preserving `clear(tree.valueStore.buckets)`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tree_benchmark_test.go`:
- Line 37: Move every custom ReportMetric invocation, including metrics emitted
by helpers, to execute after the corresponding b.Loop() completes. Apply this to
tree_benchmark_test.go at lines 37-37, 48-48, 64-64, 83-83, 118-118, 129-129,
144-144, 172-172, and 217-217, and value_store_benchmark_test.go at lines 25-25,
138-138, and 175-175; preserve each metric’s existing values and reporting
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2551f5f1-9014-40ab-b071-3820a019e029
📒 Files selected for processing (6)
CHANGELOG.mdaudit.goaudit_test.gotree.gotree_benchmark_test.govalue_store_benchmark_test.go
be15cad to
08678b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@audit.go`:
- Around line 79-82: Replace the potentially narrowing int-based bounds checks
with uint64(ref) >= uint64(len(s.nodes)) for all six reference-validation sites
in audit.go: external, bucket, freelist, child, caller-identity, and
materialized-identity checks at audit.go lines 79-82, 96-99, 112-115, 141-146,
181-184, and 193-196. Add a 386 regression test using a high-bit valueRef and
verify each path returns the diagnostic error without indexing or panicking.
In `@CHANGELOG.md`:
- Around line 45-48: Update the changelog text describing values passed to
inserter functions so it distinguishes the existing store value from the new
caller-provided value: only the existing value is a shared, read-only store
view, while the new value is passed directly from the caller. Preserve the
guidance to copy the existing value before modifying it and not modify values
after insertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4dc9dc95-071f-4d8b-a765-5ee9c99356f3
📒 Files selected for processing (11)
CHANGELOG.mdaudit.goaudit_test.gogolden_test.gommdbtype/types.gostore_decoder.gotree.gotree_benchmark_test.govalue_store.govalue_store_benchmark_test.govalue_store_test.go
08678b9 to
bcbe0b9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
audit_test.go (1)
161-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the LRU-chain and bucket-membership branches.
The table covers the caller-identity length mismatch only. These branches in
audit.gostay untested:
"caller identity audit found an invalid LRU link at %d"and"caller identity audit found an inconsistent entry at %d"(corruptcallerIdentity[index].nextor.prev)."caller identity audit found an incomplete LRU chain"(setcallerIdentityTailto a wrong index)."caller identity audit found a head or tail in an empty cache"."found ref %d in the wrong hash bucket"(changenodes[ref].hashwithout moving the bucket entry)."found live ref %d on the freelist".
auditCallerIdentityis the most complex function inaudit.go, and its link validation has no test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@audit_test.go` around lines 161 - 171, Extend the audit test table with corruption cases covering the untested branches in auditCallerIdentity: modify callerIdentity[index].next or .prev for invalid-link and inconsistent-entry errors, set callerIdentityTail to an invalid index for incomplete-chain detection, configure a non-empty head or tail for an empty cache, alter nodes[ref].hash without moving its bucket entry, and place a live ref on the freelist. Assert each case’s corresponding audit error substring.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@audit_test.go`:
- Around line 161-171: Extend the audit test table with corruption cases
covering the untested branches in auditCallerIdentity: modify
callerIdentity[index].next or .prev for invalid-link and inconsistent-entry
errors, set callerIdentityTail to an invalid index for incomplete-chain
detection, configure a non-empty head or tail for an empty cache, alter
nodes[ref].hash without moving its bucket entry, and place a live ref on the
freelist. Assert each case’s corresponding audit error substring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0557ddf4-64d1-4180-93f6-5a97cdb8512c
📒 Files selected for processing (4)
CHANGELOG.mdaudit.goaudit_test.gotree.go
The interned value store must not change output bytes. Capture the current serialization of City-shaped and Enterprise-shaped values at a fixed build epoch so every later commit is checked against the pre-store encoding, including node order, data emission order, greedy pointers, sorted map keys, and metadata. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The store hash-conses every node of an MMDB value tree - scalars, strings, sub-maps, and slices - so each distinct value is held once. Payloads keep the final wire encoding in a shared byte arena and containers keep sorted child reference lists in a ref arena, so the inserted Go object graph does not have to stay live. Nodes are reference counted with cascading release, and freed slots and arena extents are reused. Buckets are keyed by a seeded maphash over kind, payload, and child hashes, and candidates are always compared exactly, so hash collisions cannot alter identity. Materialized views are memoized per node and registered by identity, and caller-supplied container values go through a bounded LRU of strong references, so repeated inserts of the same object skip re-interning without the pointer-reuse hazard an address key alone would have. Pointer-backed values are normalized through dereferenceDataType as before, and nil typed pointers are rejected. Nothing uses the store yet; the tree switches to it separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Records now hold a uint32 reference into the value store instead of a pointer to a retained mmdbtype graph. This shrinks every record, releases the inserted Go objects once their wire encoding is interned, and turns sibling-merge comparison into a scalar equality check. The write path indexes offsets by reference, so serialization no longer hashes or sorts anything: containers emit their sorted child references and the greedy pointer rule reads a dense offset table. Metadata is interned and emitted the same way. Output bytes are unchanged, which the golden test pins. Inserters receive materialized store views. A result equal to the existing value interns back to the same reference, replacing the old Equal fast path, and pure-inserter memoization keeps its semantics with references as keys. The dataMap, the structural hasher, and the exact wire comparison are superseded by the store and removed; pointer normalization and the slice identity helper move to the store. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Load previously unmarshaled every record into an mmdbtype graph and cached the graphs by data offset for the life of the load. The decoder now implements mmdbdata.Unmarshaler against the value store directly, so records intern as they are read and no intermediate Go values are built. The offset cache holds one store reference per source offset, which keeps networks that share a data record on one shared reference and drops the cache when loading finishes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Setting MMDBWRITER_REFCOUNT_AUDIT makes the tree verify every ownership edge after each successful insert and load: tree records, caller identity entries, and parent-to-child edges must account for every stored reference count, buckets must contain exactly the live nodes, and node and path ownership in the tree must be unique and acyclic. The audit is far too slow for production but turns a leaked or dropped reference into an immediate failure anywhere in the test suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comparing an int against math.MaxUint32 and shifting 1 by a 32-bit record size do not compile or silently wrap on GOARCH=386. Do the offset-limit, node-count, and record-capacity comparisons in 64-bit arithmetic so the package builds and its tests pass on 32-bit platforms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The reference-count audit turns any ownership mistake in the value store into a test failure, and the store's arena arithmetic must keep working on 32-bit platforms. Run the test suite once with MMDBWRITER_REFCOUNT_AUDIT set and once with GOARCH=386. Neither step uses -race: the main test step already covers it, and it is unsupported on 386. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The production Enterprise build loads a City-scale database and rewrites every record through several merge overlay passes. Model that shape directly so the load path, the merge path, and the identity caches are measured together at a network count large enough to matter, rather than only through the smaller synthetic insert benchmarks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every scalar intern allocated its own encode buffer and every container intern allocated key and child working slices, which made allocation churn the dominant insert cost. Keep one reusable encode buffer on the store, which is safe because scalars do not nest and the arena copies payloads, and pool the container slices, which nest and so are taken and returned per call. This halves the cost of interning a distinct Enterprise-shaped record. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Interning registered a caller-identity entry for every nested map and slice of every value. On unique-value workloads that registration never pays off: it grows the identity map and entry list for objects that are never presented again, it accounted for a fifth of insert allocations, and each entry pins the caller's object graph. Register only the value the caller passed to the insert; nested interning still consults both identity caches, so shared sub-containers and store-materialized views keep their fast path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Passing each key through the DataType parameter boxed the string and made key interning the largest remaining allocation source on the insert path. A concrete String entry point writes the key through the shared scratch buffer directly. The decoder uses it for map keys and string values as well. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each hash bucket held a slice of references, costing an allocation per interned node and a pointer-bearing map the garbage collector had to scan. Store the head reference in the bucket map and chain collisions through a field on the node, as the previous dataMap did. The audit walks the chains with a cycle bound. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Interning a map iterated its keys, sorted them, and then looked every value up again, paying a second map access per key. Collect the pairs during one iteration and sort those instead, keeping the scratch pooling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The production overlay passes decode a fresh value for every source network. Presenting one long-lived value per layer let the caller identity cache absorb work the real build repeats per network, which hid a per-insert interning cost that the production A/B run then exposed. Copy the overlay value for each insert instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every InsertFunc call interned the caller's value and materialized a view of it just to hand the view to the inserter. When each insert brings a fresh value, as the Enterprise overlay passes do for every source network, that is a full intern, a materialization, and a cascade release per insert with nothing reused. Give the inserter the caller's value as passed, and intern only inserter results and direct-replace values. This matches the previous writer, which also stored only the result. On the overlay-shaped benchmark this removes most of the remaining regression against the pre-store writer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
writeMetadataPatchedDB assumed the value byte follows the key and its control byte. A metadata layout change would silently patch the wrong byte instead of failing the helper. Assert the one-byte uint16 control byte before patching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The environment variable was the only switch and it is process-wide, so a caller could not enable the audit for one tree without slowing every other tree in the process. Options.RefcountAudit scopes the audit to a single tree. MMDBWRITER_REFCOUNT_AUDIT still turns it on process-wide.
b.Loop excludes setup before the loop automatically, so the ResetTimer calls are gone. The three indexed loops keep their index as a manual counter.
The first b.Loop call resets the timer, and ResetTimer deletes user-reported metrics, so metrics reported before the loop never reach the output. The old explicit ResetTimer calls had the same effect, so these metrics have never appeared. Reporting after the loop is untimed because Loop stops the timer on its last iteration. Building the shape trees after the loop instead of before also changes GC pacing during the timed region, so ns/op results are not comparable with earlier runs of these benchmarks.
insert and insertRange duplicated the same refcount-sensitive ordering: register the caller identity on success, release the insertRecord references, return the insertion error, and run the audit. finishInsert centralizes the ordering and its rationale. Benchmarks show no measurable cost.
bcbe0b9 to
33c8ddb
Compare
There was a problem hiding this comment.
Pull request overview
Replaces object-graph record storage with a refcounted, content-addressed value store to reduce memory and accelerate serialization.
Changes:
- Interns scalar and container values with collision-safe canonicalization.
- Adds direct database decoding, reference auditing, and ref-based serialization.
- Expands correctness, golden-output, performance, audit, and 32-bit testing.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
value_store.go |
Implements the interned value DAG and arenas. |
value_store_test.go |
Tests canonicalization, identities, and refcounts. |
value_store_benchmark_test.go |
Benchmarks store and tree pipelines. |
tree.go |
Integrates the store into tree operations. |
tree_test.go |
Tests insertion, loading, and store integration. |
tree_benchmark_test.go |
Adds production-shaped benchmarks. |
store_decoder.go |
Decodes MMDB values directly into the store. |
node.go |
Replaces value pointers with compact references. |
node_test.go |
Updates node merge and boundary tests. |
mmdbtype/types.go |
Documents validation and sharing semantics. |
golden_test.go |
Pins byte-identical output. |
data_section.go |
Serializes canonical references without hashing. |
data_section_test.go |
Tests offsets, pointers, and container headers. |
data_map.go |
Removes the previous value map. |
data_map_test.go |
Removes obsolete map tests. |
data_key.go |
Removes the previous structural hasher. |
data_key_test.go |
Removes obsolete hashing tests. |
CHANGELOG.md |
Documents behavior and compatibility changes. |
audit.go |
Implements ownership and refcount auditing. |
audit_test.go |
Tests audit mode and corruption detection. |
.github/workflows/go.yml |
Adds audit and 386 test runs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
horgh
left a comment
horgh
left a comment
There was a problem hiding this comment.
Seems good to me. Here are Claude's comments, they look overall minor but 🤷
| if err == nil { | ||
| t.valueStore.rememberCallerIdentity(iRec.callerValue, iRec.value) | ||
| } | ||
| iRec.releaseResolved() |
There was a problem hiding this comment.
Critical: a panicking inserter leaks store references.
On origin/main both insert and insertRange held this with
defer iRec.releaseResolved(). Commit b85a917 replaced the defer with an
unconditional call so the audit would see a balanced store, so anything that
panics between newInsertRecord and finishInsert now skips the release
entirely. The likeliest trigger is a caller-supplied inserter, which the tree
invokes directly at node.go:118; store panics (retain overflow, release
underflow, node() on an invalid ref) reach the same path.
Reproduced on this head — two distinct values under a /24, InsertPureFunc
whose inserter panics on its second call, panic recovered, then
tree.auditValueStore():
refcount audit for ref 1: stored 1, expected 0
The memo's retained key and result reference are pinned for the life of the
tree. Nothing reports it: Get and WriteTo keep working and output stays
correct, so a build pipeline that recovers around Insert to skip a bad
record leaks the whole value subgraph every time. It also misdirects
debugging — the opt-in audit then fails on a later, unrelated insert,
naming a value the failing insert never touched.
Suggested fix: make releaseResolved idempotent (clear iRec.value,
iRec.memo, iRec.memoSet as it releases) and restore the deferred call,
keeping the explicit call here so the audit still runs after release:
defer iRec.releaseResolved() // no-op once finishInsert has run
err = t.insertPrepared(prefix, iRec)
return t.finishInsert(iRec, err)insertNormalizedRef still uses defer, so Load is unaffected. Only
insert (tree.go:428) and insertRange (tree.go:717) regressed.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed in 7b3d296. releaseResolved is idempotent now and insert/insertRange defer it again.
🤖 Reply by Claude (Claude Code) on behalf of Greg.
There was a problem hiding this comment.
I am not sure this was worth fixing. If you are getting panics, recovering from them, and then continuing on like nothing happened, that is kind of on you. Anyway, Claude decided to fix it.
| s.buckets[node.hash] = node.nextInBucket | ||
| } | ||
| } else { | ||
| for prev := head; prev != nilValueRef; prev = s.nodes[prev].nextInBucket { |
There was a problem hiding this comment.
release silently tolerates a node missing from its bucket chain.
If current is not in the chain — or s.buckets[node.hash] is absent
entirely, so head is nilValueRef and the loop body never runs — execution
falls through to *node = valueNode{} and s.freeRefs = append(...). The
slot is zeroed and queued for reuse while a bucket chain still points at it.
The code this replaced checked for exactly this and refused to continue
(git show origin/main:data_map.go:278):
if !unlinked {
panic("mmdbwriter: dataMap.remove called on a value missing from its bucket")
}with the doc comment explaining that continuing "would spread the damage
silently." Because internNode compares candidates exactly, content stays
correct, so any future desync of node.hash from its bucket degrades from an
immediate localized panic into silent dedupe loss and leaked live nodes. The
audit does check this (audit.go:96-108), which is evidence the invariant is
worth checking — but the audit is opt-in. Tracking whether the unlink
succeeded and panicking otherwise restores the old guard at zero happy-path
cost.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed in 65c55ed. The guard is back and panics with the ref named.
🤖 Reply by Claude (Claude Code) on behalf of Greg.
| } | ||
| iRec.releaseResolved() | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Failed inserts skip the audit, so CI never checks the error paths.
This early return means maybeAuditValueStore runs only after a successful
insert. The MMDBWRITER_REFCOUNT_AUDIT=1 CI job is the main defense for
reference counting, and it therefore never covers the error paths — which is
exactly where leaks and over-releases live. It is also why the panic leak
above survives CI.
Only two tests assert anything about post-failure ownership
(tree_test.go:2432, tree_test.go:334), and both are weaker than the audit:
neither detects an over-release, nor a leak while other values stay live.
Auditing after expected-error inserts too — ideally one table over the failure
shapes (nil nested Uint128, negative Uint128 from an inserter mid-walk,
inserter error mid-walk, reserved-network insert, partial InsertRange) —
would close it.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed in 0e02882. finishInsert audits after failed inserts too, and a table of failure shapes covers the error paths.
🤖 Reply by Claude (Claude Code) on behalf of Greg.
| if err != nil { | ||
| return err | ||
| } | ||
| return t.maybeAuditValueStore() |
There was a problem hiding this comment.
An audit failure is reported as an insert failure, so a caller can
double-apply a change that already succeeded.
The tree has already been mutated by the time this runs. With the audit on, a
caller sees Insert return refcount audit for ref 3: stored 2, expected 1
and cannot distinguish it from "your value was rejected". The natural reaction
— log and retry — applies the insert a second time.
Since an audit failure means the library's own invariants are broken, either
panic or return a distinguishable typed error (e.g. *RefcountAuditError) so
callers can tell "the tree is corrupt" from "your input was bad".
🤖 Comment by Claude (Claude Code) on behalf of Will.
| ref, err = d.store.internUncached(mmdbtype.Float32(value)) | ||
| } | ||
| default: | ||
| return nilValueRef, fmt.Errorf("unsupported data type: %v", kind) |
There was a problem hiding this comment.
Decoder errors are returned bare, so a corrupt source database gives no
field path.
Every scalar case in decodeRef assigns to a shared err with a single bare
return, and decodeMap / decodeSlice return iteratorErr, keyErr,
valueErr bare (:195, :200, :206, :244, :251). This line reports an
unsupported kind without the offset. Nothing records which key or index
failed.
Verified: a database whose record is
{"city": {"names": {"en": "Boston"}}, "other": [...]} with the innermost
string's control byte corrupted yields only:
unmarshaling record for network 1.0.0.0/24: unsupported data type: Unknown(73)
What makes this worth fixing rather than a general nit is that the insert
side already does it well — internMap/internSlice wrap with position
(value_store.go:524, :546), producing
interning value for map key "a": interning slice index 0: unsupported MMDB data type mmdbtype.Pointer.
The load path is strictly worse than the insert path for the same class of
failure. Wrapping with kind and offset here, and threading key/index context
through decodeMap/decodeSlice the way the intern side does, would even
them up.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed in 57e3872. Decoder errors now carry the kind and offset, the map key, or the slice index.
🤖 Reply by Claude (Claude Code) on behalf of Greg.
|
|
||
| // The caller-identity cache serves the repeated shallow copies; the other | ||
| // cases disable it to measure the content-dedup and full-intern paths. | ||
| b.Run("equal-shared-nested", func(b *testing.B) { |
There was a problem hiding this comment.
These three b.Run blocks would collapse into a table.
The timed loops at :72-82, :100-110, and :118-128 are byte-identical;
only the values slice and callerIdentityLimit differ. CLAUDE.md prefers
table-driven over repetitive separate cases.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Not taken, per discussion with Greg: only the timed loops match; the setups differ per case (warm-up loop, callerIdentityLimit, value shapes), so a table needs per-case setup closures and adds indirection rather than removing duplication.
🤖 Reply by Claude (Claude Code) on behalf of Greg.
| _, writeErr := tree.WriteTo(&buf) | ||
| require.NoError(t, writeErr) | ||
|
|
||
| f, err := os.CreateTemp(t.TempDir(), "mmdbwriter-load-inserter-*.mmdb") |
There was a problem hiding this comment.
Duplicated fixture boilerplate.
The write tree → CreateTemp → write → close → Load sequence is
copy-pasted four times on this branch (:159-175, :341-357, :1084-1100,
:1164-1180), matching three pre-existing copies. A
writeTempDB(t, tree) string helper removes about 50 lines and stays DAMP.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed in 4422703.
🤖 Reply by Claude (Claude Code) on behalf of Greg.
| for index := 1; index < nodeCount; index++ { | ||
| assert.Equal(t, valueKindInvalid, store.nodes[index].kind) | ||
| } | ||
| assert.Len(t, store.freeRefs, nodeCount-1) |
There was a problem hiding this comment.
Brittle assertion on an implementation detail.
Asserting the exact freeRefs length — and the bucket insertion order at
:304-305 (head == third) — would fail if the chain switched from prepend
to append, with no behaviour change. Both are acknowledged in comments, but
locating the mid-chain node dynamically would make the second robust.
Minor, same file: the for index := 1; index < len(store.nodes) loops at
:124-126, :147-149, and :224-226 duplicate the liveValueNodeCount
helper at :472.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed in 5f74187.
🤖 Reply by Claude (Claude Code) on behalf of Greg.
| - name: Test | ||
| run: go test -race -v ./... | ||
|
|
||
| - name: Test with the reference-count audit |
There was a problem hiding this comment.
Good call on both new legs.
Running the full suite under MMDBWRITER_REFCOUNT_AUDIT=1 is what gives the
audit teeth, and the GOARCH=386 leg plus the deliberate high-bit
valueRef(1<<31) audit cases show the 32-bit work was done properly rather
than just made to compile.
One gap worth knowing about: because finishInsert returns early on error
(tree.go:528), the audit leg never exercises any insert error path.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Thanks. The audit-on-error gap is closed in 0e02882.
🤖 Reply by Claude (Claude Code) on behalf of Greg.
| `Load` completes and can increase peak memory for very large source databases. | ||
| Source networks that reference the same data offset also share a decoded | ||
| value, so custom inserters must copy values before modifying them. | ||
| - Reworked value storage to intern every value node once in a content-addressed |
There was a problem hiding this comment.
Hmm, quite the changelog entry :P
There was a problem hiding this comment.
Split into three bullets in 256779d.
🤖 Reply by Claude (Claude Code) on behalf of Greg.
insert and insertRange lost their deferred releaseResolved when the register-identities commit made the call unconditional. A panic from a caller-supplied inserter then skipped the release, pinning the memo's key and result references for the life of the tree. A pipeline that recovers around Insert to skip a bad record leaked the value subgraph on every panic, and the opt-in audit then failed on a later, unrelated insert. releaseResolved now clears each field as it releases, so a second call is a no-op. insert and insertRange defer it again for panic safety, and finishInsert keeps the explicit call so the audit still runs on a released store.
The unlink loop fell through silently when the chain no longer reached the node, zeroing the slot and queueing it for reuse while a chain still pointed at it. The map-based store this replaced panicked here on purpose: continuing spreads the damage silently as lost deduplication and leaked nodes. Restore the guard. The happy path gains one boolean store per release.
The overflow and underflow panics carried no diagnostic context, so a stack trace from a long build gave no idea which value was involved. They now name the ref and kind with the mmdbwriter: prefix the previous store used, matching the invalid-reference guard. That guard's bounds check also becomes width-safe, so a high-bit ref panics with the diagnostic on 386 instead of an index error. Tests pin all three guards.
The tree is already mutated by the time the audit runs, so a caller seeing a plain error cannot tell a broken tree from a rejected input, and the natural log-and-retry reaction double-applies a change that already succeeded. maybeAuditValueStore now wraps failures in RefcountAuditError so callers can distinguish them with errors.As.
The audit ran only after successful inserts, so the CI audit job never covered the error paths, which is exactly where leaks and over-releases hide. The panicking-inserter leak survived CI for this reason. finishInsert now audits after the release regardless of the insert outcome and joins an audit failure with any insert error. A table of failure shapes pins that the error paths balance.
The audit validated the node table thoroughly and the arenas not at all, and a violated extent invariant is the one ownership mistake that corrupts records silently: two nodes sharing bytes stay readable and keep balanced reference counts. The audit now marks every live payload and child extent, errors on overlap, and walks both free lists so a freed extent that overlaps a live one, or appears twice, is reported. The cost is O(arena) inside a function documented as expensive.
Recycling slots through the freelist is the right production behavior, but it means a stale ref whose slot was reused passes every check and silently returns the wrong value. With the audit on, the store now skips recycling, so any use-after-release hits the invalid-reference panic in the CI audit job. The audit's freelist check expects empty in this mode. Production behavior is unchanged: the new branch is one boolean test per freed slot.
A data record holding the nil ref was already caught, but a non-data record left holding a released ref balanced and passed. The invariant is central to how records own values, and the check is one comparison per record.
memoFirst and memoResult went stale once the map existed, and only a reading convention kept them from serving a stale hit. Clearing them at promotion turns the convention into a fact: a reader that forgets to check the map first now reads nil.
materializePath moves a path record's value ownership into the expanded nodes, but the paths arena kept a live-looking copy. Nothing reads the dead slots today, since path indexes are never reused. Zeroing them makes a future accidental read fail loudly instead of double-counting the moved reference.
rememberCallerIdentity refuses entries without an identity-bearing non-nil value and a non-nil ref, so the defensive validity check and its delete fallback in the lookup could never fire.
replaceDataRecord is the one guarded mutation path for a record's value, and the owned-flag inversion is easy to read backwards. The sibling inline sequence stays inline on purpose: the neighboring split case transfers the old reference rather than releasing it, so a shared helper would cover only part of the pattern.
The per-network wrappers named the network but not the file, so an overlay build loading several source databases could not tell which one was corrupt, and the decoder returned scalar and container errors bare. Load errors now carry the database path, and the decoder wraps failures with the kind and offset, the map key, or the slice index, matching the context the intern path already provides.
rememberOffset shared one silent return between the benign already-recorded case and an offset overflow that maybeWrite, and the writer this replaced, both treat as an error. The record-capacity check in copyNode masks the overflow today, so this settles the inconsistent policies rather than fixing a live bug.
The reference protocol at intern, internNode, and materialize was unwritten, the internOwnedChildren mechanism comment stated the opposite of what internNode does, and dataIdentityKey carried three unstated safety requirements. This also scopes the intrusive-chain GC claim, states the arena extent contract and the clear asymmetry, explains the per-store hash seed and the audit's alias skip, and marks the scalar writer's pointer methods unreachable by construction.
inserter.Func is the doc an inserter author reads, and it still carried the pre-store wording. It now states the view semantics, the Copy instruction, and that only direct inserts and results are validated, so a function can receive an unsupported input and must replace or discard it. The Insert doc scopes the identity cache to direct inserts of identity-bearing values and names the slice-append aliasing case, and InsertFunc and Options.Inserter say that a mid-walk failure leaves the already-visited records updated. Also corrects the Load doc's "instead" and the Tree doc's article.
Splits the long value-store bullet, scopes the caller-value cache to direct inserts of identity-bearing values, documents that Load rejects a source record whose map repeats a key, and notes that a mid-walk inserter failure leaves already-visited records updated.
Every uncovered block in decodeMap and decodeSlice was a release path: the iterator error, the key intern error, and the nested value error. A missing or doubled release there surfaces on a truncated or corrupt source database as a refcount panic instead of an error return. Four hand-built buffers now pin that each path errors and releases every partial child.
Adds tests for the LRU cache's mid-chain unlink, which production hits constantly but the small-limit tests never reached; empty containers, where node kind is the only discriminator under a hash collision, plus their round-trip through serialization and load; per-store hash seeding, which lost its assertion when the old hasher tests were deleted; and arena reuse against a live neighbor, the silent record-rewrite failure the refcount audit cannot see.
The Pointer and Uint128 rejections were tested only at store level, while the doc comments promise behavior of Tree, and no test re-inserted a value obtained from Get, which is the pattern the examples use and the store-owned identity branch exists for. A table covers the rejected shapes and asserts a failed insert leaves no live nodes, an inserter result carrying a Pointer is rejected, and re-inserted Get views pass the audit.
The write-temp-close-load sequence was copy-pasted nine times with small variations. writeTempDB serializes a tree and writeTempFile writes patched bytes; both return the path and rely on the test temp directory for cleanup.
The cascade test pinned the exact freelist length and the unlink test pinned the chain's prepend order, so an internal strategy change with no behavior difference would fail both. The unlink test now locates its mid-chain node dynamically, and three hand-rolled all-invalid loops use the liveValueNodeCount helper.
Summary
once in a content-addressed store keyed by a seeded structural hash, with
exact comparison on collision
references for containers, in shared arenas; release inserted Go value
graphs after interning
record.valueto auint32reference, making sibling-merge a scalarcomparison
(
MMDBWRITER_REFCOUNT_AUDIT) that CI runs across the test suitereference cache
map sorting
GOARCH=386tests in CI and fix the 32-bit arithmetic that previouslydid not compile
Motivation
This is the peak-memory half of the v2 redesign (ENG-5178) and the second
focused part split out of #239. #240 landed collision-safe record keying but
kept every distinct value alive as an
mmdbtypeobject graph, which thedesign estimated at 2.5-4.5 GB of live heap for an Enterprise-scale build.
#239 implemented this store but was closed because it bundled composition,
spilling, and cursor work with an unattributable wall-time regression. This
PR lands the store alone and includes the diagnosis and fix of its own
regression rather than deferring it.
Compatibility
Treeis documented as not safe for concurrent use. In v1, concurrentlookups on an unmodified tree were safe; v2 lookups materialize shared views
lazily.
Getand passed to inserters are shared, read-onlyviews: equal to the inserted values but not necessarily the same Go
objects. Copy before modifying.
mmdbtype.Pointerreturns an error. It previously wrote adangling data-section pointer.
mmdbtype.Uint128returns anerror. The wire encoding holds only the magnitude, so such values
previously produced incorrect data.
Validation
go test ./...,go test -race ./...,MMDBWRITER_REFCOUNT_AUDIT=1 go test ./...,GOARCH=386 go test ./...,golangci-lint run ./...: all passforced hash-collision tests resolve by exact comparison
comments, error handling, type design); the final round found no remaining
code-correctness defects
Performance evidence
Production Enterprise builds, measured as in #240: prebuilt binaries, fixed
build epoch, counterbalanced A B B A A B, three runs per variant, medians.
The run used the content-equivalent ancestor
aa7e7e8of this head; onlyfailure-path, test, and documentation changes followed.
worst run against the baseline's best, given baseline variance that run)
An initial A/B run showed a +9.7% wall regression. It was diagnosed to
per-insert interning and materialization of inserter inputs that never
repeat, as in the Enterprise overlay passes, and fixed in this branch;
inserters now receive the caller's value as passed and only results are
interned.
Locally, write benchmarks improved 67-87% and a Load-then-rewrite of a 402 MB
Enterprise database went from 54.1 s and 9.6 GB peak RSS to 29.3 s and
5.9 GB, byte-identical. Insert-heavy microbenchmarks remain slower than the
previous writer at small heap sizes where its GC costs do not appear; the
production numbers above are the intended measure.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Uint128values.Documentation