Collections: a rotating key set moves its window instead of compacting the array - #3524
Conversation
|
Gate verdict: fails — back for redesign. Paired measurement on a quiet box: 8 interleaved rounds, alternating order, per-round difference statistic, 95% CI must exclude zero. Shared rows only (
The change wins exactly the scenario it targets — a rotating key set (−5.5…−7.3%, plus the script-level keys-after-rotation row) — but the cost lands on every other shape, including the one row that must stay clean: AddOnly, which contains no deletes at all, pays +9.86% at 16 properties. An earlier indicative run showed the same row at +8.68%, so this is stable, not noise. Add-then-remove and re-add-middle pay 4–7%. That trade fails the gate (>1% regression with a CI excluding zero blocks). Converting to draft; the win on rotation is real, so the direction is worth keeping — but the next design must leave the add path untouched (e.g. engage the window only after the first delete, or keep the compaction and bound it, per #3315's framing). |
487553a to
4284e9f
Compare
|
Gate verdict on the redesign ( Full paired run (8 rounds, quiet box, scanner off, baseline = merge-base
The two suspect rows were re-measured as their own pair at 12 rounds — the second-agreeing-pair rule — and both reproduced:
What the redesign promised, it delivered: AddOnly is clean at both widths (the previous design's +9.86% is gone), rotation is genuinely faster, and the [16] remove-newest lane even improved. But Per the standing rule (>1% with a CI excluding zero blocks, confirmed by a second pair), the redesign goes back once more. Staying draft. |
…g the array (sebastienros#3315) sebastienros#3285 made a removed entry a tombstone rather than a free slot the next add pops, because a JS property store enumerates in entry order and reusing a vacated slot puts a key back in a position older than its creation. It handed a slot straight back when the entry removed was the newest -- the high-water mark walks back -- so `o.k = v; delete o.k;` churn never reached a compaction. There was no equivalent for the oldest, which is the shape an object used as a bounded cache has: a fresh name in, the oldest one out. Every step left a hole below the mark, the array filled, and the table compacted every `capacity - live` adds forever even though the live window never grew. Measured on today's main, a 16-key rotation settles on a capacity of 64 and compacts 209 times per 10,000 steps, which is 10,000 / (64 - 16) exactly. The entries become a window [_firstIndex, _lastIndex) over the array rather than a prefix of it, and that window may wrap the end of the array, so a removal at either end retires its slot: the top walks back for the newest, and the base advances for the oldest, past the slot and past any tombstones behind it. The same rotation now reaches no resize at all and settles on 32 -- twice its live count where compaction settled it on four times. A removal from the middle still leaves its tombstone where it is, because reclaiming it would move the keys above it and their position is their creation order. What the window costs the two paths that do not benefit from it is the whole design question, and both answers here were arrived at by reading the JIT's output rather than by argument. The add path. A first attempt made the window's width a third field and the add path's "no room" test, and the paired gate priced that on a workload with no deletes in it at all: AddOnly[16] +9.86%. So the width is not stored. What is stored is _limit, the exclusive bound on _lastIndex: the capacity while the window runs to the end of the array, the base while it wraps below it, and 0 for the shared dummy array, which nothing may write to. That is one field to read and one comparison to make where the mark read an array length and made two, so AddKey emits mov ecx, [rbx+0x20] / cmp ecx, [rbx+0x24] / jne against main's mov ecx, [rbp+0x08] / cmp [rbx+0x1C], ecx / je / cmp ecx, 1 / jne with the rest of the method identical instruction for instruction and five bytes shorter overall. Running out of room calls MakeRoom, which tail-jumps to the code Resize has always been whenever the window is based at slot 0, so a table that never deletes reaches exactly today's resize through one extra compare. Storing three ints instead of four would not have been cheaper: 16 bytes of references plus 12 of ints pads to the same 32, so the object is 48 bytes either way, and the fourth int is what buys the shorter test. The removal path. The gate's second run then found AddThenRemoveNewest[64] +1.15..+1.64% -- on a path whose instructions had not changed. They had not, but their layout had: spelled as an if/else, the newest-entry branch compiled to a taken `je` with its single store moved out of line, where main falls through with no taken branch at all, and Remove grew from 282 bytes to 306. The data side was ruled out by measurement rather than by argument: the object does grow by 8 bytes, but the entry array's cache-line alignment is uniformly distributed over consecutive invocations on both sides (the hot entry straddles a line in 35 of 96 rounds here against 32-36 of 96 on main), because a gen0 collection resets the bump pointer long before an 8-byte footprint difference can bias it. So the test is inverted and everything that is not the newest entry is behind one tail call, which is the only one of four formulations tried that RyuJIT lays out as a comparison the path falls straight through into its store and its return -- main's shape, with no taken branch on it. What that moves is the cost of a removal from the middle, which now reaches a small tail-called RetireSlot rather than an inline compare; the tombstone walk is split off into RetireBase so that half stays small. RotateBehindPinnedKey is the row that pays it. Where the window cannot help this degrades to exactly today's behaviour: when the oldest live entry is never removed the base cannot advance, every removal is from the middle, and the table compacts on the same schedule through the same code. The buckets are unaffected by the moving base, since a bucket and a chain link are physical slot indexes that encode no position in the window. Compacting in place keeps the base where it is and moves the survivors down towards it, so the write cursor can never overtake the read cursor, which is what makes a wrapped window safe to squeeze without a second array; growing is the chance to unwrap, so the survivors land at the bottom of the doubled array and the window starts over at slot 0. DictionarySlim, the symbol store, gets the identical treatment: it is a deliberate near-duplicate of StringDictionarySlim, changed together by sebastienros#3285, and letting one keep a high-water mark while the other has a window is a maintenance trap worth more than the change costs. Both types are internal and nothing observable outside the assembly depends on which slot a key occupies -- except enumeration order, which is what the tests are for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S
4284e9f to
7d5140b
Compare
|
Gate verdict on v3 ( Full pair, 8 rounds (quiet box, scanner off, baseline
Confirmation pair on the flagged families plus RotateOldest, 12 rounds:
Why the script-row flags do not block. No two runs agree. The add path is untouched (as designed and as measured twice), the confirmed v2 regressions are gone, and the rotation wins are real. Marking ready and merging. |
|
Closing datum — the unpaired degradation control, run candidate-only after the merge (default BDN job, quiet box), with
The pinned-key rotation — the shape the window cannot help, and the one that pays the new |
1 participant
Fixes #3315.
#3285 made a removed entry a tombstone rather than a free slot the next add pops, because a JS
property store enumerates in entry order and reusing a vacated slot puts a key back in a position
older than its creation. It handed a slot straight back when the entry removed was the newest —
the high-water mark walks back — so
o.k = v; delete o.kchurn never reached a compaction.There was no equivalent for the oldest, which is the shape of an object used as a bounded cache:
Every step left a hole below the mark, the array filled, and the table compacted every
capacity − liveadds forever even though the live window never grew. On today'smain, over10,000 rotations: 16 live keys settle on capacity 64 and reclaim 209 times (10,000 / 48); 64 live
keys settle on 256 and reclaim 53 times (10,000 / 192).
Why this is a rewrite, not a fixup
The first version of this change made the window's width a third field and used it as the add
path's "no room" test. The paired gate priced that on a workload with no deletes in it at all:
The rotation win was real, and every other row paid for it. The binding constraint is that the add
path must not change, so this version does not store the width at all.
A second gate then cleared
AddOnlyat both widths and kept the rotation win, but flaggedAddThenRemoveNewest[64](+1.15…+1.64%, two independent pairs) — on a path whose instructions hadnot changed. They had not; their layout had, and that is what the
Removesection below is about.The data-side explanation was ruled out by measurement, not by argument: the object does grow by 8
bytes, but the entry array's cache-line alignment is uniformly distributed over consecutive
invocations on both sides — the hot entry straddles a line in 35 of 96 rounds here against 32–36 of 96
on
main— because a gen0 collection resets the bump pointer long before an 8-byte footprintdifference can bias it. Storing three ints instead of four would not have helped either: 16 bytes of
references plus 12 of ints pads to the same 32, so the object is 48 bytes either way.
What is stored instead
_limit: the exclusive bound on_lastIndex. It isThat single field is what tells a full window from an empty one — both leave
_firstIndexequal to_lastIndex— and it is what the add path already had to test against something.The add path, at the instruction level
AddKeyreads one field and makes one comparison where the mark read an array length and made two.The JIT's own output for
StringDictionarySlim<__Canon>:AddKey(DOTNET_JitDisasm, FullOpts):Two instructions fewer, one branch fewer. Everything after that —
_lastIndex++, the two entrywrites, the bucket update,
_count++— is identical instruction for instruction, and the wholemethod is five bytes shorter (151 against 156).
Running out of room calls
MakeRoom, 47 bytes, whose first branch tail-jumps to the codeResizehas always been whenever the window is based at slot 0. So a table that has never deletedanything reaches exactly today's resize — same growth test, same
Array.Copy, same descending bucketrebuild — through one extra compare and a tail jump. That is the entire per-resize cost on
AddOnly,against two instructions saved on every add.
The wrap itself, which is what makes a rotating key set free, is the other branch of
MakeRoom:No data movement, no allocation, no bucket rebuild. The same 10,000 rotations reach 0 resizes,
at capacity 32 for 16 live keys and 128 for 64 — half what the compaction threshold settled on.
What
RemovecostsIts newest-entry branch is the one both delete-churn shapes run, so it is written to compile to main's
exact six instructions with no taken branch on it: the test is inverted and everything that is not
that case sits behind one tail call.
That shape is not what the obvious spelling produces. Four formulations were written and disassembled:
an
if/else, anelse ifchain, and both of those with the base test at the call site all make RyuJITmove the single store out of line and put a taken branch on the commonest delete there is. Only
"invert the test, hand the rest over with a tail call" lays it out straight-line — and that is what the
first gate caught (see below).
What this moves is the cost of a removal from the middle: it now reaches a small tail-called
RetireSlot(six instructions: compare the base, return) instead of one inline compare. The tombstonewalk is split off into
RetireBaseso that half stays small.RotateBehindPinnedKeyis the row thatpays it, and it is branch-only.
Resetting an emptied table lives in the base branch, where the walk over tombstones needs the count
anyway; a table emptied through the newest branch simply closes its window where it stands, which is
an empty window like any other, and
ClearPreservingCapacityputs it back at the base of the array itkept.
The invariants, and where this degrades
_entries[_firstIndex]is live whenever_countis not 0. This is why the base advance skipsthe tombstones behind the slot it retires: without it the base would come to rest on a tombstone,
the next removal of the oldest would no longer recognize itself, and one delete from the middle
would disarm the fast path for good.
that has reached its bound is always a full one and never a closed one — which is what lets the
in-place squeeze assume there is something to squeeze.
When the oldest live entry is not removed, the base cannot advance and this degrades to exactly
today's behaviour — every removal is from the middle, the tombstones pile up, the table compacts on
the same schedule, and it runs the same
Resize. That is an object with one field set once and arotating key set above it; the benchmark carries it as
RotateBehindPinnedKey.Growth, compaction,
Clear, and the bucketschain link is a physical slot index; neither encodes a position in the window.
write cursor can never overtake the read cursor. Rebasing to 0 in place would not be safe: the
survivors above the wrap point would overwrite unread slots below it.
window order — two
Array.Copyruns when there is nothing to compact, tail of the old array thenhead — and the window starts over at slot 0.
ClearPreservingCapacityclears the window's slots, wrapping if it has to, and always puts thewindow back at slot 0; its early-out is now the dummy array itself.
Both stores, not only the string one
DictionarySlim(the symbol store) gets the identical treatment. It is a deliberate near-duplicateof
StringDictionarySlim— same tombstone design, same doc comment, changed together by #3285 — andletting one keep a high-water mark while the other has a window is a maintenance trap worth more than
the change costs.
No public API change: both types are
internal, and nothing observable outside the assembly dependson which slot a key occupies. The one thing that is observable — enumeration order — is what the
tests are about. No migration-guide entry, for the same reason #3285 has none: nothing an
embedder can see changes.
Tests
Jint.Tests/Runtime/OwnPropertyCreationOrderTests.csis the file #3273/#3285 left behind; thewrap-around cases are appended to it, and they fail on unmodified
main(four of them by assertingthe settled capacity or by naming a field that does not exist there).
The new one this rewrite adds is
TheWindowBoundStaysTheOneAnAddMayTest, which checks the windowarithmetic after every single operation of a 10,000-step mix of oldest / newest / middle
removals, drains and pooled resets — the bound is the capacity or the base and never below the top,
the base is never a tombstone, the width never below the live count, an emptied table closes its
window. Since the add path is now one comparison against that bound, it is where a mistake would be.
The tests catch a broken window. Three mutations of the merged code:
TheWindowBoundStaysTheOneAnAddMayTest(both arms) — and nothing else, which is why it exists_limit = lengthalways)Array.Copyruns the other way roundAWrappedWindowWithNothingToCompactIsUnwrappedByGrowingSuites
Everything
-c Release, freshly built, on this branch.Jint.TestsJint.Tests.PublicInterfaceJint.Tests.CommonScriptsJint.Tests.SourceGenerators(failed / passed / skipped;
dotnet build -c Releaseon the solution is clean,TreatWarningsAsErrorsand all. The order tests were also run under
-c Debug, where theDebug.Asserts inResizeandResizeWindoware live.)The test262 figure is unchanged from the first revision of this branch, which measured the same
102,537 / 0 / 151 on
mainat its branch point.Benchmark, and what to expect from the gate
PropertyDeleteChurnBenchmarkkeeps theRotateBehindPinnedKeyrow from the first version — therotation with a pinned key underneath it that the window cannot help — so the shape that still
compacts is priced rather than assumed.
No numbers here; the gate table follows in a comment. Predicted direction per row:
AddOnly[16],AddOnly[64]AddThenRemoveNewest[16/64]ReAddMiddle[16/64]RotateOldest[16/64]capacity − liveadds; the first version got −5.5 / −7.3% while also paying for a heavier add pathRotateBehindPinnedKeyRetireSlotScriptKeysAfterRotation[16]ScriptKeysAfterRotation[64],ScriptRotateOldest[16/64],ScriptDeleteReAdd[16/64]Allocated bytes should fall on the rotation rows (the entry array settles at half the capacity) and
rise by 8 bytes per dictionary object, which now carries four
intfields instead of two.