Merge train 216: statement-position inlines, implicit-this rooting, node:net/http value dispatch, i128 split-module literals, Node-API 9/10 hosting, dynamic-key reads (v0.5.1594) - #10610
Merged
Conversation
… the caller A call used as a statement (`f(x);`) whose inlined callee ended in an `if` containing `return` had that return spliced into the caller: the statement arm of inline_calls_in_stmts only looked for nested returns in take(len - 1) and only rewrote a bare trailing Return. The caller then returned the callee's value (decimal.js pow() returned true), and at module top level the stray ret produced invalid LLVM IR. The statement arm now removes every return structurally (discard_inlined_returns): `return e` becomes `e;`, and statements after an `if` that returns move into the branch that falls through to them. No loop wrapper is added. It declines (keeps the call) instead of duplicating or dropping statements, or when a return sits under a loop, switch, try or label. Two siblings of the same blind spot are fixed as well: the declined-inline fallback replaced the call statement with the setup hoisted out of its arguments (deleting the call), and the void-method expression inliner spliced statements that follow `return;`.
…restores it `js_native_call_method`'s prototype-override early path (#9247) bound the callee's `this` through a private `ImplicitThisScope` guard that kept the displaced value -- the caller's receiver -- in a plain struct field and wrote it back in `Drop`. The callee is user code; a copying minor inside it moves the caller's receiver, and the restore reinstalled the retired from-space address, so the caller's next `this.x` read `undefined` (#10490; cheerio 1.2.0's `_findBySelector`). The #9445 sweep rooted every `let prev = js_implicit_this_set(..)` but not the same save hidden in a guard's `Drop`, nor the two identical guards behind the Array.prototype callback engines (`DenseThisGuard`, 11 dense methods; `ThisGuard`, 9 `js_arraylike_*`). Replace the three private guards (and the already-rooted regex copy) with one shared `object::ImplicitThisScope<'scope>` that roots the displaced value in a borrowed `RuntimeHandleScope` and re-reads it in `Drop`. Also root the other displaced values the audit found held across user code: the accessor receiver override in the handle-method prototype walk, `new.target` in the Intl and Temporal subclass `super()` bridges, and ten stdlib save/restores (domain, events, process warnings, net, web streams, tls ALPNCallback, worker_threads).
…their providers
The value forms of net/http/https/http2 exports (a CommonJS require('net'),
an aliased module object, a destructured or pulled-out export, new on a bound
class value) reached a dispatcher that only an auto-optimized stdlib with
external-http-server-pump registered, so they returned undefined under
PERRY_NO_AUTO_OPTIMIZE=1 and in every net-only program. perry-ext-net and
perry-ext-http now own and register their export dispatchers from their
namespace installs and from the entry prologue, net classes construct through
the ctor registry, the net IP helpers are callable values, the provider
namespaces are cached so require('node:http') === require('http'), and
perry-ext-http registers its client handle-dispatch extension so erased
ClientRequest/IncomingMessage/Agent receivers work without the stdlib pump.
…e IR reader The in-process dialect reader, which builds every function of a split (multi-codegen-unit) module, materialized integer operands with `IntType::const_int(v as u64, v < 0)`. That API takes a single 64-bit word, so an `i128` operand kept only its low word. BigInt literals that fit in i128 lower to exactly such operands (`NativeRep::SmallBigInt`), so in a split module `1180591620717411303424n` (2^70) read back as `0n` and a 97-bit negative literal became a different 64-bit value, while single-unit builds (LLVM's own text parser) were correct. Widths above 64 bits now build both two's-complement words with `const_int_arbitrary_precision`, matching the assembler's semantics; the unsigned full-width spelling LLVM accepts is parsed too. Tests: a dialect unit test that builds every constant operand form codegen emits through the typed and the line paths and compares each against LLVM's own parse of the same text (only the i128 forms diverged); a unit test that lowers wide BigInt literals through the real emitter and checks the words passed to js_bigint_from_i128_parts; an integration test compiling the issue repro with PERRY_CODEGEN_UNITS=2 against Node's output; a gap test.
… as functions in napi_typeof The Node-API host rejected every addon declaring Node-API 9 or 10 (argon2 0.45, better-sqlite3 13, sharp 0.35) and exported none of the version 9/10 entry points. It now implements the stable surface through version 10: node_api_symbol_for, node_api_create_syntax_error/node_api_throw_syntax_error, node_api_get_module_file_name, node_api_create_property_key_*, node_api_create_external_string_* and node_api_create_buffer_from_arraybuffer. Node's per-module facts (declared version, module file URL) are attributed to the addon whose code runs, and exceptions left pending by async-work completions, TSFN callbacks and finalizers follow Node's uncaught-exception policy instead of leaking into the next call. napi_typeof kept its own tag ladder, which read an INT32-tagged class ref as a number and a class object as an object. It now classifies through the typeof operator's classifier, napi_create_int32/uint32 produce plain doubles so a small integer can no longer read as a class, the number getters reject class refs, and ToObject returns a class ref unchanged.
Every successful Node-API call rebuilt its napi_ok error-info message as a fresh CString, one allocation per call. The status bookkeeping now reuses the stored message while the status and message are unchanged. The native-callback trampoline switches the module attribution inside the environment borrows it already takes instead of borrowing twice more.
`o[k]` costs 674 instructions per access — in a loop, with a constant key, on a two-property object — against node's ~4.5. It does not amortize: every iteration pays the full lookup. `prop_read` is the second most common construct in real TypeScript at 258 per 1k lines across a 1.6M-line corpus, and dynamic-key access is how every map-like object and dispatch table reads. Profiled with symbols, three of the costs are the same shape as the array-push work: a helper re-deriving per access something already established. 1. `keys_find_slot_by_bytes` ran `clean_arr_ptr` — allocator-ownership plus forwarding classification — on `descriptor.keys`, 16.2% of the loop. That field is maintained BY THE COLLECTOR: when the keys array moves, `shapes::scan_shape_table_rekey_mut` writes the forwarded address back into every descriptor record in the family (`(*record).keys = addr as u64`), and a descriptor whose keys array died is pruned in the same pass. So the pointer read out of a live descriptor already IS the resolved live head. The resolved entry deliberately delegates back to the original for key_count >= KEYS_INDEX_THRESHOLD, where the indexed path owns its own receiver handling. 2. `try_data_get_bytes` asked `is_process_env_ptr` on every access, which reads a thread-local to answer "is this process.env?" — `_tlv_get_addr` was 14.1% of the loop with half of it from here. A sticky process-global latch means the thread-local is touched only once such an object exists. 3. `is_anon_shape_class_id` took an RwLock READ GUARD on every access — 16.1%, the largest single frame left after (1). A guard is an atomic read-modify-write on a lock word shared by every thread in the image, paid to consult a set written only from module init. It is now answered from a lock-free open-addressed mirror. 674.1 -> 546.0 instructions per access, -19.0%, both arms built from this commit's parent. Measured by differencing two probes that differ only in access count, inside each binary, so driver dispatch and code layout cancel before the arms are compared; the bare-loop control reads 0.06 and -0.11, and callback / `new` / object-literal controls are flat to two decimals. The mirror is sound because the set is INSERT-ONLY — the registrar only ever calls `insert`, nothing removes — so an entry once published stays valid for the life of the image and an open-addressed table needs no reclamation scheme. Slot 0 is the empty sentinel and the registrar rejects class id 0. An insert that exhausts its probe run sets an overflow flag, after which a miss no longer proves absence and falls back to the locked set. It lives in the class IMAGE, beside `parent_dense`, NOT in a process-global: `ANON_SHAPE_CLASS_IDS` is an `ImageTable` and every access resolves through `current()`, so a process-global mirror would answer one image's question out of another image's registrations. The process-global version measured -24.3% and was wrong; this one is -19.0% and is not. Two things that did NOT work, recorded so they are not retried: * A sticky "is the anon-shape set empty?" latch measured ZERO. Object literals ARE anon shapes, so the flag is true in essentially every real program and the lock was taken anyway. That is why this is a mirror and not a latch. * Caching the resolved slot against the key was considered and dropped: a cached key pointer is a heap pointer whose address can be recycled, which would produce a WRONG slot rather than a miss, and it would need its own GC root scanner. Removing the re-derivations gets most of it with none of that.
|
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 (98)
✨ 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 18, 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
1 participant
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.
This train lands #10530, #10564, #10565, #10566, #10569 and #10570 as v0.5.1594. Each source commit is verified to preserve its patch-id and authorship.
#10416) — a statement-position inline no longer returns out of the caller.thisin every guard that restores it (#10490) #10564 (#10490, partially) — the displaced implicitthisis rooted in every guard that restores it.#10428,#10429) —node:net/node:httpexports reached as values dispatch to their providers.#10545) — the split-module IR reader keeps the high word ofi128constants.#10456,#10461) — hosts Node-API 9/10 addons;napi_typeofreports class constructors as functions.Two changes this train adds on its own
run_parity_tests.sh: repeated--filternow intersects instead of last-wins (#10585).scripts/run_gap_tests.shwrapsrun_parity_tests.sh --filter test_gap_ "$@", sorun_gap_tests.sh --filter bufferarrived as two--filters and the second discarded the wrapper's own. The gap suite then selected every one of the 1672 fixtures matchingbufferrather than the 823test_gap_*fixturespr-gategates on — while still printing that it was running the gap suite. Measured on one merge-queue sweep: 94 of 192 fixtures (21m08s of 41m37s) were outside the gate's scope, and six of that sweep's seven reds were in that half, where a red cannot block a merge but still costs a full A/B to attribute. A single--filterand the no-filter full sweep are unchanged.object/native_module.rssplit for the 2000-line cap. The file sat at exactly 2000 lines, so #10565's five-line addition trippedcheck_file_size.sh. CJS default-export resolution moves toobject/native_module/cjs_default.rs, leaving 1942. The bodies are byte-identical and the parent re-exports the three names reached from elsewhere.That region was chosen for a reason worth recording: it carries no raw-handle debt sites.
raw_handle_debt.py --no-raise-vsis strictly per-path monotone with no notion of a relocation, so moving debt-carrying code out reads asceiling raised to N (was not listed at the merge base)even when the total is unchanged — the bare run then demands the emptied source line be deleted, and the two required invocations of the same gate contradict each other. Filed as #10583. All four of this file's recorded sites sit in the vtable-access block, so splitting elsewhere kept the ledger valid; that was luck, not a strategy, and the file now has ~58 lines of headroom.Validation
Validated head
17e9ad344f. Five-package release build pinned and hash-verified, and re-verified after the gap run (artifacts_match_pin_after_gap=True).issue_10545_split_unit_wide_bigint_literals— pass, fix(codegen): keep the high word of i128 constants in the split-module IR reader #10566's acceptance test.node_api_host_e2e— fails, and not because of this train.mainalready fails 4 of this suite's 5 tests withInvalid Mach-O symbol library ordinalwhile inspecting prebuilt.nodeaddons on this host. The train fails those same 4 plus its own NEW testnode_api_10_addons_match_node(added by fix(runtime): host Node-API 9/10 addons and report classes as functions in napi_typeof #10569, absent frommain) with a 4th error of the identical class. Regressions among tests that exist on both sides: none. fix(runtime): host Node-API 9/10 addons and report classes as functions in napi_typeof #10569 therefore cannot be verified locally; its CIe2e-scopedpassed (1h5m36s), which is the evidence that the hosting path works on a supported toolchain. Thatmainhas been 4/5 red here went unnoticed precisely because nothing ran the suite.lintis 82/83, the one red being the public-benchmark freshness step known-red onmain.--filterintersects — each line records what the run actually selected against whattest_gap_*matching predicts, so a filter that silently fails to narrow is a red):copy_slot_decode::sabotaged_remembering_arm_is_refused_by_the_coverage_cross_check(observable behind#[cfg(debug_assertions)]atcopying.rs:1033) andheap_generation::a_free_or_move_outside_every_scope_is_caught_in_debug_builds(main's long-known one).Reds attributed
The gap sweep produced no unexplained regressions.
#10564 lands partially, and stays open
This train carries #10564 at
d2ed8ef9e0— the rooting fix. After assembly the PR gained threemore commits (
8be1120beb, +218 lines) making the displaced implicit-this/new.targetexception-safe: an inner
js_throwcrossing a bare save/call/restore site leavesIMPLICIT_THISstuck at the inner value instead of the enclosing
try's baseline. That hole is pre-existing onmainand the rooting fix neither introduces nor fixes it, so landing the rooting half alone is astrict improvement rather than a partial regression.
It is held back for two reasons: the follow-up measures +7.83% instructions on a 5M-iteration
try/catch loop (895.28M → 965.34M, ~14 per
try-push), which is a compute trade needing theowner's decision rather than a train's; and the author records that none of the four named bare
sites is proven reachable. So the close keyword for #10490 is deliberately NOT carried here, #10564 is left
open, and its remaining three commits ride a later train.
Issues closed by this train
A merge train closes its source PRs rather than merging them, so the
Fixes #Nkeywords in those PR bodies never evaluate. They are carried here, on the PR that actually merges, so they fire:Fixes #10416
Fixes #10428
Fixes #10429
Fixes #10545
Fixes #10456
Fixes #10461
Fixes #10585
Summary by CodeRabbit
Bug Fixes
node:net,http,https, andhttp2exports when accessed through aliases, destructuring, or CommonJS module values.New Features
Documentation