fix(runtime): split function-metadata registration into copying and borrowing entry points (#9188) - #9705
Conversation
…orrowing entry points (PerryTS#9188) Registering function metadata runs once per function a bundle CONTAINS — 72,713 of them on the compiled claude-code TUI — so what one call costs is a startup cost every program pays whether or not it ever reads a name. The copy had already been removed by storing `(ptr, len)` and borrowing the program image, which is sound only if the bytes outlive the PROCESS. That is strictly stronger than the "outlives the call" these entry points published, and `js_register_function_name` / `js_register_function_source` are `#[no_mangle] pub extern "C"` symbols reachable from separately-loaded provider images and from FFI, so it is not a promise that can be imposed on callers that already exist. PerryTS#9188 was filed to make that a deliberate decision rather than a side effect of a perf commit. This is option 2 from the issue — split the entry points instead of retightening the contract: * `js_register_function_name` / `js_register_function_source` are back to their original contract: the bytes need only outlive the call, because the registry copies them. Every caller that is not codegen uses these. * `js_register_function_name_static` / `js_register_function_source_static` require process lifetime and store the borrowed slice. Codegen emits these, and only these, from `__perry_init_strings_<prefix>`, where the bytes are `@.str.N` `private unnamed_addr constant` globals in the image. All the volume is on the borrowing side, so the startup copy stays gone from the path that had it, and no published contract was tightened underneath a caller. Borrowed and owned bytes live in separate maps rather than one map of an enum: an enum value would add 8 bytes to every one of the ~60,000 borrowed entries to carry the handful of owned ones, which measured as a net loss (+0.31 MB). Owned entries take precedence on read, and the two locks are never held at the same time, so there is no acquisition order to get wrong. The registries move out of `formatting.rs` (26 lines under the 2,000-line cap) into a new `builtins/fn_metadata.rs`. The two copy tests are sabotage tests: they register from a heap buffer, overwrite it in place while it is still alive, and assert the registry still returns what was registered. Rewiring the copying entry points to borrow fails 3 of the 6 deterministically, instead of producing latent UB in a provider image. `codegen/emission_order_tests.rs`'s IR-text matchers were updated to the emitted spelling — left on the old name they would have matched nothing and passed vacuously. Both new symbols are added to `check_runtime_symbols.sh`, so a runtime archive predating the split fails there rather than at link time on a build worker. Claude-Session: https://claude.ai/code/session_01KL1tsB4oYnxRzF533NzHJF
|
Warning Review limit reachedNext included review available in 9 minutes. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (13)
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 |
cf0fd4d to
773f8f8
Compare
|
Holding this one — I think the The contract table reads The emission in The registries have no way to drop those entries: This is the exact hazard the PR is written to prevent — "outlives the call" vs "outlives the process" — with Two shapes of fix, your call:
I'd lean (1). Everything else in the PR looks right to me — restoring the published contract with a second symbol instead of tightening the existing one in place is the correct call, the separate borrowed/owned maps are well argued with the +0.31 MB measurement, and adding both spellings to |
|
Landed on |
1 participant
Summary
#9188 asked for a contract decision, not an optimisation:
js_register_function_name/js_register_function_sourcecould stop copying their bytes if the registries held(ptr, len)instead, but that turns "outlives the call" into "outlives the process" on two#[no_mangle] pub extern "C"symbols reachable from separately-loaded provider images and from FFI.By the time this was picked up, the copy had already been removed — by tightening those published contracts in place. This PR implements option 2 from the issue (its own recommendation) and puts the contract back where it was, without giving up the win:
js_register_function_name/js_register_function_sourceparseInt,isNaN, …), provider images, FFIjs_register_function_name_static/js_register_function_source_static__perry_init_strings_<prefix>, where they are@.str.Nprivate unnamed_addr constantglobalsAll the volume is on the borrowing side — 72,713 registrations on the compiled claude-code TUI, 5.1 MB of names and 23.8 MB of source text — so the startup copy stays gone from the path that had it, and no published contract is tightened underneath a caller.
Storage
Borrowed and owned bytes live in separate maps rather than one map of an enum: an enum value would add 8 bytes to every one of the ~60,000 borrowed entries to carry the handful of owned ones, which measured as a net loss (+0.31 MB) even though it removed 1.8 MB of copies. The name registry already had that shape (a small overrides map for names
register_function_name_if_absentinfers at run time); source text gains the matching pair. Owned entries win on read — an owned entry can only come from an explicit runtime registration, which is the more specific statement about that function — and the two locks are never held simultaneously, so there is no acquisition order to get wrong.The registries move out of
formatting.rs, which was 26 lines under the 2,000-line cap, into a newbuiltins/fn_metadata.rs.The tests are sabotage tests
copying_name_entry_point_owns_its_bytesandcopying_source_entry_point_owns_its_bytesregister from a heap buffer, overwrite that buffer in place while it is still alive, and assert the registry still returns what was registered. Deleting the copy makes them fail deterministically — verified by rewiring both copying entry points to borrow, which fails 3 of the 6 — instead of turning into latent UB in a provider image, which is the failure this split exists to prevent. The other three pin the two-map read rules, including thatError.stack's staleness check counts both maps.codegen/emission_order_tests.rs's IR-text matchers were moved to the emitted spelling: left on the old name they would have matched nothing and passed vacuously. Both new symbols are added toscripts/check_runtime_symbols.sh, so a runtime archive predating the split fails there instead of at link time on a build worker.Not done here: the issue's option 3 (have codegen skip source registration entirely when a whole-program analysis proves
Function.prototype.toStringis unreachable). The dynamic-access analysis is the hard part and is a separate change.Testing
cargo test -p perry-runtime --lib— 3,075 passed (RUST_TEST_THREADS=1)cargo test -p perry-codegen --lib— 1,400 passed;cargo test -p perry-hir --lib— 380 passedscripts/run_lint_gates.sh— all 62 script gates pass;cargo clippy --workspaceclean;cargo check --workspace --all-targetsunder-D warningsclean (the Linux-onlypthread_getattr_npredeclaration warnings reproduce on a cleanorigin/maincheckout and are not from this change)origin/mainover 110 function-metadata-relevant tests (function,tostring,inspect,closure,stack,_name,fn_), same host and same Node: identical verdicts on both armsconsole.log(fn),fn.name, class-method names,fn.toString()for both a declaration and an arrow, and computed-key name inference — all match NodeNo version bump.
Closes #9188
https://claude.ai/code/session_01KL1tsB4oYnxRzF533NzHJF