perf(runtime): symbol probe stops taking the global mutex to say "no" (third latch instance) - #9177
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe symbol registry now tracks minimum and maximum registered addresses. Lookups can reject out-of-range pointers before locking the registry. All symbol-pointer insertion paths widen the range, including garbage-collector forwarding paths. A copying-minor regression test verifies moved symbols remain discoverable. ChangesSymbol registry range filter
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The PR adds address-range filters before existing registry lookups to reduce mutex overhead without changing accepted registry membership. It is mergeable with owner awareness or follow-up because Symbol classification across some cross-thread handoffs could still depend on range visibility, so a targeted publication test is advisable. Sequence Diagram(s)sequenceDiagram
participant SymbolRegistration
participant SymbolAddressRange
participant SymbolRegistry
participant GarbageCollector
participant SymbolLookup
SymbolRegistration->>SymbolAddressRange: widen registered pointer bounds
SymbolRegistration->>SymbolRegistry: insert symbol pointer
GarbageCollector->>SymbolAddressRange: widen forwarded pointer bounds
GarbageCollector->>SymbolRegistry: reinsert forwarded pointer
SymbolLookup->>SymbolAddressRange: check pointer bounds
SymbolLookup->>SymbolRegistry: lock and probe in-range pointer
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description is detailed and covers the change, rationale, related issue, test results, benchmarks, and kill switch. It omits the template headings and checklist, but the required technical information is mostly present. ✨ 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 |
Third instance of the same defect, after PerryTS#9176's two. `is_registered_symbol` has an idle latch, and the latch stops discriminating the moment a program creates its first symbol — which every program touching a well-known symbol does. After that `is_registered_symbol_slow` took the PROCESS-GLOBAL MUTEX on every probe, including the overwhelming majority asking about pointers that are not symbols at all. The `cc --help` profile puts it at 0.80% (1.9% at higher resolution). An address range, widened before the insert — the same ordering, and for the same reason, as the latch arm directly above it, whose comment already says "Arm before taking the lock, so the entry is never reachable while the latch still reads idle". A pointer outside the range cannot be in the set, so rejecting is sound; accepting falls through to the lookup that was already there. Atomic rather than thread-local because this registry is global, and death pruning removing entries without narrowing the range is harmless: those pointers reach the lookup, which correctly says no. `register_symbol_pointer` is the only insertion site. Quiet Mac mini, medians of 3, a loop doing `typeof` and Map lookups over pointer-tagged values (string keys fail `js_is_symbol`'s tag check before ever reaching the registry, so they measure nothing here): idle — latch never armed, the floor 4.75 us/iter armed, filter on 5.12 armed, filter off 6.31 so 19% off the armed path, and the arming penalty falls from 33% to 7.8%. THE PATTERN, stated because this is the third site: a latch that answers "has this EVER happened?" is only a fast path for programs where it never happens. The moment it does, the latch is pure overhead and something narrower has to take over. The remaining `*_EVER_*` flags guarding a global structure are where the fourth instance will be. perry-runtime lib 2844/0, including all 15 `registry_latch_probes` — among them `symbol_is_found_after_the_idle_fast_path_ran`, `unregistered_address_misses_every_probe`, and the cross-thread SAB case. Kill switch: PERRY_SYMBOL_RANGE_FILTER=0. Claude-Session: https://claude.ai/code/session_01F1dt1jfzK2cheMZyus6y6p
SYMBOL_POINTERS is a GC root registry: the collector re-keys entries when it moves a symbol. Only the registration path widened the range filter, so a symbol evacuated past its allocation's bounds was rejected by is_registered_symbol while still live and registered. Fold widening into the insert so all three sites share it, and cover it through the public probe after a real copying minor.
ba90b0d to
32ecedb
Compare
|
Merged, rebased onto The bug. This lifts the range-filter pattern from #9176, but
A symbol evacuated past the bounds its own allocation established was therefore a live, registered symbol that Widening and inserting are now one operation ( Worth flagging about the test, because it changed my mind mid-fix: my first patch only fixed site 2, and the new test failed anyway — which is how I found site 3. Had I trusted the fix and shipped, this would have gone in still broken, because the existing Validation: Added the Worth applying the same question to any further instance of this pattern: is the registry a GC root registry, and if so does the forwarding rewrite widen? That's the difference between #9176 (safe — buffer addresses are pruned on death, never re-keyed) and this one. |
Hoist PerryTS#9177's symbol address range out of is_registered_symbol_slow into is_registered_symbol as a RegistryAddrWindow, so the common negative answer costs no call; add the same window to is_uint8array_buffer. Both rejections are re-derived from the authoritative tables under debug_assertions. Not yet measured on cc --help.
Hoist PerryTS#9177's symbol address range out of is_registered_symbol_slow into is_registered_symbol as a RegistryAddrWindow, so the common negative answer costs no call; add the same window to is_uint8array_buffer. Both rejections are re-derived from the authoritative tables under debug_assertions. Not yet measured on cc --help.
Hoist PerryTS#9177's symbol address range out of is_registered_symbol_slow into is_registered_symbol as a RegistryAddrWindow, so the common negative answer costs no call; add the same window to is_uint8array_buffer. Both rejections are re-derived from the authoritative tables under debug_assertions. Not yet measured on cc --help.
Hoist PerryTS#9177's symbol address range out of is_registered_symbol_slow into is_registered_symbol as a RegistryAddrWindow, so the common negative answer costs no call; add the same window to is_uint8array_buffer. Both rejections are re-derived from the authoritative tables under debug_assertions. Not yet measured on cc --help.
Hoist PerryTS#9177's symbol address range out of is_registered_symbol_slow into is_registered_symbol as a RegistryAddrWindow, so the common negative answer costs no call; add the same window to is_uint8array_buffer. Both rejections are re-derived from the authoritative tables under debug_assertions. Not yet measured on cc --help.
…9225's linear scan gated — cc --help −1.25% instructions, −2.37% cycles (#9291) * wip(runtime): address windows for the symbol and Uint8Array probes Hoist #9177's symbol address range out of is_registered_symbol_slow into is_registered_symbol as a RegistryAddrWindow, so the common negative answer costs no call; add the same window to is_uint8array_buffer. Both rejections are re-derived from the authoritative tables under debug_assertions. Not yet measured on cc --help. * perf(runtime): a monotone address FILTER in front of the symbol and class-prototype probes Round 2 (#9272) put an inline [lo, hi] address window in front of the buffer and typed-array probes. Measured against the four probes it named as follow-up, a window is the wrong shape for two of them and the right shape for one: is_registered_symbol 378,163 calls, window rejects 38.3% is_registered_class_prototype_object 26,290 calls, window rejects 54.0% is_uint8array_buffer 537,921 calls, window rejects 100% Symbols and class prototypes are ordinary GC-heap objects, so [lo, hi] grows to cover most of the heap. RegistryAddrFilter is the same monotone contract over a 1024-bit Bloom filter instead of a range; replaying each probe's real argument stream from a cc --help run, it rejects 99.58% and 99.05%. is_uint8array_buffer keeps the cheaper window (100% rejection, 0 true answers). Every rejection is re-derived from the authoritative table under debug_assertions, so a registration route added without admitting panics in the first test that touches it. * changelog: registry-probe address filter (symbol, class prototype) + Uint8Array window * test(runtime): keep TEST_SYMBOL_REGISTRY_PROBES meaning 'entry past the latch' Two sabotage checks in other suites defeat a cheaper upstream screen and require this counter to move; counting filter admissions instead made them fail. Filter admissions get their own counter, mirroring typedarray::TEST_TA_WINDOW_ADMITTED_PROBES. * test(runtime): the unregistered-scratch probe sweep covers the class-prototype probe too * docs(runtime): the descriptor-target scan comment's premise is false for every bundle (#9225) * docs(runtime): name the filter's saturation regime and the knob for it * fix(runtime): the debug audits use try_lock/try_read, not lock/read The rejection path never took either lock, so a blocking audit could hang on a caller the audited code would not have. Sabotage-checked: removing the admit from either registration funnel fails 1 test (symbol) and 3 tests (class prototype), so the audits demonstrably run. * docs(runtime): the symbol side's comments and the funnel's name say 'filter', not 'range' * docs(runtime): bits accrue per admission (the collector re-keys both tables); record the end-of-run false-positive rate * revert: unrelated cargo fmt reformat of two perry-codegen files They are fmt-dirty on pristine main (42d0f45) from #9274/#9279; a stray `cargo fmt --all` picked them up. Reported separately, not fixed here. * test: split the #8067 shape-authority tests out of parent_static.rs parent_static.rs was at 1992 lines on main and this PR adds 52, crossing the 2000-line cap. Extracts the inline shape_authority_tests_8067 module to a sibling under parent_static/; body unchanged. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Stacked on #9176 — its commit shows here until that merges. Third instance of the same defect.
The pattern, now confirmed three times
A latch that answers "has this ever happened?" is only a fast path for programs where it never happens. The moment it does, the latch is pure overhead and something narrower has to take over.
is_registered_buffer— falls to a TLS +RefCell+ hash lookup (perf(runtime): registry probes stop paying a hash and a mutex to say "no" (5.3% of cc --help) #9176)is_uint8array_buffer— falls to the global mutex on every miss (perf(runtime): registry probes stop paying a hash and a mutex to say "no" (5.3% of cc --help) #9176)is_registered_symbol— falls to the global mutex on every probe (here)Every program that touches a well-known symbol creates one, so this latch stops discriminating almost immediately. After that
is_registered_symbol_slowtakes the process-global mutex for every probe, and the overwhelming majority are asking about pointers that are not symbols at all. Thecc --helpprofile puts it at 0.80%, 1.9% at higher resolution.I'd note the pattern in review terms: the remaining
*_EVER_*flags guarding a global structure are where the fourth instance will be, and it's cheaper to grep for them than to wait for a profile to surface each one.The change
An address range, widened before the insert — the same ordering, and for the same reason, as the latch arm directly above it, whose comment already says:
A pointer outside the range cannot be in the set, so rejecting is sound; accepting falls through to the lookup that was already there. Atomic rather than thread-local because this registry is global, unlike #9176's.
register_symbol_pointeris the only insertion site — that audit is the load-bearing part of the argument, since a missed insertion would be a silent false negative. Death pruning removes entries without narrowing the range, which is harmless: those pointers reach the lookup, which correctly says no.Numbers
Quiet Mac mini, medians of 3. A loop doing
typeofandMaplookups over pointer-tagged values — string keys failjs_is_symbol's tag check before ever reaching the registry, so a string-keyed benchmark measures nothing here, which cost me a probe to discover:19% off the armed path, and the arming penalty falls from 33% to 7.8%.
Gates
-D warningsclean; perry-runtime lib 2844/0, including all 15registry_latch_probes— among themsymbol_is_found_after_the_idle_fast_path_ran,unregistered_address_misses_every_probe, and the cross-threadshared_array_buffer_allocated_on_another_thread_is_found_here. Census, address-classification, file-size and raw-handle-debt lints all pass with none raised.Kill switch
PERRY_SYMBOL_RANGE_FILTER=0restores the unconditional mutex acquisition.Summary by CodeRabbit
typeof, symbol-keyed properties, and iterator dispatch, after memory movement.