Skip to content

fix(json): route the traversal-feedback counters through the hot thread-local cache - #10203

Closed
proggeramlug wants to merge 2 commits into
mainfrom
json/traversal-feedback-hot-tls
Closed

proggeramlug wants to merge 2 commits into
mainfrom
json/traversal-feedback-hot-tls

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Summary

scripts/check_thread_locals.py (the "Enforce the thread-local policy ratchet" step of tls-budget.yml's self-test-checkers job, which runs on every PR) fails on current main for two files. One of them is mine: #10150 declared json/traversal_feedback.rs's two per-thread counters with a raw thread_local!, and the policy allows none (every declaration goes through crate::perry_thread_local!, which lands the address in the thread's hot cache instead of paying _tlv_get_addr, #7469). This converts both; the syntax and every .with() call site are unchanged.

The second failing file, regex/perex_owner.rs (one raw declaration, from #10166), is not touched here, so the job stays red on this PR for that file alone.

Validation

  • python3 scripts/check_thread_locals.py --self-test passes; python3 scripts/check_thread_locals.py no longer lists json/traversal_feedback.rs (only regex/perex_owner.rs remains).
  • python3 scripts/gc_runtime_root_holders.py: OK (the two counters keep their existing not_a_gc_pointer verdicts).
  • cargo check --release -p perry-runtime: clean.
  • RUST_TEST_THREADS=1 cargo test --release -p perry-runtime traversal_feedback: 3 passed, 0 failed.

Summary by CodeRabbit

  • Performance
    • Improved JSON parsing performance by optimizing per-thread counter access.
  • Documentation
    • Added a changelog entry describing the JSON parsing performance improvement.

Ralph Küpper added 2 commits September 13, 2026 15:27
…ad-local cache

The thread-local policy ratchet allows no raw thread_local! in the
runtime; #10150 declared json/traversal_feedback.rs's two counters with
one. Both now use crate::perry_thread_local!, with unchanged call sites.
@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 594a0075-a763-4941-a2d8-0de5eac42047

📥 Commits

Reviewing files that changed from the base of the PR and between 5d3bf85 and 388e83a.

📒 Files selected for processing (2)
  • changelog.d/10203-traversal-feedback-hot-tls.md
  • crates/perry-runtime/src/json/traversal_feedback.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The JSON traversal feedback counters now use crate::perry_thread_local!. The comment describes their per-parse reads. A changelog fragment documents the change and its thread-local policy effect.

Changes

Traversal feedback TLS

Layer / File(s) Summary
Hot TLS counter update
crates/perry-runtime/src/json/traversal_feedback.rs, changelog.d/10203-traversal-feedback-hot-tls.md
SCORE and EAGER_RUN now use crate::perry_thread_local!. The comment and changelog describe the hot thread-local access and policy update.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 388e8

This is a localized performance-policy change with validation passing and no identified merge-blocking behavior.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: routing JSON traversal-feedback counters through the hot thread-local cache.
Description check ✅ Passed The description is on-topic and mostly complete. It explains the change, motivation, affected files, unrelated remaining failure, and validation results. It does not use the template headings for Chan…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch json/traversal-feedback-hot-tls

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

CI on this head compared against main's run 34757970864 (fresh, same period): cargo-test fails the same single unrelated test (native_stack::tests::stack_top_respects_custom_thread_stack_sizes, 3711 passed); the gap suite's failing set is identical to main's 10 (empty difference both ways); the failed jobs are main's (warnings, lint public-baseline freshness, check API docs drift, gap-suite shards, gc-stress matrix/merge).

The tls-budget.yml self-test-checkers job still fails on this PR for crates/perry-runtime/src/regex/perex_owner.rs alone (from #10166, not touched here); json/traversal_feedback.rs is no longer listed. Taking this out of draft.

@proggeramlug
proggeramlug marked this pull request as ready for review September 13, 2026 14:18
proggeramlug pushed a commit that referenced this pull request Sep 13, 2026
(cherry picked from commit 388e83a)
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed via merge train #10216 (rebase-merged; main 5d3bf86823, tree identical to the train), cherry-picked onto 44e78debf9 with the version bump to 0.5.1559. Validation and CI attribution are in #10216.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant