perf(runtime): monotone latches make unused-feature type probes free - #7755
Conversation
📝 WalkthroughWalkthroughThe runtime adds a monotone ChangesRegistry probe latches
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime
participant RegistryLatch
participant Registry
participant Probe
Runtime->>RegistryLatch: arm before registration
Runtime->>Registry: publish address or metadata
Probe->>RegistryLatch: check latch state
alt latch is idle
Probe-->>Runtime: return miss
else latch is armed
Probe->>Registry: perform active lookup
Registry-->>Probe: return registration result
end
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/registry_latch_probes.rs (1)
60-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMake the idle-path coverage deterministic.
These latches are process-global and never reset. Another test can arm a latch before these tests run. In that case, each scratch-address assertion still passes through the slow path. The tests then do not prove that the idle path ran.
Run each idle-path case in a fresh process, or add isolated test state with a global test lock. Assert that the relevant latch is idle before the first probe.
🤖 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 `@crates/perry-runtime/src/registry_latch_probes.rs` around lines 60 - 216, Make the idle-path tests deterministic by isolating each case from process-global latch state: run them in fresh processes or protect/reset them with a global test lock and isolated state. Before each test’s initial scratch-address probe, assert the relevant latch is still idle, using the corresponding typed-array, buffer, symbol, map, or set latch accessors. Preserve the existing allocation and recognition assertions after proving the fast path was initially unarmed.
🤖 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.
Nitpick comments:
In `@crates/perry-runtime/src/registry_latch_probes.rs`:
- Around line 60-216: Make the idle-path tests deterministic by isolating each
case from process-global latch state: run them in fresh processes or
protect/reset them with a global test lock and isolated state. Before each
test’s initial scratch-address probe, assert the relevant latch is still idle,
using the corresponding typed-array, buffer, symbol, map, or set latch
accessors. Preserve the existing allocation and recognition assertions after
proving the fast path was initially unarmed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b923ce94-6171-4003-a93a-83ffe7d68eda
📒 Files selected for processing (12)
changelog.d/7755-registry-probe-latches.mdcrates/perry-runtime/src/buffer/detach.rscrates/perry-runtime/src/buffer/header.rscrates/perry-runtime/src/buffer/mod.rscrates/perry-runtime/src/lib.rscrates/perry-runtime/src/regex.rscrates/perry-runtime/src/registry_latch.rscrates/perry-runtime/src/registry_latch_probes.rscrates/perry-runtime/src/shared_sab.rscrates/perry-runtime/src/symbol.rscrates/perry-runtime/src/typedarray/mod.rscrates/perry-runtime/src/typedarray_props.rs
Two unrelated realistic programs spent 13% of runtime asking side tables whether ordinary values were typed arrays, Buffers, SharedArrayBuffer backings or Symbols — features neither program used. Each probe cost at minimum a `_tlv_get_addr` (Darwin has no local-exec TLS), a `RefCell` borrow and a hash; `is_registered_symbol` and `is_uint8array_buffer` took a process-global mutex on every miss. Generalise the #7474 Map/Set trick into `registry_latch::RegistryLatch`: a monotone process-global flag armed by the registration site and checked first by the probe, so a program that never uses the feature answers from one atomic load. The latch has no `disarm` by construction, so the only observation it can get wrong is "idle while non-empty" — which the arm-before-publish ordering rule rules out. Latched: typedarray kind lookup, buffer registry (incl. the SAB fallback), Uint8Array-from-ctor, ArrayBuffer, SharedArrayBuffer, DataView, detached buffers, secret/crypto/asymmetric key metadata, the ArrayBuffer alias map, Symbol pointers, and the RegExp pointer table.
…appears The latch removed the thread-local resolutions (visible as a 19.9% -> 14.6% drop in _tlv_get_addr on asyncpipe and 10.8% -> 5.5% on interp), but the probes' own self-time did not move: with the latch check behind a non-inlinable cross-module call, what was left WAS the call. Split each latched probe into an inlinable idle check plus an #[inline(never)] slow path, so an unused feature costs one atomic load at the call site.
0ceda2a to
75e0d74
Compare
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 `@Cargo.toml`:
- Line 318: Revert the release metadata changes in Cargo.toml at lines 318-318
and CLAUDE.md at lines 11-11: restore the previous [workspace.package].version
and Current Version values in both files, leaving release version updates to
maintainers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4da544f5-c2ff-4fdd-8d70-7759b28ab0c3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
CLAUDE.mdCargo.toml
|
|
||
| [workspace.package] | ||
| version = "0.5.1442" | ||
| version = "0.5.1443" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Revert contributor-owned release metadata changes.
This feature PR changes release metadata in both files. Revert both changes and let maintainers update version metadata during release.
Cargo.toml#L318-L318: restore the previous[workspace.package].version.CLAUDE.md#L11-L11: restore the previousCurrent Versionvalue.
📍 Affects 2 files
Cargo.toml#L318-L318(this comment)CLAUDE.md#L11-L11
🤖 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 `@Cargo.toml` at line 318, Revert the release metadata changes in Cargo.toml at
lines 318-318 and CLAUDE.md at lines 11-11: restore the previous
[workspace.package].version and Current Version values in both files, leaving
release version updates to maintainers.
Sources: Coding guidelines, Learnings
Merging as v0.5.1443The ordering proof is the part that makes this landableA monotone latch is only safe if every registration site arms before it publishes. One missed The two-sided model. The per-feature tests take the idle fast path first, then register, then require the probe to find it — which is the only ordering in which a missing The "unregistered address misses every probe" sweep against a heap-plausible pointer with readable bytes in front of it is a good touch — the probes that read a The design argument holdsMonotone with no Shipping The The second commit exists because the first measured honestlyAfter the latches, Two of these took a process-global mutex on every miss, in every process, for every non-Symbol / non-Uint8Array value ( Finding the sites by grepping the pattern rather than following the profile is also right — several latched probes appear in neither profile, and those are exactly the ones that would have been left behind. Gates 21/21. |
1 participant
The finding
Symbolicated profiles of two unrelated realistic programs show 13% of runtime spent probing side-table registries for features the program never uses.
asyncpipe.ts(an async service pipeline — Map/Set, template literals,Promise.all, closures in loops):set::is_registered_settypedarray::lookup_typed_array_kindbuffer::header::is_registered_buffermap::is_registered_mapshared_sab::is_shared_sabsymbol::is_registered_symboltypedarray_props::typed_array_add*buffer::header::is_uint8array_buf*That program allocates no typed array, no Buffer, no SharedArrayBuffer, no Symbol.
interp.ts(a tree-walking interpreter) shows the same two leaders independently —lookup_typed_ar*3.4%,is_register*1.9% — in a program that uses neither. Two unrelated workloads, same tax: this is a generic-path cost, not a workload quirk.Mechanism
#7474 established the shape for
Map/Set: a monotone "has anything ever been registered" flag answers for programs that use neither, with no counter to get wrong. This generalises it intoregistry_latch::RegistryLatchand applies it to every remaining address-keyed type probe on a generic path.A probe checks one process-global
AtomicBoolbefore touching its table. Idle ⟹ answer "no" immediately. Previously each probe cost at minimum a_tlv_get_addr(Darwin has no local-exec TLS), aRefCellborrow and a hash — and two of them (is_registered_symbol,is_uint8array_buffer) took a process-global mutex on every miss, in every process, for every non-Symbol / non-Uint8Array value.RegistryLatchdeliberately has nodisarm. Monotone is the whole safety argument: the only incorrect observation the design can produce is "idle while the table is non-empty". "Armed while every table is empty" is merely slower. There is no counter to get wrong and no unregister/probe race.Ordering — why it is right
The binding rule, documented on the type:
arm()runs BEFORE the registry mutation it advertises, in the registering thread's program order. Arming after the insert opens exactly the window this PR must not create — feature live and reachable, latch still idle, probe takes the fast path and denies a genuinely registered address. This repo already learned that lesson once:js_buffer_register_externallatchesEXTERNAL_BUFFERS_NONEMPTYbefore its insert and says why inline.With the arm placed first:
BUFFER_REGISTRY,TYPED_ARRAY_REGISTRY,UINT8ARRAY_FROM_CTOR, …): only the arming thread can find their entries at all, and a thread always observes its own prior store. No window exists.external_buffers,SHARED_SAB_REGISTRY,SYMBOL_POINTERS): a thread can only probe an address it holds, and every route by which an address reaches another thread in this runtime passes through a synchronising edge (theSerializedValuedeep-copy queue, thePENDING_THREAD_RESULTSdrain — both mutex/channel mediated). The arm precedes the registration, which precedes the hand-off, so the arm is in the reader's happens-before past.Acquire/Releaseis therefore stronger than today's routes require —Relaxedwould be sound given those edges. It costs one instruction and removes the need to re-audit this file the next time someone publishes a heap address through a lock-free path, so the stronger ordering is what ships.One ordering bug was found and fixed on the way in:
shared_sab::alloc_shared_sablatchedSHARED_SAB_NONEMPTYafter its registry insert. Harmless before (the address could not escape the call that returns it), but it is the wrong pattern to leave next to a new latch that depends on the right one. It now arms first, and also armsis_registered_buffer's latch — a SAB backing reads as a registered buffer without ever enteringBUFFER_REGISTRY, so the local latch has to know about it.Probe sites latched
Found by grepping the pattern, not by following the profile — several of these never appear in either profile:
typedarray::lookup_typed_array_kindbuffer::is_registered_bufferis_shared_sabsymbol::is_registered_symbolMutexbuffer::is_data_viewbuffer::is_uint8array_bufferbuffer::is_array_bufferbuffer::is_shared_array_bufferregex::is_regex_pointer/is_valid_regex_ptr/is_registered_regexbuffer::buffer_ab_aliasbuffer::crypto_key_metabuffer::asymmetric_key_metabuffer::is_detached_bufferbuffer::is_secret_keytypedarray_props::typed_array_addr_from_valueandtypedarray::is_offheap_sidetable_allocare fixed transitively — the latter is what puts these probes on everyDate/Temporalbrand check.Correctness
Speed was the easy half.
registry_latch_probes.rscovers each feature with the case where an ordering bug would hide: take the probe's idle fast path first, then register, then require the probe to find it. Typed arrays,Buffer,Uint8Array,ArrayBuffer,DataView, detached buffers,SharedArrayBuffer(including one allocated on another thread),Symbol,MapandSetall get that treatment, plus an "unregistered address misses every probe" sweep run against a heap-plausible pointer with readable bytes in front of it, so the probes that read aGcHeaderare exercised on an arbitrary pointer.The ordering rule itself is proven rather than asserted:
latch_semanticsmodels both orderings against a private latch+table pair.arm_before_publish_is_never_observably_inconsistentshows the correct order has no window;arm_after_publish_is_observably_inconsistentrequires the wrong order to produce one. If that second test ever stops failing under sabotage, the rule has stopped being load-bearing and the first test is proving nothing.Verified the suite discriminates by deleting the
arm()fromregister_typed_array:typed_array_is_found_after_the_idle_fast_path_ranfails withleft: None, right: Some(7).Measured (quiet M1 mini, best-of-N, absolute seconds)
Two commits: the latches, then an inline split. The second exists because the first told me something. After the latches,
_tlv_get_addrfell from 19.9% → 14.6% on asyncpipe and 10.8% → 5.5% on interp — the thread-local resolutions were gone — but the probes' own self-time did not move at all (lookup_typed_array_kind0.267 s before, 0.267 s after). With the latch check behind a non-inlinable cross-module call, what was left was the call. Splitting each probe into an inlinable idle check plus an#[inline(never)]slow path removed it.Absolute seconds
Interleaved A/B/C on the mini (round-robin so contention hits all three arms equally, best of 7). Baseline is this branch's parent,
27d5358d0, built and measured identically:asyncpipeinterpiso_miss(canary)Protected benchmarks, same interleaved run — no regression anywhere, and the allocation-adjacent ones improve:
churntreechurn_alloctree_widechurn_readretainpush_clsretain_widepush_numfib40cyclesdeeplistEvery protected benchmark is inside its ceiling, and
fib40(pure integer compute, no probe on its path) is flat at 0.393 in both arms — the control that says the harness is measuring what it claims to.(The host was shared with another agent's benchmark campaign throughout. Every number above is best-of-N with the arms interleaved round-robin, so contention lands on all arms equally and the minimum converges on the uncontended time;
push_numis the one benchmark whose 0.14 s scale leaves it visibly noisy, and it sits inside its 0.15 ceiling in both arms.)Profile shares (leaf,
PERRY_DEBUG_SYMBOLS=1,_bigvariants, two agreeing runs each)asyncpipe_big:typedarray::lookup_typed_array_kindbuffer::header::is_registered_buffershared_sab::is_shared_sabbuffer::header::is_uint8array_buffersymbol::is_registered_symboltypedarray_props::typed_array_addr_from_valueregex::is_regex_pointerset::is_registered_setmap::is_registered_map_tlv_get_addr(collateral)interp_big:lookup_typed_array_kindis_registered_bufferis_shared_sabis_uint8array_bufferis_registered_symboltyped_array_addr_from_valueis_registered_set+is_registered_map_tlv_get_addrEvery probe for a feature the program does not use is now at zero. The residual is entirely features the programs do use:
is_registered_set+is_registered_map— 5.2% of asyncpipe, 0.9% of interp. Both programs useMap/Set, so the perf(runtime): cache the hot thread-locals so one allocation pays one _tlv_get_addr #7474 latch is armed and the lookups are real work. A latch cannot help here; a negative cache could, butMap/Setheaders are arena-allocated and recyclable, so the address-keyed memo that is sound for typed arrays (pinned, never moved) would be an ABA hazard for them. Left alone deliberately.is_registered_symbol_slow— 0.7% of both, and neither program mentionsSymbol. The runtime's own iterator protocol registers the well-known symbols (Promise.allandfor…ofboth reachjs_get_iterator→well_known_symbol), so the latch is armed from startup. What is left is the process-globalMutexon every miss. That wants a different fix and is the clearest follow-up this work exposes.Summary by CodeRabbit
Performance
Bug Fixes
Documentation
Chores