perf(tls): thread-locals are on the fast path by default, with a gate that can fail (#7469) - #7758
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds ChangesGeneric hot TLS
TLS validation and measurement
Release metadata
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant RustBuild
participant TLSBudgetGate
participant Sample
participant TLSBudgetCheck
GitHubActions->>RustBuild: Build Perry and runtime archives
RustBuild-->>TLSBudgetGate: Release artifacts
TLSBudgetGate->>TLSBudgetGate: Compile and validate asyncpipe and interp
TLSBudgetGate->>Sample: Collect runtime profile
Sample-->>TLSBudgetGate: Return .sample report
TLSBudgetGate->>TLSBudgetCheck: Submit profile and TLS statistics
TLSBudgetCheck-->>GitHubActions: Return PASS or FAIL budget result
Possibly related PRs
Suggested labels: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
scripts/check_thread_locals.py (1)
65-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe ratchet only sees brace-delimited macro invocations.
RAW_REandHOT_RErequire{after the macro name. Rust acceptsthread_local!(...)andthread_local![...]as well. A raw declaration written with parentheses or brackets passes the checker silently, which is the exact failure mode this script exists to prevent. The same gap makesHOT_REundercount hot declarations, which weakens theHOT_SLOT_CAPACITYcheck.
block_bodiesalso assumes the opening delimiter is{; it needs the matching delimiter pair to stay in sync.♻️ Suggested change to accept all three delimiters
-RAW_RE = re.compile(r"(?m)^[ \t]*(?:std::)?thread_local!\s*\{") -HOT_RE = re.compile(r"(?m)^[ \t]*(?:crate::)?perry_thread_local!\s*\{") +RAW_RE = re.compile(r"(?m)^[ \t]*(?:std::)?thread_local!\s*[\{\(\[]") +HOT_RE = re.compile(r"(?m)^[ \t]*(?:crate::)?perry_thread_local!\s*[\{\(\[]")
block_bodiesthen needs to pick the closing delimiter from the opening one:OPEN_TO_CLOSE = {"{": "}", "(": ")", "[": "]"} def block_bodies(src: str, pattern: re.Pattern[str]) -> list[str]: """Bodies of every macro block `pattern` starts, delimiter-matched.""" bodies = [] for m in pattern.finditer(src): i = min( (src.index(o, m.start()) for o in OPEN_TO_CLOSE if o in src[m.start() : m.end()]) ) opener = src[i] closer = OPEN_TO_CLOSE[opener] depth = 0 j = i while j < len(src): if src[j] == opener: depth += 1 elif src[j] == closer: depth -= 1 if depth == 0: break j += 1 bodies.append(src[i + 1 : j]) return bodiesAdd a fifth
self_testcase that writesthread_local!(static E: u8 = const { 0 });and assertsverifyrejects it.Do you want me to open an issue to track this?
🤖 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 `@scripts/check_thread_locals.py` around lines 65 - 73, Update RAW_RE and HOT_RE to match thread_local! and perry_thread_local! invocations using {}, (), or [] delimiters. Add delimiter-pair handling in block_bodies via an opening-to-closing mapping so nested bodies are matched correctly, and extend self_test with a parenthesized raw declaration that verify rejects.benchmarks/tls-budget/asyncpipe.ts (1)
34-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo documented coverage goals are not exercised.
The comment on Line 57 states that each iteration creates a closure capturing a per-iteration binding. The loop calls
handle(req, rates)directly, so no closure capturesreqork. The file header on Lines 8-9 makes the same claim.
validatethrows only whenr.amount <= 0.makeReqsetsamount = (i % 97) + 1, soamountis always >= 1. Thecatchblock on Lines 87-90 never runs, and the finalerrorsfield of the expected output is a constant0.The workload arithmetic itself is correct: I verified the expected
39197275total and theuserSumidentity against the gate fixture.If you want the closure and error paths measured, make them reachable.
♻️ Suggested change to exercise both paths
async function runBatch(base: number, size: number, rates: Map<string, number>): Promise<Ok[]> { // Closures created inside a loop, each capturing a per-iteration binding. const jobs: Promise<Ok>[] = []; for (let k = 0; k < size; k++) { const req = makeReq(base + k); - jobs.push(handle(req, rates)); + const run = (): Promise<Ok> => handle(req, rates); + jobs.push(run()); }Note that changing the reachable request set changes the expected stdout in
scripts/tls_budget_gate.sh.Also applies to: 56-65
🤖 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 `@benchmarks/tls-budget/asyncpipe.ts` around lines 34 - 37, Update the benchmark loop around handle and validate so each iteration invokes the work through a closure that captures that iteration’s req or k binding, as promised by the file documentation. Adjust makeReq or the generated request set so at least one request has a non-positive amount, making validate’s throw and the catch path reachable, then update the expected stdout in scripts/tls_budget_gate.sh to match the resulting totals and error count.
🤖 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 @.github/workflows/tls-budget.yml:
- Around line 27-32: Update the follow-through documentation around the
tls-budget workflow to explicitly track promoting both tls-budget and
self-test-checkers into branch protection’s required contexts after their first
green run on main. Preserve the existing rationale and reference to the
gc-root-dominance precedent, and record the tracking issue or actionable
maintainer follow-up rather than leaving promotion as an unassigned question.
In `@crates/perry-runtime/src/tls_hot.rs`:
- Around line 1190-1216: Replace the process-global claimed_slots() sampling in
crates/perry-runtime/src/tls_hot.rs:1190-1216 with per-declaration slot_index()
results: have each worker return the three probe indices, then assert they are
in range, distinct, and identical across all eight workers. Apply the same
correction in crates/perry-runtime/src/tls_hot.rs:1300-1313 by removing
after_main/after_workers, capturing each registry declaration’s slot_index() on
the main thread, returning those indices from every worker, and asserting they
match.
- Around line 523-579: Update the exit-time report function in
maybe_install_stats_hook so it does not call blocking claimed_slots(); use a
non-blocking try_lock on CLAIM_LOCK and report an explicit unknown value when
the lock is unavailable, while preserving the existing claimed count when the
lock is acquired.
In `@scripts/tls_budget_check.py`:
- Line 284: Remove the unnecessary f-string prefix from the PASS verdict print
statement in the TLS budget check, leaving it as a regular string literal with
identical output.
In `@scripts/tls_budget_gate.sh`:
- Around line 110-121: Update the profiling block around sample and wait to
remove the `|| true` suppression, delete any existing `$OUT_DIR/$name.sample`
before sampling, and explicitly capture both `sample` and profiled-process
statuses without allowing `set -e` to abort the loop. Report failures, set `rc`,
and continue so every subject reaches `interp` and final aggregation; retain the
empty-sample validation for the current run. Revise the header claim to describe
the explicit status-capture approach instead of claiming the script contains no
`|| true`.
---
Nitpick comments:
In `@benchmarks/tls-budget/asyncpipe.ts`:
- Around line 34-37: Update the benchmark loop around handle and validate so
each iteration invokes the work through a closure that captures that iteration’s
req or k binding, as promised by the file documentation. Adjust makeReq or the
generated request set so at least one request has a non-positive amount, making
validate’s throw and the catch path reachable, then update the expected stdout
in scripts/tls_budget_gate.sh to match the resulting totals and error count.
In `@scripts/check_thread_locals.py`:
- Around line 65-73: Update RAW_RE and HOT_RE to match thread_local! and
perry_thread_local! invocations using {}, (), or [] delimiters. Add
delimiter-pair handling in block_bodies via an opening-to-closing mapping so
nested bodies are matched correctly, and extend self_test with a parenthesized
raw declaration that verify rejects.
🪄 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: 6188c212-993d-425e-84eb-788aac0ef86d
📒 Files selected for processing (63)
.github/workflows/tls-budget.ymlbenchmarks/tls-budget/asyncpipe.tsbenchmarks/tls-budget/interp.tschangelog.d/7758-tls-hot-by-default.mdcrates/perry-runtime/src/arena/inline.rscrates/perry-runtime/src/array/element_shape.rscrates/perry-runtime/src/array/header.rscrates/perry-runtime/src/array/iterator.rscrates/perry-runtime/src/box.rscrates/perry-runtime/src/buffer/detach.rscrates/perry-runtime/src/buffer/header.rscrates/perry-runtime/src/buffer/view.rscrates/perry-runtime/src/closure/alloc.rscrates/perry-runtime/src/closure/dynamic_props.rscrates/perry-runtime/src/closure/registry.rscrates/perry-runtime/src/exception.rscrates/perry-runtime/src/gc/roots.rscrates/perry-runtime/src/lib.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/object/arguments.rscrates/perry-runtime/src/object/async_generator_queue.rscrates/perry-runtime/src/object/class_constructors.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/class_registry/dispatch.rscrates/perry-runtime/src/object/class_registry/state.rscrates/perry-runtime/src/object/collection_proto_thunks.rscrates/perry-runtime/src/object/exotic_expando.rscrates/perry-runtime/src/object/field_get_set/accessors.rscrates/perry-runtime/src/object/field_get_set/field_ops.rscrates/perry-runtime/src/object/global_fetch.rscrates/perry-runtime/src/object/global_this/fetch_globals.rscrates/perry-runtime/src/object/global_this/populate.rscrates/perry-runtime/src/object/handle_expando.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/native_module_stream.rscrates/perry-runtime/src/object/native_this_alias.rscrates/perry-runtime/src/object/prop_plan.rscrates/perry-runtime/src/object/prototype_chain.rscrates/perry-runtime/src/object/this_binding.rscrates/perry-runtime/src/promise/async_step.rscrates/perry-runtime/src/promise/combinators.rscrates/perry-runtime/src/promise/microtasks.rscrates/perry-runtime/src/promise/mod.rscrates/perry-runtime/src/promise/reactions.rscrates/perry-runtime/src/promise/rejection.rscrates/perry-runtime/src/promise/spec_combinators.rscrates/perry-runtime/src/promise/then.rscrates/perry-runtime/src/regex.rscrates/perry-runtime/src/set.rscrates/perry-runtime/src/state.rscrates/perry-runtime/src/string/format.rscrates/perry-runtime/src/string/intern.rscrates/perry-runtime/src/symbol.rscrates/perry-runtime/src/tls_hot.rscrates/perry-runtime/src/typedarray/mod.rscrates/perry-runtime/src/typedarray_props.rscrates/perry-runtime/src/value/to_string.rsscripts/check_thread_locals.pyscripts/thread_local_cold_allowlist.jsonscripts/tls_budget_check.pyscripts/tls_budget_gate.sh
| # 2. NOT wired into branch protection's required contexts by the change that | ||
| # adds it -- a new gate has never been green, so promoting it immediately | ||
| # would block every open PR (CLAUDE.md's corollary). That is a maintainer | ||
| # action after the first observed green run on `main`, and per the | ||
| # corollary it is not optional follow-through: `gc-root-dominance` sat red | ||
| # on `main` for weeks because the second step was never taken. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Track the branch-protection promotion as required follow-through.
The coding guidelines require a CI gate to be required by branch protection. This workflow is not in the required contexts. The comment states the reason and names the follow-up, and the changelog repeats it. The comment also records that gc-root-dominance sat red on main for weeks because the second step was never taken.
The design supports promotion already: the relevance filter skips steps rather than skipping the job, so the job always concludes on every pull request.
Do you want me to open a tracking issue for adding tls-budget and self-test-checkers to the required contexts after the first green run on main?
As per coding guidelines, "A CI gate must be required by branch protection".
🤖 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 @.github/workflows/tls-budget.yml around lines 27 - 32, Update the
follow-through documentation around the tls-budget workflow to explicitly track
promoting both tls-budget and self-test-checkers into branch protection’s
required contexts after their first green run on main. Preserve the existing
rationale and reference to the gc-root-dominance precedent, and record the
tracking issue or actionable maintainer follow-up rather than leaving promotion
as an unassigned question.
Source: Coding guidelines
| /// How many slots *this thread* has populated. | ||
| pub fn published_slots() -> usize { | ||
| hot().slots.iter().filter(|s| !s.get().is_null()).count() | ||
| } | ||
|
|
||
| /// `PERRY_TLS_HOT_STATS=1` — print, at process exit, what this mechanism | ||
| /// actually did. | ||
| /// | ||
| /// This exists so a budget gate can assert its subject was LIVE rather than | ||
| /// merely quiet. `_tlv_get_addr` reading 0% is the *same observation* whether | ||
| /// the cache carried the program's thread-locals or the program simply never | ||
| /// resolved one, and #7469's history is that the second case shipped as a pass | ||
| /// three times. The line reports: | ||
| /// | ||
| /// * `claimed` — declarations that took a slot process-wide. A program that | ||
| /// exercises paths outside the sixteen named fields drives this well past | ||
| /// zero; a program that does not, does not. | ||
| /// * `published` — slots this thread actually filled. | ||
| /// * `direct_tsd` — whether `hot()` is the `mrs`-plus-two-loads path. `0` | ||
| /// means the self-check rejected direct addressing and every access is | ||
| /// paying `_tlv_get_addr` again, i.e. the whole mechanism is inert. | ||
| fn maybe_install_stats_hook() { | ||
| static INSTALLED: std::sync::Once = std::sync::Once::new(); | ||
| INSTALLED.call_once(|| { | ||
| if !matches!( | ||
| std::env::var("PERRY_TLS_HOT_STATS").as_deref(), | ||
| Ok("1") | Ok("on") | Ok("true") | ||
| ) { | ||
| return; | ||
| } | ||
| extern "C" fn report() { | ||
| #[cfg(all( | ||
| target_vendor = "apple", | ||
| target_arch = "aarch64", | ||
| target_pointer_width = "64" | ||
| ))] | ||
| let direct = u8::from(darwin_tsd::active()); | ||
| #[cfg(not(all( | ||
| target_vendor = "apple", | ||
| target_arch = "aarch64", | ||
| target_pointer_width = "64" | ||
| )))] | ||
| let direct = 0u8; | ||
| eprintln!( | ||
| "[tls-hot] claimed={} published={} capacity={} direct_tsd={}", | ||
| claimed_slots(), | ||
| published_slots(), | ||
| HOT_SLOT_CAPACITY, | ||
| direct, | ||
| ); | ||
| } | ||
| // SAFETY: `report` is `extern "C"`, takes nothing and returns nothing. | ||
| unsafe { | ||
| libc::atexit(report); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make the atexit reporter non-blocking on CLAIM_LOCK.
report runs during process exit and calls claimed_slots(), which blocks on CLAIM_LOCK. Other threads can still run at that point. If a thread holds CLAIM_LOCK inside SlotId::claim when exit runs, the reporter blocks forever and the process hangs instead of exiting. The gate script sets PERRY_TLS_HOT_STATS, so a hang here would stall the CI job rather than fail it with a diagnostic.
Use try_lock in the exit path and report an explicit unknown value when the lock is held.
🔒️ Proposed fix for the exit-time lock
+/// As [`claimed_slots`], but never blocks — for the exit reporter, which must
+/// not wait on a thread that is still claiming.
+fn claimed_slots_now() -> Option<u32> {
+ match CLAIM_LOCK.try_lock() {
+ Ok(next) => Some(*next),
+ Err(std::sync::TryLockError::Poisoned(p)) => Some(*p.into_inner()),
+ Err(std::sync::TryLockError::WouldBlock) => None,
+ }
+}
+
extern "C" fn report() {
@@
eprintln!(
"[tls-hot] claimed={} published={} capacity={} direct_tsd={}",
- claimed_slots(),
+ claimed_slots_now().map_or(-1i64, i64::from),
published_slots(),
HOT_SLOT_CAPACITY,
direct,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// How many slots *this thread* has populated. | |
| pub fn published_slots() -> usize { | |
| hot().slots.iter().filter(|s| !s.get().is_null()).count() | |
| } | |
| /// `PERRY_TLS_HOT_STATS=1` — print, at process exit, what this mechanism | |
| /// actually did. | |
| /// | |
| /// This exists so a budget gate can assert its subject was LIVE rather than | |
| /// merely quiet. `_tlv_get_addr` reading 0% is the *same observation* whether | |
| /// the cache carried the program's thread-locals or the program simply never | |
| /// resolved one, and #7469's history is that the second case shipped as a pass | |
| /// three times. The line reports: | |
| /// | |
| /// * `claimed` — declarations that took a slot process-wide. A program that | |
| /// exercises paths outside the sixteen named fields drives this well past | |
| /// zero; a program that does not, does not. | |
| /// * `published` — slots this thread actually filled. | |
| /// * `direct_tsd` — whether `hot()` is the `mrs`-plus-two-loads path. `0` | |
| /// means the self-check rejected direct addressing and every access is | |
| /// paying `_tlv_get_addr` again, i.e. the whole mechanism is inert. | |
| fn maybe_install_stats_hook() { | |
| static INSTALLED: std::sync::Once = std::sync::Once::new(); | |
| INSTALLED.call_once(|| { | |
| if !matches!( | |
| std::env::var("PERRY_TLS_HOT_STATS").as_deref(), | |
| Ok("1") | Ok("on") | Ok("true") | |
| ) { | |
| return; | |
| } | |
| extern "C" fn report() { | |
| #[cfg(all( | |
| target_vendor = "apple", | |
| target_arch = "aarch64", | |
| target_pointer_width = "64" | |
| ))] | |
| let direct = u8::from(darwin_tsd::active()); | |
| #[cfg(not(all( | |
| target_vendor = "apple", | |
| target_arch = "aarch64", | |
| target_pointer_width = "64" | |
| )))] | |
| let direct = 0u8; | |
| eprintln!( | |
| "[tls-hot] claimed={} published={} capacity={} direct_tsd={}", | |
| claimed_slots(), | |
| published_slots(), | |
| HOT_SLOT_CAPACITY, | |
| direct, | |
| ); | |
| } | |
| // SAFETY: `report` is `extern "C"`, takes nothing and returns nothing. | |
| unsafe { | |
| libc::atexit(report); | |
| } | |
| }); | |
| } | |
| /// How many slots *this thread* has populated. | |
| pub fn published_slots() -> usize { | |
| hot().slots.iter().filter(|s| !s.get().is_null()).count() | |
| } | |
| /// As [`claimed_slots`], but never blocks — for the exit reporter, which must | |
| /// not wait on a thread that is still claiming. | |
| fn claimed_slots_now() -> Option<u32> { | |
| match CLAIM_LOCK.try_lock() { | |
| Ok(next) => Some(*next), | |
| Err(std::sync::TryLockError::Poisoned(p)) => Some(*p.into_inner()), | |
| Err(std::sync::TryLockError::WouldBlock) => None, | |
| } | |
| } | |
| /// `PERRY_TLS_HOT_STATS=1` — print, at process exit, what this mechanism | |
| /// actually did. | |
| /// | |
| /// This exists so a budget gate can assert its subject was LIVE rather than | |
| /// merely quiet. `_tlv_get_addr` reading 0% is the *same observation* whether | |
| /// the cache carried the program's thread-locals or the program simply never | |
| /// resolved one, and `#7469`'s history is that the second case shipped as a pass | |
| /// three times. The line reports: | |
| /// | |
| /// * `claimed` — declarations that took a slot process-wide. A program that | |
| /// exercises paths outside the sixteen named fields drives this well past | |
| /// zero; a program that does not, does not. | |
| /// * `published` — slots this thread actually filled. | |
| /// * `direct_tsd` — whether `hot()` is the `mrs`-plus-two-loads path. `0` | |
| /// means the self-check rejected direct addressing and every access is | |
| /// paying `_tlv_get_addr` again, i.e. the whole mechanism is inert. | |
| fn maybe_install_stats_hook() { | |
| static INSTALLED: std::sync::Once = std::sync::Once::new(); | |
| INSTALLED.call_once(|| { | |
| if !matches!( | |
| std::env::var("PERRY_TLS_HOT_STATS").as_deref(), | |
| Ok("1") | Ok("on") | Ok("true") | |
| ) { | |
| return; | |
| } | |
| extern "C" fn report() { | |
| #[cfg(all( | |
| target_vendor = "apple", | |
| target_arch = "aarch64", | |
| target_pointer_width = "64" | |
| ))] | |
| let direct = u8::from(darwin_tsd::active()); | |
| #[cfg(not(all( | |
| target_vendor = "apple", | |
| target_arch = "aarch64", | |
| target_pointer_width = "64" | |
| )))] | |
| let direct = 0u8; | |
| eprintln!( | |
| "[tls-hot] claimed={} published={} capacity={} direct_tsd={}", | |
| claimed_slots_now().map_or(-1i64, i64::from), | |
| published_slots(), | |
| HOT_SLOT_CAPACITY, | |
| direct, | |
| ); | |
| } | |
| // SAFETY: `report` is `extern "C"`, takes nothing and returns nothing. | |
| unsafe { | |
| libc::atexit(report); | |
| } | |
| }); | |
| } |
🤖 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/tls_hot.rs` around lines 523 - 579, Update the
exit-time report function in maybe_install_stats_hook so it does not call
blocking claimed_slots(); use a non-blocking try_lock on CLAIM_LOCK and report
an explicit unknown value when the lock is unavailable, while preserving the
existing claimed count when the lock is acquired.
| #[test] | ||
| fn concurrent_first_touch_claims_one_slot_per_declaration() { | ||
| let before = super::claimed_slots(); | ||
| let workers: Vec<_> = (0..8) | ||
| .map(|_| { | ||
| std::thread::spawn(|| { | ||
| PROBE_CONST.with(|c| c.get()); | ||
| PROBE_EXPR.with(|v| v.borrow().len()); | ||
| PROBE_SECOND.with(|c| c.get()); | ||
| }) | ||
| }) | ||
| .collect(); | ||
| for w in workers { | ||
| w.join().expect("probe thread panicked"); | ||
| } | ||
| let after = super::claimed_slots(); | ||
| assert!( | ||
| after - before <= 3, | ||
| "8 threads first-touching 3 declarations claimed {} slots", | ||
| after - before | ||
| ); | ||
| assert!( | ||
| (after as usize) < super::HOT_SLOT_CAPACITY, | ||
| "slot capacity {} exhausted at {after} claims", | ||
| super::HOT_SLOT_CAPACITY | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Both new capacity tests infer per-test behavior from the process-global claimed_slots() counter. The test harness runs tests in parallel inside one process, so any other test that first-touches a converted declaration changes the counter inside the sampling window. Assert on slot_index() values, which are per-declaration and stable, instead of on counter deltas.
crates/perry-runtime/src/tls_hot.rs#L1190-L1216: drop thebefore/aftersamples. Returnslot_index()for the three probes from each worker and assert the indices are in range, distinct, and identical across all eight workers.crates/perry-runtime/src/tls_hot.rs#L1300-L1313: dropafter_main/after_workers. Capture each registry declaration'sslot_index()on the main thread, return the same indices from every worker, and assert they match.
📍 Affects 1 file
crates/perry-runtime/src/tls_hot.rs#L1190-L1216(this comment)crates/perry-runtime/src/tls_hot.rs#L1300-L1313
🤖 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/tls_hot.rs` around lines 1190 - 1216, Replace the
process-global claimed_slots() sampling in
crates/perry-runtime/src/tls_hot.rs:1190-1216 with per-declaration slot_index()
results: have each worker return the three probe indices, then assert they are
in range, distinct, and identical across all eight workers. Apply the same
correction in crates/perry-runtime/src/tls_hot.rs:1300-1313 by removing
after_main/after_workers, capturing each registry declaration’s slot_index() on
the main thread, returning those indices from every worker, and asserting they
match.
| for sym, n in profile.callers.most_common(8): | ||
| print(f" {n:6d} {sym}") | ||
| if ok: | ||
| print(f" VERDICT : PASS") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the extraneous f prefix.
The string has no placeholders. Ruff reports F541 here.
🔧 Proposed fix
- print(f" VERDICT : PASS")
+ print(" VERDICT : PASS")As per static analysis hints, Ruff (0.16.1) flags f-string without any placeholders on this line.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print(f" VERDICT : PASS") | |
| print(" VERDICT : PASS") |
🧰 Tools
🪛 Ruff (0.16.1)
[error] 284-284: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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 `@scripts/tls_budget_check.py` at line 284, Remove the unnecessary f-string
prefix from the PASS verdict print statement in the TLS budget check, leaving it
as a regular string literal with identical output.
Source: Linters/SAST tools
| echo "--- profiling $name (${SAMPLE_SECONDS}s)" | ||
| PERRY_TLS_HOT_STATS=1 "$exe" >/dev/null 2>"$OUT_DIR/$name.stats" & | ||
| pid=$! | ||
| sleep 1 | ||
| sample "$pid" "$SAMPLE_SECONDS" -f "$OUT_DIR/$name.sample" >/dev/null 2>&1 || true | ||
| wait "$pid" | ||
|
|
||
| if [[ ! -s "$OUT_DIR/$name.sample" ]]; then | ||
| echo "tls-budget: sample produced no report for $name" >&2 | ||
| rc=1 | ||
| continue | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The profiling block loses two failure signals and contradicts the header.
Line 114 uses || true. The header on Line 17 states the script contains no || true. The empty-file check on Line 117 is the only backstop, and it is not sufficient: OUT_DIR is caller-supplied and the workflow passes a fixed path, so a stale $name.sample from a previous invocation satisfies -s and the gate returns a verdict on an old profile.
Line 115 runs wait "$pid" as a simple command under set -e. If the profiled program exits non-zero, the script terminates on that line. The loop then never reaches interp, rc aggregation is skipped, and the final exit "$rc" never runs. The job goes red, so no false pass occurs, but one subject is dropped without a message.
🔧 Proposed fix
echo "--- profiling $name (${SAMPLE_SECONDS}s)"
+ rm -f "$OUT_DIR/$name.sample"
PERRY_TLS_HOT_STATS=1 "$exe" >/dev/null 2>"$OUT_DIR/$name.stats" &
pid=$!
sleep 1
- sample "$pid" "$SAMPLE_SECONDS" -f "$OUT_DIR/$name.sample" >/dev/null 2>&1 || true
- wait "$pid"
+ sample_rc=0
+ sample "$pid" "$SAMPLE_SECONDS" -f "$OUT_DIR/$name.sample" >/dev/null 2>&1 || sample_rc=$?
+ run_rc=0
+ wait "$pid" || run_rc=$?
+ if [[ "$run_rc" -ne 0 ]]; then
+ echo "tls-budget: the profiled run of $name exited $run_rc" >&2
+ rc=1
+ continue
+ fi
+ if [[ "$sample_rc" -ne 0 ]]; then
+ echo "tls-budget: sample exited $sample_rc for $name" >&2
+ rc=1
+ continue
+ fi
if [[ ! -s "$OUT_DIR/$name.sample" ]]; thenThen update the header claim on Line 17 to describe the explicit status capture.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "--- profiling $name (${SAMPLE_SECONDS}s)" | |
| PERRY_TLS_HOT_STATS=1 "$exe" >/dev/null 2>"$OUT_DIR/$name.stats" & | |
| pid=$! | |
| sleep 1 | |
| sample "$pid" "$SAMPLE_SECONDS" -f "$OUT_DIR/$name.sample" >/dev/null 2>&1 || true | |
| wait "$pid" | |
| if [[ ! -s "$OUT_DIR/$name.sample" ]]; then | |
| echo "tls-budget: sample produced no report for $name" >&2 | |
| rc=1 | |
| continue | |
| fi | |
| echo "--- profiling $name (${SAMPLE_SECONDS}s)" | |
| rm -f "$OUT_DIR/$name.sample" | |
| PERRY_TLS_HOT_STATS=1 "$exe" >/dev/null 2>"$OUT_DIR/$name.stats" & | |
| pid=$! | |
| sleep 1 | |
| sample_rc=0 | |
| sample "$pid" "$SAMPLE_SECONDS" -f "$OUT_DIR/$name.sample" >/dev/null 2>&1 || sample_rc=$? | |
| run_rc=0 | |
| wait "$pid" || run_rc=$? | |
| if [[ "$run_rc" -ne 0 ]]; then | |
| echo "tls-budget: the profiled run of $name exited $run_rc" >&2 | |
| rc=1 | |
| continue | |
| fi | |
| if [[ "$sample_rc" -ne 0 ]]; then | |
| echo "tls-budget: sample exited $sample_rc for $name" >&2 | |
| rc=1 | |
| continue | |
| fi | |
| if [[ ! -s "$OUT_DIR/$name.sample" ]]; then | |
| echo "tls-budget: sample produced no report for $name" >&2 | |
| rc=1 | |
| continue | |
| fi |
🤖 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 `@scripts/tls_budget_gate.sh` around lines 110 - 121, Update the profiling
block around sample and wait to remove the `|| true` suppression, delete any
existing `$OUT_DIR/$name.sample` before sampling, and explicitly capture both
`sample` and profiled-process statuses without allowing `set -e` to abort the
loop. Report failures, set `rc`, and continue so every subject reaches `interp`
and final aggregation; retain the empty-sample validation for the current run.
Revise the header claim to describe the explicit status-capture approach instead
of claiming the script contains no `|| true`.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 642-649: Update SlotGuard::drop so teardown never calls hot()
after TLS storage has been destroyed, avoiding the hot_via_tls()
initialization/fill path during late destructors. Preserve and use the published
HotTls pointer during teardown, or otherwise clear the slot directly without
triggering hot() initialization, while retaining the existing bounds check and
slot-clearing behavior.
In `@scripts/check_thread_locals.py`:
- Around line 118-120: Update the raw TLS counting logic around RAW_RE so each
matched macro block contributes the number of DECL_RE matches within its body,
rather than counting blocks themselves; retain the existing per-file raw[rel]
aggregation used by verify(). Extend the self-test fixture with a second static
declaration inside one allowlisted thread_local! block and update its expected
count.
In `@scripts/tls_budget_gate.sh`:
- Around line 57-62: Normalize OUT_DIR to an absolute path immediately after its
initial assignment, before mkdir, executable-path construction, or any later cd.
Preserve the caller’s relative directory while ensuring compilation and
subsequent executable checks resolve the same location.
- Around line 101-107: Guard the correctness execution in the actual assignment
so a non-zero exit from "$exe" does not trigger set -e in the surrounding loop.
Preserve the existing mismatch check, set rc=1, and continue processing the
remaining entries, matching the failure-handling behavior of the compilation
path.
🪄 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: 23a58f25-1820-4820-8e96-753c8068441b
📒 Files selected for processing (63)
.github/workflows/tls-budget.ymlbenchmarks/tls-budget/asyncpipe.tsbenchmarks/tls-budget/interp.tschangelog.d/7758-tls-hot-by-default.mdcrates/perry-runtime/src/arena/inline.rscrates/perry-runtime/src/array/element_shape.rscrates/perry-runtime/src/array/header.rscrates/perry-runtime/src/array/iterator.rscrates/perry-runtime/src/box.rscrates/perry-runtime/src/buffer/detach.rscrates/perry-runtime/src/buffer/header.rscrates/perry-runtime/src/buffer/view.rscrates/perry-runtime/src/closure/alloc.rscrates/perry-runtime/src/closure/dynamic_props.rscrates/perry-runtime/src/closure/registry.rscrates/perry-runtime/src/exception.rscrates/perry-runtime/src/gc/roots.rscrates/perry-runtime/src/lib.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/object/arguments.rscrates/perry-runtime/src/object/async_generator_queue.rscrates/perry-runtime/src/object/class_constructors.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/class_registry/dispatch.rscrates/perry-runtime/src/object/class_registry/state.rscrates/perry-runtime/src/object/collection_proto_thunks.rscrates/perry-runtime/src/object/exotic_expando.rscrates/perry-runtime/src/object/field_get_set/accessors.rscrates/perry-runtime/src/object/field_get_set/field_ops.rscrates/perry-runtime/src/object/global_fetch.rscrates/perry-runtime/src/object/global_this/fetch_globals.rscrates/perry-runtime/src/object/global_this/populate.rscrates/perry-runtime/src/object/handle_expando.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/native_module_stream.rscrates/perry-runtime/src/object/native_this_alias.rscrates/perry-runtime/src/object/prop_plan.rscrates/perry-runtime/src/object/prototype_chain.rscrates/perry-runtime/src/object/this_binding.rscrates/perry-runtime/src/promise/async_step.rscrates/perry-runtime/src/promise/combinators.rscrates/perry-runtime/src/promise/microtasks.rscrates/perry-runtime/src/promise/mod.rscrates/perry-runtime/src/promise/reactions.rscrates/perry-runtime/src/promise/rejection.rscrates/perry-runtime/src/promise/spec_combinators.rscrates/perry-runtime/src/promise/then.rscrates/perry-runtime/src/regex.rscrates/perry-runtime/src/set.rscrates/perry-runtime/src/state.rscrates/perry-runtime/src/string/format.rscrates/perry-runtime/src/string/intern.rscrates/perry-runtime/src/symbol.rscrates/perry-runtime/src/tls_hot.rscrates/perry-runtime/src/typedarray/mod.rscrates/perry-runtime/src/typedarray_props.rscrates/perry-runtime/src/value/to_string.rsscripts/check_thread_locals.pyscripts/thread_local_cold_allowlist.jsonscripts/tls_budget_check.pyscripts/tls_budget_gate.sh
🚧 Files skipped from review as they are similar to previous changes (58)
- crates/perry-runtime/src/object/instanceof.rs
- crates/perry-runtime/src/lib.rs
- crates/perry-runtime/src/array/element_shape.rs
- crates/perry-runtime/src/object/class_registry/state.rs
- crates/perry-runtime/src/object/mod.rs
- crates/perry-runtime/src/symbol.rs
- crates/perry-runtime/src/string/format.rs
- crates/perry-runtime/src/string/intern.rs
- crates/perry-runtime/src/object/arguments.rs
- crates/perry-runtime/src/object/global_fetch.rs
- crates/perry-runtime/src/promise/mod.rs
- crates/perry-runtime/src/typedarray_props.rs
- crates/perry-runtime/src/buffer/header.rs
- crates/perry-runtime/src/object/handle_expando.rs
- crates/perry-runtime/src/promise/then.rs
- crates/perry-runtime/src/object/class_registry/construct.rs
- crates/perry-runtime/src/exception.rs
- crates/perry-runtime/src/object/async_generator_queue.rs
- crates/perry-runtime/src/closure/alloc.rs
- crates/perry-runtime/src/closure/registry.rs
- crates/perry-runtime/src/regex.rs
- crates/perry-runtime/src/object/field_get_set/accessors.rs
- crates/perry-runtime/src/arena/inline.rs
- crates/perry-runtime/src/state.rs
- crates/perry-runtime/src/array/header.rs
- crates/perry-runtime/src/object/native_module.rs
- crates/perry-runtime/src/typedarray/mod.rs
- crates/perry-runtime/src/object/class_constructors.rs
- crates/perry-runtime/src/closure/dynamic_props.rs
- crates/perry-runtime/src/object/class_registry/dispatch.rs
- crates/perry-runtime/src/promise/rejection.rs
- crates/perry-runtime/src/promise/reactions.rs
- crates/perry-runtime/src/array/iterator.rs
- benchmarks/tls-budget/asyncpipe.ts
- crates/perry-runtime/src/promise/async_step.rs
- crates/perry-runtime/src/buffer/detach.rs
- crates/perry-runtime/src/object/this_binding.rs
- crates/perry-runtime/src/object/native_module_stream.rs
- crates/perry-runtime/src/object/global_this/populate.rs
- crates/perry-runtime/src/object/native_this_alias.rs
- crates/perry-runtime/src/promise/spec_combinators.rs
- crates/perry-runtime/src/buffer/view.rs
- crates/perry-runtime/src/object/exotic_expando.rs
- crates/perry-runtime/src/set.rs
- scripts/thread_local_cold_allowlist.json
- crates/perry-runtime/src/object/global_this/fetch_globals.rs
- crates/perry-runtime/src/object/prop_plan.rs
- crates/perry-runtime/src/gc/roots.rs
- crates/perry-runtime/src/promise/microtasks.rs
- benchmarks/tls-budget/interp.ts
- crates/perry-runtime/src/object/collection_proto_thunks.rs
- crates/perry-runtime/src/object/prototype_chain.rs
- crates/perry-runtime/src/box.rs
- .github/workflows/tls-budget.yml
- crates/perry-runtime/src/promise/combinators.rs
- crates/perry-runtime/src/value/to_string.rs
- crates/perry-runtime/src/map.rs
- crates/perry-runtime/src/object/field_get_set/field_ops.rs
| impl Drop for SlotGuard { | ||
| fn drop(&mut self) { | ||
| let idx = self.idx.get(); | ||
| if (idx as usize) < HOT_SLOT_CAPACITY { | ||
| hot().set_slot(idx, std::ptr::null_mut()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect hot() and fill() for lazy per-call initialization reachable from a destructor.
set -euo pipefail
ast-grep outline crates/perry-runtime/src/tls_hot.rs --items all
sed -n '200,445p' crates/perry-runtime/src/tls_hot.rsRepository: PerryTS/perry
Length of output: 13473
🏁 Script executed:
#!/bin/bash
# Description: Inspect all thread-local accessors used by fill, HotKey, and HotCell to determine destructor reachability.
set -euo pipefail
# Locate the hot_addr definitions and HotTls fields used in the initial inspection.
rg -n "hot_addr|temp_roots|layout_slot_masks|gc::|arena::|object::" crates -g '*.rs' | head -n 200 || true
# Map each field written by fill to its accessor definition/source module if present.
python3 - <<'PY'
from pathlib import Path
import re
src = Path("crates/perry-runtime/src/tls_hot.rs").read_text()
fields = [
"arena", "inline_state", "page_generation_cache", "page_generations",
"arena_free_list", "arena_free_list_nonempty", "birth_extra_flags",
"incremental_mark_valid_ptrs", "incremental_mark_minor_only",
"layout_slot_masks", "typed_layouts", "shape_layouts",
"per_object_layouts_nonempty", "shape_install_memo",
"learned_inline_fields", "temp_roots",
]
for f in fields:
print(f"--- {f} ---")
for line in src.splitlines():
if re.search(rf"\b({re.escape(f)}\s*=\s*)", line):
print(line)
PY
# Show all occurrences and likely definitions for the hot_addr helpers.
rg -n "pub(crate)(:)? fn .*_hot_addr|fn .*_hot_addr|thread_local|thread_local!" crates -g '*.rs' | head -n 250 || true
# Static check: whether hot_uncached calls hot_via_tls, and how hot_via_tls fills.
python3 - <<'PY'
from pathlib import Path
text = Path("crates/perry-runtime/src/tls_hot.rs").read_text()
print("has_hot_uncached_calls_hot_via_tls:", "let slots = hot_via_tls();" in text[text.find("fn hot_uncached"):text.find("/// The per-thread address cache. On Apple", text.find("fn hot_uncached"))])
print("has_hot_via_tls_fills_on_first_use:", "if (*slots).temp_roots.is_null() {" in text and "fill(slots);" in text[text.find("fn hot_via_tls"):text.find("/// Direct pthread thread-specific-data addressing", text.find("fn hot_via_tls"))])
PYRepository: PerryTS/perry
Length of output: 43632
🏁 Script executed:
#!/bin/bash
# Description: Find definitions of all _hot_addr helpers used by fill().
set -euo pipefail
for mod in arena gc object; do
echo "--- $mod/module ---"
fd -a 'mod\.rs$' crates/perry-runtime/src/$mod 2>/dev/null | sort
done
echo "--- definitions search ---"
rg -n "pub\(crate\) fn (arena|inline_state|page_generation_cache|page_generations|(arena_free_list|birth_extra_flags|incremental_mark_(valid_ptrs|minor_only)|incremental_mark_minor_only|layout_slot_masks|typed_layouts|shape_layouts|per_object_layouts_nonempty|shape_install_memo|learned_inline_fields|temp_roots|gc_write_barriers_emitted|js_shadow_frame_push|js_shadow_slot_set)_hot_addr)|fn .*_hot_addr" crates/perry-runtime/src crates/perry-codegen/src -g '*.rs' || trueRepository: PerryTS/perry
Length of output: 2791
🏁 Script executed:
#!/bin/bash
# Description: Inspect the bodies of all _hot_addr accessors used by fill().
set -euo pipefail
for f in \
crates/perry-runtime/src/arena/block.rs \
crates/perry-runtime/src/arena/page_meta.rs \
crates/perry-runtime/src/object/spill.rs \
crates/perry-runtime/src/gc/fmt.rs \
crates/perry-runtime/src/gc/shape_install.rs \
crates/perry-runtime/src/gc/roots/temp_roots.rs \
crates/perry-runtime/src/arena/fence.rs \
crates/perry-runtime/src/arena/block_cache.rs \
crates/perry-runtime/src/arena/mod.rs
do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n -A 20 -B 3 "(arena_hot_addr|inline_state_hot_addr|page_generation_cache_hot_addr|page_generations_hot_addr|birth_extra_flags_hot_addr|incremental_mark_valid_ptrs_hot_addr|incremental_mark_minor_only_hot_addr|layout_slot_masks_hot_addr|typed_layouts_hot_addr|shape_layouts_hot_addr|per_object_layouts_nonempty_hot_addr|shape_install_memo_hot_addr|arena_free_list_hot_addr|arena_free_list_nonempty_hot_addr|learned_inline_fields_hot_addr|temp_roots_hot_addr|js_gc_write_barriers_emitted_hot_addr|java_global_slot_hot_addr|thread_slot_hot_addr|perry_thread_local)" "$f" || echo "no matches"
fi
done
echo "--- macro helpers ---"
rg -n -A 25 -B 5 "macro_rules! __perry_thread_local" crates/perry-runtime/src/tls_hot.rsRepository: PerryTS/perry
Length of output: 11766
Keep hot() destructor calls from filling after TLS teardown.
On non-Darwin, hot() routes through hot_via_tls(), whose normal first call runs fill() and resolves many thread-local providers. If SlotGuard::drop() can run after those providers are destroyed, it aborts inside a destructor. Preserve the published HotTls pointer for teardown, or replace the destructor behavior with no hot() fill after TLS storage is gone.
🤖 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/tls_hot.rs` around lines 642 - 649, Update
SlotGuard::drop so teardown never calls hot() after TLS storage has been
destroyed, avoiding the hot_via_tls() initialization/fill path during late
destructors. Preserve and use the published HotTls pointer during teardown, or
otherwise clear the slot directly without triggering hot() initialization, while
retaining the existing bounds check and slot-clearing behavior.
| count = len(RAW_RE.findall(src)) | ||
| if count: | ||
| raw[rel] = count |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Count raw TLS declarations instead of macro blocks.
Lines 118-120 count thread_local! macro blocks. A contributor can add another static declaration inside an existing allowlisted block. The count does not change, so verify() accepts the new raw declaration.
Count DECL_RE matches inside each RAW_RE block. Update the self-test to add a second static in one macro block.
Proposed fix
- count = len(RAW_RE.findall(src))
+ count = sum(
+ len(DECL_RE.findall(body)) for body in block_bodies(src, RAW_RE)
+ )- "thread_local! { static A: u8 = const { 0 }; }\n"
- "thread_local! { static D: u8 = const { 0 }; }\n"
+ "thread_local! {\n"
+ " static A: u8 = const { 0 };\n"
+ " static D: u8 = const { 0 };\n"
+ "}\n"Also applies to: 230-236
🤖 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 `@scripts/check_thread_locals.py` around lines 118 - 120, Update the raw TLS
counting logic around RAW_RE so each matched macro block contributes the number
of DECL_RE matches within its body, rather than counting blocks themselves;
retain the existing per-file raw[rel] aggregation used by verify(). Extend the
self-test fixture with a second static declaration inside one allowlisted
thread_local! block and update its expected count.
| OUT_DIR="${2:-$(mktemp -d)}" | ||
| REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" | ||
| FIXTURES="$REPO_ROOT/benchmarks/tls-budget" | ||
|
|
||
| mkdir -p "$OUT_DIR" | ||
| PERRY_BIN="$(cd "$(dirname "$PERRY_BIN")" && pwd)/$(basename "$PERRY_BIN")" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize OUT_DIR before compiling.
If the caller passes a relative output directory, Line 90 changes into that directory but exe still contains the relative prefix from Line 81. The -o path and the later executable check then resolve relative to different working directories. For example, tmp/tls/asyncpipe resolves as tmp/tls/tmp/tls/asyncpipe during compilation.
Convert OUT_DIR to an absolute path after Line 61.
Proposed fix
mkdir -p "$OUT_DIR"
+OUT_DIR="$(cd "$OUT_DIR" && pwd -P)"
PERRY_BIN="$(cd "$(dirname "$PERRY_BIN")" && pwd)/$(basename "$PERRY_BIN")"Also applies to: 80-90
🤖 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 `@scripts/tls_budget_gate.sh` around lines 57 - 62, Normalize OUT_DIR to an
absolute path immediately after its initial assignment, before mkdir,
executable-path construction, or any later cd. Preserve the caller’s relative
directory while ensuring compilation and subsequent executable checks resolve
the same location.
| # Correctness BEFORE timing: a program that prints the wrong answer is not | ||
| # a faster program. | ||
| actual="$("$exe")" | ||
| if [[ "$actual" != "$expected" ]]; then | ||
| echo "tls-budget: $name printed '$actual', expected '$expected'" >&2 | ||
| rc=1 | ||
| continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Capture correctness-run failures without aborting the loop.
Because set -e is enabled on Line 54, a non-zero exit from Line 103 terminates the script immediately. The script does not update rc, profile interp, or reach the final aggregation.
Guard the assignment, set rc=1, and continue as in the compilation path.
Proposed fix
- actual="$("$exe")"
+ if ! actual="$("$exe")"; then
+ echo "tls-budget: $name correctness run failed" >&2
+ rc=1
+ continue
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Correctness BEFORE timing: a program that prints the wrong answer is not | |
| # a faster program. | |
| actual="$("$exe")" | |
| if [[ "$actual" != "$expected" ]]; then | |
| echo "tls-budget: $name printed '$actual', expected '$expected'" >&2 | |
| rc=1 | |
| continue | |
| # Correctness BEFORE timing: a program that prints the wrong answer is not | |
| # a faster program. | |
| if ! actual="$("$exe")"; then | |
| echo "tls-budget: $name correctness run failed" >&2 | |
| rc=1 | |
| continue | |
| fi | |
| if [[ "$actual" != "$expected" ]]; then | |
| echo "tls-budget: $name printed '$actual', expected '$expected'" >&2 | |
| rc=1 | |
| continue |
🤖 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 `@scripts/tls_budget_gate.sh` around lines 101 - 107, Guard the correctness
execution in the actual assignment so a non-zero exit from "$exe" does not
trigger set -e in the surrounding loop. Preserve the existing mismatch check,
set rc=1, and continue processing the remaining entries, matching the
failure-handling behavior of the compilation path.
…neric HotTls slots Work in progress: mechanism + conversion of the measured-hot subsystems.
…already has slots in
…l (83% of interp's residual _tlv_get_addr)
… leaving it to set -e behind a pipe
282ad29 to
31b1a3b
Compare
…sertion claimed_slots() is process-global, so another test's first touch of a converted declaration lands inside the measurement window. Equality failed 6/6 under load while passing in isolation and single-threaded. The tolerance still separates the outcomes by two orders of magnitude: per-thread claiming would add 5 x 64 = 320, not 1. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Merging as v0.5.1444, with one test fixed on the branchThe framing is the finding
16 hand-wired slots against ~520 declarations, curated against whichever workload was profiled last — so Inverting the default removes the curation entirely, and it also removes a hazard the old contract needed a test to catch: untyped named slots could hand a correctly-typed reference to the wrong object if The teardown reasoning is the subtlest part and it is right. Both gates verified
The sabotage failing on the budget while the liveness assertions stayed satisfied ( And the unplanned demonstration is the best evidence of all: the ratchet failed the build on a raw The one thing I changed
Replaced with a tolerance of 8, documented. It still separates the outcomes by two orders of magnitude — per-thread claiming, the bug it exists to catch, would add 5 declarations × 64 workers = 320, not 1. 0/4 failures after, versus 6/6 before.
|
1 participant
_tlv_get_addrwas 18-20% of the worst realistic program. The mechanism was fine; the coverage policy was the bug.On Darwin every
thread_local!access is an out-of-line call to_tlv_get_addrin libdyld. Two good PRs already attacked it — #7474 built the
HotTlsaddresscache, #7565 made reaching that cache free via Apple aarch64's directly
addressable pthread TSD array. Both are preserved here unchanged.
But the measured share kept coming back:
churn_allocchurn_allocinterp/retain27d5358d0)asyncpipeHotTlshad 16 hand-wired slots against ~520thread_local!declarations,and the 16 were curated against whichever workload was profiled last — the
allocation path.
churnis covered by construction and reads 0% forever.asyncpipepays 18.2% through Map/Set registries, buffer brands, descriptorstate and field-lookup tails that were on nobody's list. Adding a slot took four
manual steps including a hand-written test, and forgetting them produced a
working slow path, not a build error.
What this lands: option 2, invert the default
crate::perry_thread_local!— same syntax asthread_local!, samewith/try_withat every call site, so converting a declaration converts all of itsuses. The value's address lands in a generic slot of the same per-thread cache,
so a read is loads instead of a call. Nothing to wire: no slot, no provider
function, no line in
fill, no line in a test.It also removes the hazard the old contract needed a test to catch. The untyped
named slots could hand out a correctly-typed reference to the wrong object if
fillwas mis-wired; here the storage, the resolver and the key'sTall comefrom one declaration, so the mis-pairing cannot be expressed.
And it is safer on thread teardown than what it extends.
HotCell<T, GUARD>takes
GUARD = needs_drop::<T>() as usizefrom the macro:RefCell<HashMap<…>>→ a one-element guard array whoseDropruns beforethe value's (fields drop in declaration order), un-publishing this thread's
cached address. A later access falls back and gets std's "accessed during or
after destruction" panic instead of reading a dropped map.
Cell<u64>→ a zero-length array, which has no drop glue, soHotCellhasnone: no destructor registered, std's
const-init fast path preserved.The 16 named fields are unchanged and stay a closed set — a fixed offset is
one load cheaper than a claimed slot, and the allocation path is where that
matters. 155 declarations across 51 files use the macro. One more fix in the
same family:
js_inline_arena_statewas resolvingINLINE_STATE/ARENAthrough
.with()even though both are already named slots — a covered entrythe caller wasn't using (5.2% of
interp's remaining calls).Results (quiet M1 mini, best-of-5 interleaved, absolute seconds)
Base and arm at the same commit (
27d5358d0), both built-p perry -p perry-runtime-static -p perry-stdlib-static, isolated target dirs, outputsverified against node before timing.
_tlv_get_addrshare, symbolicated, 8ssample, two runs in agreement:asyncpipe_biginterp_bigWall clock, interleaved best-of-5:
churn_allocis the control: its paths were already covered by the sixteennamed fields, so it is exactly the benchmark that must not move. It does not.
The asyncpipe/interp deltas reproduced across three independent interleaved runs
(-6.4%/-7.0%/-5.4% and -3.7%/-3.4%/-2.9%); everything else stayed inside +-2%,
which is this host's noise floor while a second agent was benchmarking on it
(both arms inflated equally by interleaving; absolute seconds run ~3% above the
same host's quiet reference).
The gates
Two, because they fail on different things.
scripts/check_thread_locals.py— structural. A new rawthread_local!inperry-runtimeis a build error unless recorded inscripts/thread_local_cold_allowlist.jsonas deliberately cold. The countsratchet in both directions: a file that loses a declaration also fails,
because a stale entry is one nobody has to justify any more. It also fails as
declarations approach
HOT_SLOT_CAPACITY, since slot exhaustion is correct butsilent.
--self-testdrives all four rejections.scripts/tls_budget_gate.sh— outcome, and its design is about vacuity.Profiling
churn_alloc— the benchmark every previous fix was tuned against —would pass forever while the real cost grew, because churn's thread-locals are
exactly the covered ones: a gate green because its subject never ran. So the
subjects are
benchmarks/tls-budget/asyncpipe.tsandinterp.ts, andscripts/tls_budget_check.pyrefuses a pass unless the run proves it was live:PERRY_TLS_HOT_STATS=1must reportdirect_tsd=1(otherwisehot()is itselfcalling
_tlv_get_addr, the mechanism is inert, and a low share would mean theprogram resolved nothing) and
claimedabove a floor no allocationmicrobenchmark clears. Seven rejections, all
--self-tested, run compiler-freeon every PR.
Sabotage checks
The budget gate goes red on the regression it exists to catch. Two
declarations reverted to raw
thread_local!—buffer/header.rs'sBUFFER_REGISTRYblock andstate.rs'sSTATE_PTRblock — rebuilt, gate re-run:asyncpipeinterpGATE_EXIT=1, and the liveness assertions stayed satisfied throughout(
direct_tsd=1,claimed=111/102) — so it failed on the budget, not on abroken run. Restoring the macro restores
GATE_EXIT=0.A second, unplanned demonstration: while this branch was being rebased,
mainadded a raw
thread_local!togc/policy.rs. The structural ratchet failed thebuild on it immediately, which is exactly the intended workflow — it is recorded
as cold in this PR rather than converted in a file this change does not own.
The teardown guard's test is also sabotage-checked. Neutering
SlotGuard::dropmakesteardown_unpublishes_a_dropping_valuefail withleft: 2— it reads the droppedVec— proving the test detects a realuse-after-free rather than passing vacuously.
Not wired into branch protection
Deliberately. A new gate has never been green, so promoting it immediately would
block every open PR (CLAUDE.md's corollary). That is a maintainer action after
the first observed green run on
main— and per the same corollary, notoptional follow-through.
Correctness
node --experimental-strip-typesv26.5.1,verified before timing, on both arms, for all 15 programs plus the two
_bigprofiling variants.iso_missprintschecksum 437840 misses 0on both arms.cargo test -p perry-runtime --lib: 1995 passed, 0 failed.converted_declarations_survive_thread_turnoverdrives 64 short-lived threads through the converted registry probes and
asserts indices are claimed per declaration, not per thread;
a_generic_slot_is_per_thread,concurrent_first_touch_claims_one_slot_per_declarationand
teardown_unpublishes_a_dropping_valuecover the rest.Overlap
The three largest
_tlv_get_addrcallers onasyncpipeareis_registered_buffer/is_registered_set/is_registered_map— 55% of it —which is also the subject of the concurrent "remove registry probes from generic
paths" work. The two changes compose (this one makes the resolution cheap; that
one removes the call), but they overlap in measurement: whichever lands second
will show a smaller delta on those symbols. Flagging rather than resolving
unilaterally.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation