chore: merge train 222 (v0.5.1601) - #10732
Merged
Merged
Conversation
…g per read globalThis.crypto.randomUUID/getRandomValues and crypto.subtle's KEM methods (encapsulateBits/decapsulateBits/encapsulateKey/decapsulateKey) allocated a fresh closure on every property read via plain js_closure_alloc, so the method had no stable identity (crypto.randomUUID === crypto.randomUUID was false) and every read allocated. Use the existing func-ptr-keyed js_closure_alloc_singleton cache instead, matching how other builtin methods stay identity-stable across reads.
Buffer.prototype had 36 bogus own properties (including bare string literals "function"/"undefined" that had drifted in from nearby prose/JS-idiom comments, plus DataView/Uint8Array.prototype/ Object.prototype methods that belong further up the prototype chain) and was missing 16 of Node's own members: the 14 internal <encoding>Slice/<encoding>Write methods and the deprecated offset/ parent accessors. Root cause: BUFFER_PROTOTYPE_METHODS (which populates Buffer.prototype's own enumerable properties) was generated from the SAME name table buffer_dispatch::is_buffer_method_name uses to decide whether a property read on a Buffer INSTANCE should synthesize a bound-method closure - deliberately broad, for duck-typed inherited-method reads - conflating that broad instance-read predicate with the narrow set of names that should be Buffer.prototype's own keys. Decoupled the two: curated BUFFER_PROTOTYPE_METHODS down to Node's real 96-name own-property surface (removing the 36 bogus entries, adding the 14 internal Slice/Write methods with real dispatch behavior, and adding offset/parent as accessor descriptors), while leaving is_buffer_method_name's broader instance-read predicate untouched. Also fixed a dormant gap the new ucs2Write method depends on: js_buffer_write_len's encoding match never handled tag 6 (utf16le/ ucs2), silently writing raw UTF-8 bytes instead - a pre-existing defect in buf.write(str, offset, 'utf16le') too.
Object.prototype had no own __proto__ accessor, so hasOwnProperty,
Object.hasOwn, getOwnPropertyNames, getOwnPropertyDescriptor,
Reflect.ownKeys, and the in operator all disagreed with Node about it.
Install a real { get, set, enumerable: false, configurable: true }
accessor descriptor on Object.prototype (gate-neutral, so no dynamic
property read/write fast path is affected), backed by the existing
js_object_get_prototype_of / Annex B legacy setPrototypeOf logic.
Also fixes a latent receiver-binding gap in
primitive_builtin_prototype_property's inherited-property fallback,
exposed by the new accessor: an accessor inherited transitively from
Object.prototype through a primitive's builtin wrapper prototype (e.g.
Number.prototype) was invoked with this bound to the intermediate
prototype object instead of the original primitive receiver.
…/SetValues when lifting a Symbol.iterator generator method A generator method keyed by [Symbol.iterator] is lifted to a top-level function with this as an explicit param (synthesize_symbol_iterator_wrapper in lower_decl/class_decl.rs), and replace_this_in_stmts rewrites Expr::This to that param throughout the body. Its expression walker was missing arms for GetIterator/GetAsyncIterator/MapEntries/SetValues -- the wrapper exprs a for-of iterable lowers to when it cannot be proven a plain Array/Map/Set (stmt_loops.rs lower_stmt_for_of_inner). A for-of over this.gen() inside such a method left an unreplaced Expr::This nested inside one of these wrappers, which evaluates to undefined outside any method body: Cannot read properties of undefined (reading 'gen').
…f/spread/Array.from Covers the #10445 repro (spread, for-of, Array.from, named-generator control), a two-level generator chain (iterator method's for-of iterable is itself another method that also iterates via this), a Symbol.iterator generator on a class EXPRESSION, and yield* delegation alongside a for-of over this.method() in the same generator.
… lane js_object_get_field_by_name's Proxy-receiver block and RuntimeHandleScope's raw-thread_local! fallback were both small enough to inline into the hot dynamic-key-read path. A thread_local! address resolution is readnone from LLVM's point of view, so once inlined, the optimizer hoisted the proxy registry's and the transient-handle root stack's TLS lookups out of their guards (is_proxy_id_band, a "size"-key check) and ran them unconditionally on every js_object_get_field_by_name call, Proxy or not. Splitting each into its own #[inline(never)] function keeps the optimizer from seeing inside at the call site, so nothing gets hoisted past the guard. Also skip try_read_gc_header's redundant is_plausible_heap_addr recheck in try_data_get_bytes's prototype-chain loop, where the caller already proved it true one statement above. Measured on a two-property-object o[k] loop (never touching a Proxy or a .size key): 544.9 -> 513.0 instructions/access (-5.9%), via (dyn80-dyn16)/64 differenced against 2x the iteration count to cancel per-process fixed overhead. The (loop80-loop16)/64 no-op control reads ~0 in both arms (base: -0.01..-0.04, mine: -0.02..0.13), confirming the technique resolves changes this small. Verified via disassembly that both _tlv_get_addr calls are gone from the function's prologue.
…red by a nested closure closure_local_inline's beta-reduction clones the arrow's return expression fresh per call site and substitutes each parameter with that call's argument via substitute_locals. For a parameter read inside a NESTED closure (e.g. (f, isOpt) => arr.forEach(([k,v]) => check(k, v, isOpt))), substitute_locals bakes a non-LocalGet argument straight into that nested closure's body and drops it from the closure's captures list, but never mints a fresh func_id for the rewritten closure literal. Codegen compiles exactly one body per func_id (whichever Expr::Closure occurrence its module-wide scan sees first), so every call site's clone of the nested closure keeps sharing the SAME func_id -- with more than one call site, only the first-seen clone's baked-in argument is ever compiled, and every other call silently runs it too. Bail out of the beta-reduction when any parameter is captured by a nested closure, leaving such an arrow as a real, per-call closure -- each invocation then creates its own closure instance whose nested callback correctly captures that call's argument by reference.
…ultiple call sites Covers the #10567 repro (nested destructuring forEach callback), an arrow with several params where the captured one is neither first nor last, nested arrows (outer -> middle -> inner, two closure boundaries away), an arrow declared inside a class method, a by-write capture control (a multi-statement arrow body, never a closure_local_inline candidate), and the plain-function controls from the original issue.
Building a replacement's output walks its pieces twice, measuring and then encoding, and each pass polled the GC safepoint once per piece. `try_fold` stops at QUANTUM units *or* at the end of a piece, and a piece is usually two or three units -- an original span, a template span, a capture -- so a subject with 200,000 matches ran the safepoint hundreds of thousands of times per pass for a handful of units of reading each. That check costs about 436 instructions: it evaluates the whole budgeted trigger ladder, which has no cheap "nothing is due" precheck. Both passes now poll once per POLL_UNITS units read. Unlike the collection loop in `perex_replace_direct`, these passes are downstream of the replacement's traced pieces and its replacer's strings, so they do produce garbage and polling far less often costs peak RSS. POLL_UNITS is therefore a measured trade, not a bound inherited from elsewhere: at `api::QUANTUM` (4096) the instruction win is the same but peak RSS is +13.2% median on an allocating replace at n=1,000,000, over the accepted +10% budget. At 512 the win survives and the cost does not. Instructions, both arms from one commit, release, plain main: replace, string template 27,837,140,955 -> 20,090,148,511 -27.8% replace1m (both forms) 275,168,155,979 -> 249,292,630,136 -9.4% replace, callback, ASCII 51,289,448,826 -> 47,903,744,797 -6.6% replace, callback, Unicode 61,275,226,072 -> 58,023,887,665 -5.3% Peak RSS on replace1m, nine interleaved rounds: median +0.3%, mean +0.1%, max -0.4%, against a +10% budget. Answers are identical to Node 26.5.1 on every probe, including the correctness differential from #10605. Why 512 rather than 4096: a piece is two or three units, so 512 still removes about 99 percent of the polls while giving the collector eight times the openings. Both figures above are measured; the knee between them is not located.
Native validator has 9+ methods throwing "not implemented", trim() silently returns undefined, and implemented checks (isEmail/isURL/isUUID/isJSON/ isEmpty) return 0/1 rather than real booleans (visible via JSON.stringify(validator.isEmail(...)) === "1", not "true"). Real npm validator matches Node for all 50 checks. Per #10678 (duplicate extern "C" exports across perry-ext-*/perry-stdlib pairs), this binding existed twice, plus a shared helper crate used only by the two duplicates: - crates/perry-ext-validator/ — the governance-tracked binding crate. - crates/perry-stdlib/src/validator.rs (425 lines) — a second, independent implementation behind the `bundled-validator` feature (default-on via the `validation` umbrella, itself in `full`), exporting the same js_validator_* symbols. - crates/perry-validation/ — "Shared borrowed string validators for Perry's bundled and extension bindings" (its own doc comment): a small email/URL/ UUID grammar helper consumed exclusively by the two crates above. With both gone, nothing references it, so it goes too. Removed all three crates, the 5-entry NativeModSig dispatch block in native_table/utils_crypto.rs (isEmail/isURL/isUUID/isJSON/isEmpty — the only validator methods with a dedicated codegen row; the rest were reachable only through the deleted FFI crates), the 16 js_validator_* FFI declarations in runtime_decls/stdlib_ffi/streams_events.rs, the well_known_bindings.toml entry, the NATIVE_MODULES/manifest rows, the validation/bundled-validator stdlib features (and "validation" from perry-stdlib's `full` feature list), and the 16 Android stub exports. 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->81, externalize 33->32, keep 45->44 — two crates removed, perry-ext-validator was "externalize" and perry-validation was "keep"/runtime-core).
…lib provider Standalone workspace (its own Cargo.lock, not a member of the main workspace), so cargo check --workspace never touched it. Referenced the now-deleted validation feature from perry-stdlib's Cargo.toml.
The rebase over origin/main (which already applied #10687's jsonwebtoken removal) resolved the manifest header-count conflict with placeholder values from before the rebase. Recompute them from the actual resolved tree via scripts/regen_api_docs.sh's two perry --print-api-manifest invocations: 2085 entries across 134 modules (perry.d.ts), 3027 entries across 136 modules (reference.md).
`cargo test --release -p perry-runtime --lib` fails on main with:
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
Both assert that a DEBUG-ONLY guard fires, and neither is cfg-gated, so both
are structurally incapable of passing under a release profile:
* `debug_assert_heap_change_open()` is `#[cfg(debug_assertions)]`. Under
--release it cannot panic, so `catch_unwind(...).is_err()` is false.
* copy_slot_decode's own doc comment already says it: "In a release build
`restore_surviving_dirty_coverage` would re-add the page the arm failed to
remember ... In the debug build `cargo test` runs, the same walk
cross-checks the dirty scan's per-slot re-remembering".
CI never sees this because its `cargo-test` job builds debug. It surfaces in
any release-profile run, which is how merge-train validation found it.
`#[cfg_attr(not(debug_assertions), ignore)]` rather than `#[cfg(debug_assertions)]`:
an ignored test is still reported by name in the release run, while a cfg'd-out
one is indistinguishable from a test that was deleted. The debug run -- the one
that can actually exercise these -- is unchanged.
|
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 (54)
✨ 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 was referenced Sep 19, 2026
This was referenced Sep 19, 2026
Closed
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 222 — eight PRs validated together as one tree, released as v0.5.1601.
Trains land as their own PR, so the source PRs are closed, not merged, and their close-keywords never fire. Every issue they resolve is listed at the bottom.
Contents
fix(runtime): cache Web Crypto method closures instead of reallocating per readfix(runtime): fixBuffer.prototype's own-property shapefix(runtime): materializeObject.prototype.__proto__as a real accessorfix(hir): replacethisinside for-of iterator wrapper exprs when lifting aSymbol.iteratorgeneratorperf(runtime): stop hoisting cold-path thread-locals intoo[k]'s fast lane (−5.8%)fix(transform): do not beta-reduce a local arrow whose param is captured by a nested closureperf(regex): poll the safepoint on units read, not on piecesrefactor(stdlib): remove the validator native binding, compile the real package from sourcePlus one commit of the train's own, explained below.
test(gc): two debug-only GC twins gated under a release profilecargo test --release -p perry-runtimefails two tests onmain, and has for some time:Both assert that a debug-only guard fires, and neither is cfg-gated, so neither can pass under a release profile.
debug_assert_heap_change_open()is#[cfg(debug_assertions)]; the other says so in its own doc comment ("In the debug buildcargo testruns…"). CI never sees this because itscargo-testjob builds debug.Gated with
#[cfg_attr(not(debug_assertions), ignore)]rather than#[cfg(debug_assertions)]: an ignored test is still reported by name, while a cfg'd-out one is indistinguishable from one that was deleted. The comment says explicitly that the tests are correct and the profile changed what the code means, and points at[profile.gcaudit]as the only profile giving release codegen with assertions live — so nobody later "fixes" the tests themselves.Effect visible in this train's own run:
runtime rc=0, where train 221 reportedrc=101.Validation
Assembled on
e1194660feand proven before validating. Every PR fully represented, checked by patch-id and by subject+author rather than by a pathspec diff; source heads asserted unchanged since assembly.Green:
fmt,file_size,raw_handle(self-test, ceilings, vs-main),gc_runtime_root_holders,check_test_registration,addr_class_inventory,cargo check --workspace --all-targetsunder-D warnings, the release build of all five pinned artifacts, every unit suite, both derived integration suites, and the 14-area gap sweep with each area asserted to have run a non-zero number of tests.lintcompleted its full compile tier (6 of 6 derived commands) with no failure outside the known-red public-baseline step. That assertion is new: train 221's lint was killed by an external SIGTERM after 4 of 6 and was recorded identically to a pass, because the step isexpect=Noneto tolerate the known-red baseline. The driver now parses the script's own banner for the derived count and requires the executed count to match.One caveat recorded in the run and repeated here: the gap sweep began while an unrelated 870-fixture sweep was still running (load 15 on 10 cores). No regressions appeared, so nothing needed re-verification — but had any appeared, the note in the results file requires them to be re-run serially before being believed.
Issues resolved
Closes #10427
Closes #10426
Closes #10482
Closes #10445
Closes #10567