Repository navigation
MemoryKV is the address of a memory header: no pointer or count ceiling - #166
Merged
Merged
Conversation
The handle is 0 for the empty store, otherwise the address of a 16-word header the first insert allocates and zeroes at the free memory pointer: 15 full-width list heads, then a meta word holding the word count above a 15-bit occupancy mask. Nothing is packed into the handle, so there is no pointer ceiling and no count ceiling, and both errors are gone. Export walks the occupancy mask lowest bit first and maps each bit to its list through SLOT_TABLE, which gives the same pair order as the bisect it replaces. Copies of a non-empty handle are one store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (31)
💤 Files with no reviewable changes (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughChangesMemoryKV header migration
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
test/lib/LibMemoryKVPacked.sol is main's LibMemoryKV verbatim, renamed, reusing the key and value types from src. The parity tests check the new layout against it: pooled and raw differential fuzz over export and get/has, every one of the 32767 occupancy masks with its exact meta word and export order, SLOT_TABLE rebuilt from its definition, and the first insert's header zeroed over dirty memory with exactly 0x260 allocated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test/lib/LibMemoryKVLayout.sol holds the functions that read and build a store's header and nodes directly: metaOf, lengthOf, maskOf, headOf, headAddressOf, occupiedSlots, occupancyBitOf, craftHeader, writeList, craftNode and handleWith. Every layout value comes from LibMemoryKV's constants. The readers answer 0 for the empty store, which has no header. Its tests sit beside it in LibMemoryKVLayout.t.sol. The packed-layout constants, withCount, and the head-slot bound on craftNode are gone from LibMemoryKVTestHelpers.sol, along with the test-side LIST_COUNT and NODE_BYTES, which src now names. LibMemoryKVSlow's linear export reads the heads and the meta word. SetAtFreePointer keeps only the in-frame form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test-lib-support splits the test helpers into files named for what they hold. The header-layout readers and crafting functions move into LibMemoryKVHandle.sol, replacing its packed-handle readers and withCount, and their tests into LibMemoryKVHandle.t.sol. Keys, free memory, assert and export helpers come from their new files. src keeps the header layout. It takes the ^0.8.25 pragma and the shorter has NatSpec from the base; the base's packed constants do not apply to the header layout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
errorAbi and wordCountOverflow pinned the two errors the header layout removes, and setSlotMask pinned the packed head slot's clear mask; none of the three has a subject left. types loses the two empty-store tests that shifted and masked the zero word (#137). saturate, types and exportAlloc read the word count, heads and occupancy mask through the header readers, and name list counts through LibMemoryKV.LIST_COUNT. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
getset: the first insert allocates HEADER_BYTES + NODE_BYTES, later ones NODE_BYTES; the 0xFFFF boundary and overflow tests go with the bound. setInsert: header at the free pointer, node after it, header zeroed over dirty memory word by word, one occupancy bit per list, whole addresses above 0xFFFF; a dropped later return is harmless, a dropped first return loses the store. setUpdate: an update leaves every header word as it was; a repeated upsert allocates one header and one node. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bisect.t.sol becomes exportOccupancy.t.sol: every list alone, adjacent and alternate lists, all 32767 masks from the start pointer and from above 0xFFFF, and count bits never read as mask bits (no write past the end of the export while the count runs through bits 1..7). setCapacity is the #13 acceptance test: 2000 pairs from above 0xFFFF, every key read back and exported once; and a store straddling 0x10000 at every alignment reads back whole. exportWalk: a next word at exactly 0x10000 and a head and next pointer above it, pairs checked by countPair (#106). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e store getMatch and getWalk craft their lists with craftNode and handleWith and read counts and heads from the header. frameLocal and crossFunction state that the word crossing an external call is the header address alone, and that every live copy of a non-empty handle is the one store (#107, #134). handleAlias pins updates and inserts reaching older copies, and the empty handle as the one copy that branches. setCapacity hashes its values in scratch so nothing allocates between the inserts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gasClaims pins the export as the same gas for every list, a list left empty as costing the export nothing (spread over k lists costs exactly k - 1 list visits more than stacked in one), and the walk beating the linear loop for stores of up to five pairs. The README figures are re-measured with two-sided bounds: get ~250, insert into an empty list ~345, first insert into an empty store ~430 (it also allocates and zeroes the header), ~65 per key walked, fourth key into one list ~540. The shallow-slot and bisect-saving claims are gone with the bisect. toBytes32ArrayLinear copies each list with the same inline loop as the mask walk instead of a Yul function call per head, so the comparison measures the mask and nothing else. The README describes the header layout in place of the packed word. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The table is a literal because inline assembly reads it, and its trailing zero bytes are the indices no occupancy bit reduces to. testSlotTableIsItsDefinition rebuilds it from its definition, which is the miscounted-digit risk the detector flags. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
crossFunction's header no longer says its claims are independent of what set computes. gasClaims says padMemory makes the measurement order irrelevant, which it does, rather than calling the order pessimistic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
errorAbi, setSlotMask and wordCountOverflow stay deleted. The handle helpers keep the header readers and crafting; COUNT_MAX names the widest count the meta word holds. getset takes main's names and messages with the header allocation; the 16 bit bound tests stay dropped. setUpdate takes main's collider names and testSetRequiresEveryBitOfTheKey, which puts the crafted list back before each insert because an insert writes the header in place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A consumer compiles the library with its own optimizer settings, so an absolute gas figure only holds for this repository's build. The README's get, insert, first-insert and per-key figures are gone, and so are the README_* constants, ROUNDING_PERCENT and assertNear that pinned them. - testTheFirstInsertCostsMoreThanALaterInsertIntoAnEmptyList: the first insert into an empty store costs more than the same insert into an empty list of a store that already has its header. - testEachKeyAheadInAListAddsGas: each key already in a list makes an insert into it, and a get of the key furthest from its head, cost more than the one before. - README states gas relatively; the getsetgas header no longer says gasClaims measures README figures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Takes #164's forge-std 1.16.2 bump. bisect, errorAbi, setSlotMask and wordCountOverflow stay deleted: main changed only their forge-std import. exportOccupancy and packedParity, which this branch adds, import forge-std-1.16.2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Takes #174. crossFunction.t.sol keeps this branch's testEveryLiveCopyIsTheOneStore: in the header layout every copy of a non-empty handle is one word, and the test already asserts that an update and an insert both return it. #174's NatSpec and per-handle count describe the packed handle, which this branch removes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Names the last list through LibMemoryKV.LIST_COUNT, builds the alternating masks from occupancyBitOf, reads the export's address through LibBytes32Array.startPointer, and states present behaviour in the NatSpec the merge carried. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er than sixteen bits exportAlloc: the export allocates at the free memory pointer 1 to 31 bytes past a word and leaves the live bytes below it intact, including after an abi.encodePacked allocation. setCapacity: with a crafted word count from 0x10000 up to the widest the field holds, an insert adds exactly one pair with the occupancy mask intact, an update leaves the meta word alone, and the export sizes its array from the whole count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #13
Implements the plan in #13 (comment).
#123 is closed by #192, which named the packed layout's literals. This layout names every offset, width and mask as an
internal constantthe same way.Layout
MemoryKVstaysuint256andMEMORY_KV_EMPTYstays0. No function signature changes.0is the empty store, which owns no memory. Any other value is the address of a 16-word header. The first insert allocates it at the free memory pointer and zeroes it withcalldatacopy.ssets bit14 - s.MemoryKVOverflowandMemoryKVLengthOverfloware removed.getreturns(0, 0)forkv == 0without reading memory.toBytes32Arraywalks the mask lowest bit first and maps each bit to its list throughSLOT_TABLE. That is lists 14 down to 0, each newest first, which is the order the bisect produced. The order stays unspecified. The bisect and its slithercyclomatic-complexitydisable are gone.setmust still be assigned, because the first insert's return is the only thing holding the header.MemoryKVNatSpec, and every offset, width and mask is a namedinternal constant.PAIR_COUNT_INCREMENT = 0x20000is a literal. Written asshl(COUNT_BIT_OFFSET, 2), it was not folded and every insert paid for the shift.Tests
bisect.t.sol,errorAbi.t.sol,setSlotMask.t.sol,wordCountOverflow.t.sol.test/lib/LibMemoryKVCapacity.sol(EMPTY_FRAME_PAIRS, the packed 682-pair ceiling) is deleted with them.packedParity.t.sol: differential againsttest/lib/LibMemoryKVPacked.sol, kept as the oracle for export order andget/has. The oracle is main's packedLibMemoryKVat f50c40d, not current main: that version's code with the library and handle type renamed (LibMemoryKVPacked,PackedMemoryKV), the key and value types imported from src, and its own pragma and NatSpec. Main now names the same literal values as constants, so the oracle behaves as main does. The tests pin the exact meta word and export order for every occupancy mask from 0 toOCCUPANCY_MASK;SLOT_TABLErebuilt from its definition; the header zeroed over dirty memory.exportOccupancy.t.sol: every list alone, both ends of the mask, adjacent and alternate lists, every occupancy combination, below and above0x10000, and count bits never read as mask bits.test/lib/LibMemoryKVHandle.solreads and crafts the header layout through src's constants. The rest of the tree moves to the new layout.types.t.solpins how the header constants tile the header and that each head points at its list's node.storageParity.t.solchecks every fuzzed pair, with no per-store cap.setCapacity.t.sol:testTwoThousandPairsFromAboveSixteenBitsputs the free pointer at0x10000and inserts 2000 distinct keys; every key reads back, and the count, the export and the allocation are exact.testAStoreStraddlingSixteenBitsReadsBackWholefuzzes the start so the header and nodes straddle0x10000.gasClaims.t.solcompares paths within one build and pins no gas figure, and the README states gas the same way. One pair exports at the same cost from every list. Each list a store leaves empty saves exactly one list visit. The walk costs less thanLibMemoryKVSlow.toBytes32ArrayLinearat every occupancy from 0 toLIST_COUNTlists, and from one occupied list on the saving never grows as lists fill. The first insert into an empty store costs more than an insert into an empty list of a store that has its header. Each key ahead in a list makes an insert into it, and a get of the key furthest from its head, cost more.QA
b0a6a20:rainix-solstatic, legal and test pass; 27 suites, 160 tests passed, 0 failed (https://github.com/rainlanguage/rain.lib.memkv/actions/runs/35382810381).3f21e4a, through three lenses: semantics, src and tests (adversarial-semantics.md,adversarial-src.md,adversarial-tests.mdin/home/gildlab/artifacts/memkv/issue13/). It found no src defect. Its two findings are fixed here: no test pinned the export at an unaligned free memory pointer, and this body called the packed oracle current main verbatim.test-equivalence.md, same directory): of the 78 main test names this branch does not have, 49 are covered by a test here and 26 pin behaviour this layout removes. The other 3, main'swordCountOverflowinserts up to a full count and update at a full count, were a gap, because no test here drovesetor the export above 4000 words. Fixed by the word-count tests below.exportAlloc.t.sol):testExportAllocatesAtAnUnalignedFreePointerputs the free memory pointer 1 to 31 bytes past a word, above live bytes. The array is at the pointer and the live bytes are unchanged.testExportAfterEncodePackedLeavesTheBytesAloneruns the export afterabi.encodePacked, and the packed bytes and their length are unchanged. Mutant E04 (array := and(mload(0x40), not(0x1f))), run against the whole suite atb0a6a20: killed, by these two tests and no other (baseline 160 passed).setCapacity.t.sol):writeListcrafts a header with a count from0x10000up to the widest the field holds. An insert adds exactly 2 and keeps the mask (testInsertAddsOnePairToAWordCountWiderThanSixteenBits). An update leaves the meta word unchanged (testUpdateLeavesAWordCountWiderThanSixteenBitsAlone). The export takes its length and its allocation from the whole count (testExportSizesItsArrayFromAWordCountWiderThanSixteenBits). Two mutants narrow the count to 12 bits, each run against the whole suite atb0a6a20. W01, inset's insert, is killed by the insert test alone. W02, in the export's sizing, is killed by the export test alone. The update test is killed, by hand, by a mutant that sends everysetdown the insert path (switch iszero(pointer)toswitch 1).src/lib/LibMemoryKV.sol(constants,get,set, the export walk,SLOT_TABLE,has), whole suite, atd8b5358(the headb0a6a20only adds tests on top of it; src is unchanged): 63/63 killed, 0 no-run, 0 harness errors. Run with--gas-limit 10000000000, so a mutant that links a list into a cycle (S13: the node allocation leaves the free pointer where it was) fails out of gas instead of timing out as a no-run. C04 (NODE_BYTES0x60 -> 0x80) is killed bytestSetInsertIntoTheEmptyStoreAllocatesAHeaderThenANode, which checks that the first insert's allocation ends after the node's next word.d8b5358(the headb0a6a20only adds tests on top of it; src is unchanged): 43/43 killed. B01–B31 cover the readers and crafting functions intest/lib/LibMemoryKVHandle.sol, includingwriteList's bounds and kept bits;test/lib/LibMemoryKVHandle.t.solalone kills all 31. L02–L09 (LibMemoryKVSlow.toBytes32ArrayLinear) are killed bytestRoundTripLinearinarray.t.sol, and L01 (the empty-store guard) bytestSlowLinearExportAllocatesWhatItWritesinLibMemoryKVSlow.t.sol. F01–F03 (setAtFreePointerInFrame) are killed bytestSetInsertRecordsTheWholeAddressinsetInsert.t.sol.LibMemoryKVSlow.toBytes32ArrayLinear: the walk costs less at every measured size, 0 to 15 occupied lists and 0 to 20 sequential keys. The saving is largest with one list occupied and falls with each list after it. It is smaller for the empty store than for one occupied list, because both exports skip everything there.setCapacitytests named under Tests put a store above and across0x10000. No test pins an absolute gas figure: noassertApproxEq*or bound against a gas constant, andgit grep '~[0-9]'oversrcandREADME.mdfinds nothing.🤖 Generated with Claude Code