chore: merge train 227 (v0.5.1606) - #10763
Merged
Merged
Conversation
…path
Every non-throwing `try` entry paid a real out-of-line `_tlv_get_addr`
call, unconditionally, that nothing else in `try_push_with_kind` did.
Profiling `try { t += v; } catch { t = 0; }` (differenced inside each
binary: `(try80-try16)/64` vs `(loop80-loop16)/64` control, per
CLAUDE.md's verification method, N=20000, median of 7) confirmed the
178-instruction/entry baseline and pinned the cost to one leaf via
`sample`+`PERRY_KEEP_SYMBOLS=1` at 20M iterations:
`try_push_with_kind`+176 -> `_tlv_get_addr`, ~6% of instructions
retired per entry.
Root cause: `CatchSavepoint::capture()`'s shadow field is captured
unconditionally on every entry (`shadow_stack_savepoint`), and once any
function anywhere in the process has pushed a shadow frame — true
almost immediately for any real program, since a `catch (e)` binding
itself needs one — the `SHADOW_FRAMES` latch from #7469's wave-1 fix is
already set, so the latch stops making this read rare in practice.
Unlike `EXCEPTION_STATE`, `CALL_METHOD_DEPTH`, and the named
`runtime_handle_stack`/`temp_roots` fields, `SHADOW` was still a raw
`thread_local!`, so this one read never got the `tls_hot` fast path at
all.
Fix: move `SHADOW` (crates/perry-runtime/src/gc/roots/shadow_stack.rs)
from `thread_local!` to `crate::perry_thread_local!` — a pure
storage-mechanism swap (same type, same `.with()`/`.try_with()` call
sites, same const-init/drop-free semantics: `ShadowStackState` still
has no `Drop`, so no destructor is registered) that changes nothing
about liveness, throw-time restore, or the fixed-address contract
`js_shadow_frame_enter` depends on for its whole-activation pointer
cache. Confirmed via `--trace llvm`/`sample` that the leaf disappears
entirely post-change.
Measured (mybase = own build at this commit's parent, 9df5075,
codegen-units=16; both PERRY_NO_AUTO_OPTIMIZE=1):
before: try 179.0-179.4 instr/entry, control -0.001..-0.2
after: try 164.3-165.7 instr/entry, control -0.14..+0.44
-> ~14-15 instructions/entry (~8%), controls stay noise-floor both arms.
`try_push_with_kind`'s remaining `CatchSavepoint::capture()` work and
`js_try_end`'s own (already tls_hot-fast) EXCEPTION_STATE resolution
are UNTOUCHED — js_try_end disassembles to ~28 near-minimal
instructions with nothing left to cut without threading a pointer
through codegen from push to end, which was evaluated and deferred as
materially riskier (touches try_stmt.rs's early-exit/closure/
generator/async call sites) for a smaller remaining win.
scripts/thread_local_cold_allowlist.json: only the
`shadow_stack.rs: 2 -> 1` line this change causes. `_hot_declarations`
is left untouched — verify() never reads it (only `files` is
enforced) — because a plain `--update` on unmodified main already
produces 466, not the committed 460: six pre-existing, unrelated
`perry_thread_local!` additions had drifted the count before this
change touched anything. Fixing that drift is not this PR's job.
Correctness: the shadow stack is the GC's precise root set, not pure
exception bookkeeping, so this was checked past "it compiles":
- Added test-files/test_gap_try_entry_shadow_hot_tls.ts (byte-identical
to node v26.5.1 --experimental-strip-types --no-warnings, checked
against both this change and the pristine parent arm): same-level
catch, a throw 4 frames deep, a throw crossing Array.prototype.map's
runtime trampoline, finally on both the normal and throwing paths,
nested try with an inner rethrow, a catch that itself throws, 25
levels of try/finally nesting with finally-order and try_depth
restored checked afterward, and a non-throwing control loop. Every
caught value is read back after GC-pressure allocation.
- Ran that fixture, plus the pre-existing
test_gap_gc_catch_param_rooting.ts, test_gap_try_savepoint_subsystems.ts,
test_gap_try_finally_no_catch_rethrow.ts and test_gap_try_setjmp_volatile.ts,
under PERRY_GC_SCHEDULE_SEED (5 seeds) + PERRY_GC_SCHEDULE_RATE=1 +
PERRY_GC_SCHEDULE_ALLOC_KB=0 + PERRY_GC_PROTECT_FROMSPACE=1 +
PERRY_GC_DIAG=1 (per CLAUDE.md's rooting-bug instruments) — a
collect-at-every-safepoint, evacuating, quarantine-and-mprotect
schedule. All 5 seeds matched node byte-for-byte; the diagnostic
output confirms the instrument was live, not vacuous
(forced_collections=2566, copying_minors=2566, moved_objects=37000,
fromspace retired_set up to #4 with bytes_protected growing) — no
stale-shadow-stack SIGSEGV, no output drift.
- `RUST_TEST_THREADS=1 cargo test --release -p perry-runtime --lib`:
4010 passed. The 2 failures
(gc::tests::copy_slot_decode::sabotaged_remembering_arm_is_refused_by_the_coverage_cross_check,
gc::tests::heap_generation::a_free_or_move_outside_every_scope_is_caught_in_debug_builds)
are pre-existing: reverted this change with `git checkout --` and
reran just those two on the pristine parent commit — identical
failures, same messages, confirming they assert debug_assert!-gated
behavior compiled out under --release, unrelated to this change.
- `cargo fmt --all -- --check`, `scripts/check_file_size.sh`,
`scripts/check_test_registration.py`,
`RUSTFLAGS="-D warnings" cargo check -p perry-runtime --all-targets`:
all clean.
#10619 routed SHADOW through perry_thread_local! unconditionally for an ~8% try-entry win, measured on Darwin. CI's cargo-test job (Linux, debug profile) SIGSEGV'd on it (run 35374727647, job 105594641738); the crash was never pinned to a specific test or line (stdout block-buffering ate the FAILED line), and it did not reproduce on macOS with the Darwin path, on macOS with the Darwin path forced off, or on a Linux x86_64 VM matching the CI runner's triple through 1968+ of ~4013 tests. tls_hot.rs's own docs say the hot-cache shortcut only pays for itself on Darwin aarch64; everywhere else it is strictly more work than the raw thread_local! it replaced (an extra HOT resolution plus a slot indirection), win or no SIGSEGV. So SHADOW's declaration is now cfg-split: perry_thread_local! only under all(target_vendor = "apple", target_arch = "aarch64", target_pointer_width = "64"), a plain thread_local! (the pre-#10619 form) everywhere else. This keeps the measured win where it was measured, removes the pessimization elsewhere, and removes CI's only exposure to the unconfirmed SIGSEGV mechanism. The open investigation is tracked in #10709. Re-verified: debug and release perry-runtime lib tests, GC-stress with live copying minors and from-space quarantine, the gap fixture byte-identical to node 26.5.1, and a fresh differential probe showing the try-entry win survives on Darwin (162.2 -> 152.0 instructions/ entry, ~6.3%, bare-loop control ~1 instruction in both arms).
Pushed the cfg-gated fix and read CI's own cargo-test job on the real failing runner: green (run 35433215970) where the unconditional perry_thread_local! swap was red (run 35374727647), same commit otherwise. That settles causation directly, superseding the local repro attempts (macOS both arms, qemu Linux) which all came back clean and were inconclusive on their own -- including a qemu-VM A/B whose two SIGKILLs were momentarily misread as a reproduced crash before being traced to an operator pkill -f self-match, not a fault. Also resolves why cargo-test stayed green on #10644/#10647/#10650/ #10651 against the same base: none of them touch shadow_stack.rs, and this PR was never merged to main, so their runs never contained the change at all. The internal mechanism inside tls_hot.rs's resolution path is still not understood; #10709 tracks that open half. This commit only updates the code comment and changelog to say plainly what is now confirmed versus what remains unknown.
dotenv.parse(Buffer) -- the idiomatic dotenv.parse(fs.readFileSync(...)) -- returned 0 keys, silently. dotenv.config() reported no error but populated neither result.parsed nor process.env: a silent total no-op. Real npm dotenv matches Node exactly. Per #10678 (duplicate extern "C" exports across perry-ext-*/perry-stdlib pairs), this binding existed twice: crates/perry-ext-dotenv/ (the governance-tracked crate) and crates/perry-stdlib/src/dotenv.rs (a second, independent implementation behind the default-on bundled-dotenv feature, exporting the same js_dotenv_config/js_dotenv_config_path/js_dotenv_parse symbols). Removed both, the 2-entry NativeModSig dispatch block + its dedicated regression test in native_table/utils_crypto.rs, the js_dotenv_* FFI declarations, the well_known_bindings.toml entry, the "dotenv"/"dotenv/config" NATIVE_MODULES entries + manifest rows, the bundled-dotenv stdlib feature, 3 Android stubs, and "dotenv" from PERRY_NATIVE_EXTENSION_PACKAGES (that array makes the module walker skip a node_modules/dotenv/ tree entirely; with the binding gone, dotenv's real source needs to reach the walker like any other npm package). Six tests edited (not deleted) to keep testing their real subject rather than a removed registry entry: binding_faithfulness.rs's lookup_preserves_registered_subpaths_before_falling_back kept its mysql2/promise half, dropped the dotenv/config half; well_known.rs dropped "dotenv" from shipped_unproven_bindings_are_partial's array (kept nanoid/uuid, separate PRs), deleted dotenv_is_registered, and retargeted node_prefix_stripped_on_lookup from "dotenv" to "bcrypt" (generic node:-prefix-stripping logic, not dotenv-specific); deleted dotenv_parse_is_registered (api-manifest) and dotenv_parse_dispatches_to_native_impl_as_an_object (utils_crypto.rs) -- both were regression guards for the exact rows removed above; trimmed the two dotenv/config-only side-effect-only-module allowlists in unimplemented_api_check.rs and manifest_consistency.rs to &[]. Also fixed tests/release/packages/next-app-route/provider/stdlib/Cargo.toml (a standalone workspace with its own Cargo.lock, not a member of the main workspace, so cargo check --workspace never touches it) which referenced the now-deleted bundled-dotenv feature. Regenerated docs/api/perry.d.ts, docs/src/api/reference.md, and docs/src/native-libraries/governance.md. Updated workspace-architecture.json (workspace_members 83->82, externalize 33->32).
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (29)
✨ 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge train 227 — two PRs validated together as one tree, released as v0.5.1606.
Trains land as their own PR, so the source PRs are closed, not merged, and their close-keywords never fire.
Contents
perf(runtime): give the shadow-stack root state the hot-TLS fast path, cfg-split to Darwin aarch64 (−6.3% try entry)refactor(stdlib): remove the dotenv native binding, compile the real package from source#10619 — held, then cleared on evidence
This PR originally caused a Linux-only debug-profile SIGSEGV in
cargo-test(#10709). It was held until re-scoped: the hot-TLS swap is now cfg-split to Darwin aarch64, and the exact CI job that was red is green.The mechanism is still unknown and #10709 tracks that. What justifies landing anyway:
tls_hot.rsstates the published-cache shortcut is Darwin-specific, and that elsewhere "resolving a thread-local is already a fixed offset and the extra cache indirection has no demonstrated benefit". Off-Darwin the swap cost a TLS access plus a slot indirection where a rawthread_local!is one fixed-offset access — it was a pessimization on those targets even without the crash.perry_thread_local!; a general defect in that path would have CI red across the board. It isn't. The fault is specific toSHADOW's usage, so gating narrows the reachable surface rather than concealing a shared bug.The win was re-measured honestly and the title corrected from −8%: 162.2 → 152.0 instructions per try entry (−6.3%), with a bare-loop control reading ~1.0 in both arms, against a base built from the PR's own parent.
Counts re-derived on the assembled tree
workspace_architecture.py --checkon this tree: 77 members / externalize=28 / keep=44, re-derived here rather than taken from the PR. For the record, because it caused a real error upstream: main is 78/29/44 — the 77/28/44 figure is this train's assembled value, and confusing the two sent a downstream rebase off by one crate before its own script caught it.Also green on the assembled tree: ledger 376 rows / 326 providers,
unrooted_local_shapeat 578, zeroperry-ext-dotenventries in the regenerated lock.Validation
Assembled on
91a566c8af; source heads asserted unchanged since assembly. Both PRs proven fully represented by patch-id and by subject+author — zero dropped.All nine cheap gates,
cargo check --workspace --all-targetsunder-D warnings, the release build of all five pinned artifacts, every unit suite, and a 7-area gap sweep (try,catch,error,throw,require,cjs,module) with zero unexplained regressions and every area asserted to have run a non-zero number of tests. Sweep run withPERRY_RUN_TIMEOUT=30rather than the harness default of 10s, so a loaded host cannot turn a pass into a timeout the harness would classify as a failure.lintcompleted its full 6-of-6 compile tier with nothing outside the known-red public-baseline step.Follow-up this train creates
shipped_unproven_bindings_are_partialinwell_known.rsiterates["dotenv", "nanoid"]. With dotenv gone it reads["nanoid"], and the pending nanoid removal would take it to[]— a test that iterates nothing, asserts nothing and reports green forever. That PR deletes the test rather than emptying it; its subject population (uuid, dotenv, nanoid) is now empty, andshipped_subset_bindings_are_partialstill covers undici, node-forge, lru-cache and qs.