perf(runtime): reach the hot-TLS cache without _tlv_get_addr (#7469 structural half) - #7565
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe runtime adds validated direct pthread TSD access for the hot TLS cache on Apple aarch64. It caches ChangesDirect TLS cache access
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HotPath as tls_hot::hot
participant DarwinTSD as darwin_tsd
participant HotTls
participant LearnedFields as learned_inline_fields
HotPath->>DarwinTSD: Read direct TPIDRRO_EL0 address
DarwinTSD->>HotTls: Return published per-thread cache
HotPath->>HotTls: Fall back to TLS initialization when unavailable
HotTls->>LearnedFields: Provide cached table address
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Raw GC-ratchet A/B, both arms same sessionBoth arms built from one target directory with the identical The one semantic cell is
That last row is #7559 verbatim. It does not move here in either direction — this PR is not its cause and not its fix. The eight two-sided breaches on Caveat stated plainly: this pair ran on the shared dev machine (load 42), so only the semantic families — which the harness documents at 0.000% spread and which are machine-independent — carry weight. The wall-time table in the PR body is from the pinned quiet host. |
BLOCKER — this branch breaks
|
| arm | test_gap_net_connect_bound_value |
|---|---|
origin/main |
5/5 exit 0, correct output (round trip: echo:hello) |
| this branch | 5/5 SIGABRT (134) |
Deterministic in both directions. The branch arm was re-verified after force-rebuilding the ext staticlibs (cargo clean -p perry-ext-{net,http,fetch} --release first), because cargo did not re-emit them when only perry-runtime changed — so the first comparison had a stale archive on one side. The failure survives that correction.
Symptom
connect : function
createConnection : function
thread '<unnamed>' panicked at tokio-1.53.1/src/net/tcp/listener.rs:304:22:
there is no reactor running, must be called from the context of a Tokio 1.x runtime
2: <tokio::runtime::scheduler::Handle>::current
3: <tokio::net::tcp::listener::TcpListener>::bind_addr
4: perry_ext_net::js_net_server_listen::{closure#1}
5: tokio::runtime::task::core::Core<…, Arc<multi_thread::handle::Handle>>::poll
…
9: <tokio::runtime::scheduler::current_thread::Context>::run_task
13: <tokio::runtime::runtime::Runtime>::block_on_inner::<…async_bridge::run_one_tick…>
14: perry_stdlib::common::async_bridge::run_one_tick
15: _js_wait_for_event
perry_ffi_spawn_blocking_with_reactor and the rest of perry-stdlib's async bridge are linked (checked in the symbol table of an unstripped build), so this is not the #6314 stub-shadowing shape. The task reaches TcpListener::bind without a reactor in scope.
The whole gap-suite failure cluster is the same family — 6 of the 7 crashes are network (fetch_request_from_node_incoming_message, 4x http_*, net_connect_bound_value).
Ruled out
- Not the ext archives being absent. Reproduces with
libperry_ext_{net,http,fetch}.apresent and freshly built against this branch's rlib. - Not the runtime: ext staticlibs bundle perry-runtime and shadow perry-stdlib's real symbols with its no-op stubs (node:http servers get no tokio reactor) #6314 stub shadowing. The real perry-stdlib symbols are in the binary.
- Not the 7th crash.
test_gap_gc_same_module_call_argument_rootingis a harness timeout, not a defect: standalone under its ownparity-env(PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_ZEAL=1) it exits 0 in 13.5 s with output byte-identical to node (bad 0), against the harness's 10 s cap.
Also worth recording from the same run
- The gap gate cannot render a verdict on macOS at all:
scripts/run_gap_tests.shselectstest-parity/gap_snapshot.${platform}.jsonfor non-Linux hosts, andtest-parity/gap_snapshot.macos.jsondoes not exist in the repo. The run ends inFileNotFoundError, soGAP_HARNESS_EXIT=1here is a missing-baseline error, not a computed regression list. Raw result was 466 pass / 18 parity-fail / 7 crash. - Of the 18 mismatches, 10 are snapshot
node_failentries that now compare (node 26.5.1 succeeds where the committed Linux snapshot expected it to fail) and 6 are snapshotparity_fail; onlytest_gap_diagchannel_3082_3084_3085_3086andtest_gap_zlib_3285_paramsare unlisted, and both are in the host-local family already recorded for this machine.
Next
Bisecting the two commits, and testing whether options(pure) on the TPIDRRO_EL0 read is implicated — pure lets LLVM CSE and hoist the thread-pointer read, which is unsound across any point where execution can resume on a different thread. That is the one property of this change that could plausibly interact with the async bridge.
Resolved: the blocker was
|
test_gap_net_connect_bound_value |
5/5 exit 0 (was 5/5 SIGABRT) |
fetch_request_from_node_incoming_message, http_overloads_3226plus, http_req_async_iterator, http_res_socket_writable_onfinished |
pass |
http_client_no_redirect_follow |
still fails — byte-identically on main (status=307 then the same TypeError: Cannot read properties of undefined (reading 'location')), so pre-existing on this host, not this change |
gc_same_module_call_argument_rooting |
never was a defect: standalone under its own parity-env it exits 0 in 13.5 s with output byte-identical to node, against the harness's 10 s cap |
Performance is unchanged by the fix
Re-measured on the pinned quiet host, same interleaved best-of-7 method — the pure build and the fixed build land on the same millisecond:
| probe | main |
fixed | ratio |
|---|---|---|---|
churn_alloc |
1.294 s | 1.109 s | 1.167x |
churn |
1.649 s | 1.403 s | 1.175x |
push_cls |
1.263 s | 1.104 s | 1.144x |
So giving up the CSE cost nothing measurable, which is the right trade even setting soundness aside.
The uncomfortable part, stated plainly
Nothing cheaper than the gap suite found this. 1811 perry-runtime unit tests, all 12 GC-ratchet probes, the whole allocation benchmark set, and PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 on three probes are all green with the bug present, because none of them runs an async server. The three tls_hot tests added here assert the fast path is live and reads the right address — they cannot see a hoist.
Un-drafting; the change is now clean against a same-package-set main on every test that differs.
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 `@crates/perry-runtime/src/tls_hot.rs`:
- Around line 277-365: Update darwin_tsd::ensure_key, publish, and hot so the
pthread key remains private until the first thread’s slot is successfully set
and validated; use an explicit initialization state and make hot_via_tls() the
fallback while initialization or disablement is in progress. Prevent disable()
from racing with readers that already loaded the key, coordinating state
transitions and direct access so an invalid TSD value can never be interpreted
as &HotTls.
🪄 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: d38ffb3a-6363-4b7c-942f-8833c55ff483
📒 Files selected for processing (5)
changelog.d/7565-tls-direct-tsd.mdcrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/spill.rscrates/perry-runtime/src/tls_hot.rsdocs/engine-plan.md
… aarch64 #7474 cached the addresses of the thread-locals on the allocation path, but `HOT` is itself a `thread_local!`, so every runtime function reading any hot field still paid one `_tlv_get_addr` call. Symbolicated on the pinned quiet host at 9938cbc that residue is 27.0% of `churn_alloc` self time, and the call-graph attribution is concentrated rather than diffuse: seven functions carry 98% of it and every one of them resolves `HOT`. Publish the cache's address into one `pthread_key_create` slot and read it back through `TPIDRRO_EL0`, which is how `pthread_getspecific` itself is implemented and what mimalloc (already linked here) does on this platform. The resolution becomes `mrs` plus two loads that LLVM can CSE across a function instead of an out-of-line call that clobbers caller-saved registers. The publishing thread reads its slot back through the direct path and compares it against what `pthread_setspecific` was handed; a mismatch disables the direct path process-wide and every thread falls back to `_tlv_get_addr`. There is no path on which a wrong address reaches the allocator. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
With `HOT` reachable without `_tlv_get_addr`, the residual on `churn_alloc` was 3.5% of self time and 100% of it attributed to one caller, `js_object_alloc_class_inline_keys` — which is `learned_inline_field_count`, run on every dynamic construct to right-size the inline slot count. The other thread-locals that function names statically (`MARK_SEEDS`, `WRITE_BARRIER_TRACE_COUNTERS`) sit behind cold gates and never resolve. Route it through the existing address cache, following the four-step contract in `tls_hot`: slot, provider next to the `thread_local!`, wiring in `fill`, and the pairing assertion that stands between a mis-wire and a well-typed reference to the wrong object. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
The first version of this change marked the thread-pointer asm `options(pure, nomem)` so LLVM could CSE it across a function. `pure` promises the result depends only on the inputs, and this asm has none, so LLVM may compute it once and reuse the value anywhere in the function — including across a point where execution resumes on a *different* thread. `perry-stdlib`'s async bridge is exactly that shape: `hot()` is `#[inline(always)]` and LTO inlines it into futures tokio polls, so a hoisted thread pointer outlives the thread it was read on. Every `node:net` / `node:http` server aborted with tokio's "there is no reactor running", 5/5, against 5/5 clean on `main`. No unit test and no allocation benchmark reproduced it — it took the gap suite. The counter-argument that `@llvm.threadlocal.address` already has this freedom does not hold: on Darwin it lowers to a call through the TLV descriptor, which LLVM will not hoist across arbitrary code. Replacing the call with inline asm is what made the hoist possible. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
e6cef53 to
66a4aa8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Closes the structural half of #7469:
_tlv_get_addrfalls from 27.0% to 1.1% ofchurn_allocself time, and the three allocation probes get 1.14x–1.18x faster on the pinned quiet host.Requirement zero — re-measured before scoping
Three tickets in this campaign were worked off stale headline numbers, so the first thing here is a fresh symbolicated profile, not a design.
churn_allocon the pinned quiet host (perry-macos, M1 mini, load 1.5) at9938cbc1a, 923 leaf samples:_tlv_get_addris 27.0% — the ticket's 30.5% has not drifted.What had drifted was the shape. The ticket opened saying "41 distinct call-graph sites — this is diffuse, not one hot caller". It is no longer diffuse. Seven functions carry 98% of it, and every one of the seven resolves
tls_hot::HOT:_tlv_get_addrgc::barrier::write_barrier_decoded_parentgc::layout_tables::layout_forget_objectjs_object_alloc_class_inline_keysarena::allocators::arena_allocjs_write_barrier_slotgc::barrier::barrier_child_prologuegc::layout::typed_shape_layout_entryTwo of the seven resolve nothing else. Static disassembly of the emitted binary names which thread-local each site resolves (there is no
bl _tlv_get_addrto grep for on Mach-O — the call is indirect through the TLV descriptor, so the census walksadrp/addpairs landing in__thread_vars).That attribution chose the design. The accessor is the lever, not the call graph: the ticket's option 1, "thread a context pointer through generated code", would have to cross every runtime FFI boundary against 2994
.with()sites over 255thread_local!blocks; makinghot()free fixes all seven at once and composes with everything downstream.What changed
Commit 1. On Apple aarch64 the pthread thread-specific-data array is directly addressable from
TPIDRRO_EL0— that is howpthread_getspecificitself is implemented, and what mimalloc (already linked into this runtime) does on this platform.tls_hotpublishes the cache's address into onepthread_key_createslot and reads it back inline:mrsplus two loads, no call, no caller-saved-register clobber. Every other target keeps the previous path unchanged, byte for byte.Commit 3 is a bug this PR shipped and then caught — see the comments below. The asm was originally
options(pure, nomem)so LLVM could CSE it across a function.purepromises the result depends only on the inputs, and this asm has none, so LLVM may compute it once and reuse the value anywhere in the function including across a point where execution resumes on a different thread — andperry-stdlib's async bridge is exactly that shape, sincehot()is#[inline(always)]and LTO inlines it into futures tokio polls. Everynode:net/node:httpserver aborted with "there is no reactor running", 5/5 against 5/5 clean onmain. The counter-argument that@llvm.threadlocal.addressalready has that freedom does not hold: on Darwin it lowers to a call through the TLV descriptor, which LLVM will not hoist. Droppingpurefixes it and, measured, costs nothing — the three probes land on the same millisecond either way.Commit 2. With
HOTfree,_tlv_get_addrfell to 3.5% and 100% of the residue attributed to a single caller —js_object_alloc_class_inline_keys, i.e.learned_inline_field_count, run on every dynamic construct.LEARNED_INLINE_FIELDSjoins the address cache under the same four-step contract. (The other thread-locals that function names statically,MARK_SEEDSandWRITE_BARRIER_TRACE_COUNTERS, sit behind cold gates and never resolve;ARENA_TOTAL_BYTES/OLD_GEN_IN_USE_BYTESinarena_alloconly move when a block is installed. None were touched — this campaign's own lesson about optimising what the profile did not measure.)It cannot silently read a wrong address
The publishing thread reads its slot back through the direct path and compares it against what
pthread_setspecificwas handed. A mismatch — the shape a future OS change would take — latches the direct path off process-wide and every thread reverts to_tlv_get_addr, permanently. There is no path on which a wrong address reaches the allocator. A fresh thread reads null and takes the cold path, which is POSIX ("upon thread creation, the value NULL shall be associated with all defined keys in the new thread"), not an implementation detail.The fast path is asserted live, not assumed
Every other test in
tls_hotpasses identically whetherhot()costs a call or three instructions, so a silent fallback would make this inert with nothing red.direct_tsd_path_is_live— fails if the direct path was disabled, and checks the direct read against the published address.direct_read_matches_pthread_getspecific— checks the open-coded read against the real libpthread implementation for the same key, rather than against our belief about it.a_fresh_thread_publishes_its_own_slot— worker-thread coverage.HOTdescriptor materialisations across those seven functions are gone from the emitted binary, replaced byTPIDRRO_EL0reads (3 / 3 / 2 / 2 / 1 / 1 / 7 per function). Whole-binarymrscount 104 → 482.Results
Pinned quiet host, arms interleaved round by round so load drift hits both equally, best-of-7 after a discarded warm-up,
/usr/bin/time -l:mainchurn_alloc— object literal + pushchurn— literal + push + read backpush_cls—new Node(v,w)+ push_tlv_get_addr: 27.0% → 3.5% → 1.1%, and what is left isRuntimeHandleScope, not the allocation path. That answers the ceiling question for this lever in both directions: the prize was real, and it is now spent — further thread-local work on this path is worth at most ~1%.Peak RSS flat (25.2 → 25.2–25.4 MB). Program output byte-identical across all three arms on all three probes.
Validation
cargo test -p perry-runtime --no-fail-fast: 1811 passed, 0 failed, 3 ignored.GC, instrument proven live: all three probes under
PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800 PERRY_GC_DIAG=1exit 0 with correct output and 110[gc-fromspace-protect] mode=ProtectPages retired_set=#Nlines each — a run with zero copying minors protects nothing, so the count is quoted rather than the exit code.GC ratchet, measured against a same-session
mainbuild rather than argued from the filed numbers. Both arms built from the same target directory with the identical-pset, measured back to back with the same harness:05_closure_captureheap_used_bytes +16.44%,02_survivor_promotion+2.77%). This change moves none of them in either direction.12_large_live_set.heap_used_bytesat +0.004% (2,304 B). That is the one cell the ratchet's ownprobe_overrideexcludes from gating as conservative-stack-scan sample noise (gc-ratchet has measured nothing since 2026-08-05: harness validation fails before the probes run (12_large_live_set spread 6768) #7554/gc(): the forced conservative stack scan makes every retained-heap reading nondeterministic, and 16% too large on a 50 MB live set #7558), documented spread 9,072 B over 36 runs, so 2,304 B sits well inside it. Every retention and evacuation counter on the other eleven probes is bit-identical, and every probe checksum matches.wall_msis faster on this branch on all twelve probes (−1.3% to −13.3%), even though that run happened on the dev machine at load 42.{samples, median, …}dicts and a naive numeric read compares zero of them and prints a vacuous "identical".cargo fmt --all -- --check,scripts/check_file_size.sh,scripts/gc_store_site_inventory.py,scripts/addr_class_inventory.pyall clean;raw_handle_debt.py998, at baseline.Gap suite, 466/491 on this host — this is what caught the
purebug. After the fix, five of the six network aborts pass and the sixth (http_client_no_redirect_follow) fails byte-identically onmain; the seventh crash was a harness timeout (13.5 s under its own zealparity-envagainst a 10 s cap, output byte-identical to node). Separately worth fixing: the gap gate cannot render a verdict on macOS at all —run_gap_tests.shselectstest-parity/gap_snapshot.${platform}.jsonfor non-Linux hosts andgap_snapshot.macos.jsonis not in the repo, so the run ends inFileNotFoundErrorand its exit 1 is a missing baseline, not a regression list.CI has a deep runner backlog and may not report; the above is local validation on the pinned host and the dev machine.
Refs #7469.
Summary by CodeRabbit
Performance
Documentation