fix(runtime): stop the concat memo probing when it isn't paying (from #9396) - #9397
Merged
Conversation
added 2 commits
September 1, 2026 16:15
bisected to `d923b8dcf0` and I confirmed by in-binary A/B. It is my change and
this is the fix.
The fixture builds `{ x: i, y: i * 2, name: "item_" + i }` half a million times
with EVERY result distinct — the exact shape the memo targets and the exact
shape it cannot help. Instrumented: 501,000 probes for 6 hits.
Two separate costs, and the obvious one was the smaller:
1. **Rooting.** Every miss inserted, so the memo held up to 512 strings alive
as strong GC roots on the one benchmark whose whole subject is collecting —
keeping garbage alive to be promoted, and adding a root scan per collection.
Fixed with an admission doorkeeper: a one-byte hash tag per slot means a
result must be seen TWICE before it earns an entry, so a never-repeated key
costs a byte instead of a rooted string. The tags are plain bytes, never
addresses, so the collector never sees them.
The first tag derivation was itself wrong — sliced straight out of FNV-1a's
high bits, which avalanche poorly, it admitted 256,516 of 501,000 where
~1/128 was intended. A splitmix64 finalizer before slicing brought that to
3,992.
2. **The probe itself, which was the bigger half.** Even with admissions down
to 0.8%, the row still ran 21 ms against 12. Assembling the result into a
buffer and hashing it, on every concat, for six hits, was the cost. So the
memo now measures its own hit rate over a 4096-candidate window and stops
probing when under a quarter of candidates hit, with exponential backoff to
one probing window in 2^8. A backoff always expires into a probation window,
so a program that starts cold and turns hot is still picked up.
The governor word is a relaxed global rather than a `thread_local!` — it is
advisory, correctness never depends on it, and three TLS reads per candidate
were themselves worth ~2 ms here.
Measured, min-of-15, in-binary A/B against the memo compiled out:
| | memo on | memo off | before this fix |
|---|---|---|---|
| `bench_gc_pressure` | 13 ms | 12 ms | 21 ms |
| `bench_object_property` | 15 ms | 21 ms | 15 ms |
So the hostile row is within 1 ms of not having the memo at all, and the row
the memo exists for keeps its full 6 ms win. `bench_string_heavy` unchanged
(43/44). Output identical throughout.
Tests assert the governor's DECISIONS, not wall-clock: it disables after a
window with no hits, stays enabled when every candidate hits, and always
recovers from backoff. A timing test here would be noise-sensitive and would
not say why it failed.
WHY MY PRE-MERGE ADVERSARIAL TEST MISSED THIS: #9373 did test an
all-distinct-results fixture and measured 7 ms vs Node's 8. But that fixture
had no other allocation pressure, so it exercised the probe cost in isolation
and never the interaction that actually hurt — rooting garbage while a
collector is under load. An adversarial case has to be adversarial in the
dimension the change touches, and mine covered one of two.
Gates: perry-runtime 2918 passed (single-threaded), perry-codegen 1379 passed,
fmt clean, `-D warnings` clean, shape-descriptor census clean, all five static
gc_root_dominance_check audits pass, GC store-site inventory passes, all three
touched files under the 2000-line cap.
Claude-Session: https://claude.ai/code/session_01Pcq6j6y57TdKSR2Zx2D187
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesThe short-string concatenation memo now uses adaptive probing, hit-rate backoff, and two-hit admission. Pairwise and string-plus-number paths apply these controls. Tests cover disablement, sustained hits, recovery, and delayed memoization. Concatenation memo admission
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConcatPath
participant MemoGovernor
participant AdmissionTable
participant CONCAT_MEMO
ConcatPath->>MemoGovernor: check whether probing is enabled
ConcatPath->>CONCAT_MEMO: probe using the derived slot
CONCAT_MEMO-->>ConcatPath: return hit or miss
ConcatPath->>MemoGovernor: record hit when a memo entry matches
ConcatPath->>AdmissionTable: observe the derived admission tag
AdmissionTable-->>ConcatPath: allow publication after the second observation
ConcatPath->>CONCAT_MEMO: publish admitted result
Suggested reviewers: ✨ Finishing Touches📝 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 |
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
1 participant
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.
Lands #9396, whose branch conflicts against current
main. Author's commit preserved.The conflict is with my own change. #9396 is based on #9389, before #9390 converted
CONCAT_MEMOtocrate::perry_thread_local!. Resolved by taking #9396's governor and doorkeeper wholesale and keeping theperry_thread_local!macro. Its new doorkeeper tableCONCAT_MEMO_TAGSarrived on the raw macro for the same reason, so that is converted too.Two root-holder verdicts added.
CONCAT_MEMO_TAGSis a[u8; 512]of hash tags — plain bytes, never addresses, so the collector never sees a pointer there; the admitted strings still live inCONCAT_MEMO, whichscan_concat_memo_roots_mutvisits.Chosen over #9395's version of the same fix, which is stacked on #9360 and would have dragged in the typed-array aliasing regression that is still open there. This one is standalone and, I think, the better design: a doorkeeper that admits only on a second sighting attacks the rooting cost directly, rather than switching the memo off wholesale.
Worth noting the PR's own account of getting the tag derivation wrong first — sliced from FNV-1a's high bits it admitted 256,516 of 501,000 where ~1/128 was intended, and a splitmix64 finalizer brought it to 3,992. That is the kind of thing that would have looked like a working fix on a benchmark that only measures the end number.
Validation:
perry-runtime8 suites green underRUST_TEST_THREADS=1; release build clean with no warnings; file-size, thread-local, root-holder, GC-knob, store-site, raw-handle and fmt gates all pass.Summary by CodeRabbit