Merge train 210: CommonJS preamble cost cut by a third, copy-slot-hoists doc (v0.5.1588) - #10413
Merged
Merged
Conversation
Rebased onto v0.5.1580, which carries the #10356 fix (an un-imported export must not shadow a global intrinsic). That fix looks like a prerequisite: the preamble's error helper constructs new Error(...), OpenCode's graph exports Error from packages/core twice, so any path reaching that helper met a shadowed intrinsic and threw 'undefined is not a constructor'.
isBuiltin alone is stricter than the switch it replaced: sea, sqlite, test and
test/reporters are builtins only in their node: form. OpenCode's graph contains
a bare require of one - wrangler does DatabaseSync = __require("sqlite").
DatabaseSync then new DatabaseSync(...), which became new undefined() and threw
'undefined is not a constructor' at startup.
Two native calls reproduce the switch's semantics exactly while still removing
the ~120 interned string constants it emitted into every module. Whether Perry
should accept the bare spellings is a genuine question, but it is a semantic
one and does not belong in a performance change.
The module header claimed the copying minor hoists two per-parent facts, each with a sabotaged twin. Only the weak-holder fact landed; the old-generation fact was deliberately left out because no sabotage of it could be made to fail. Say so instead of claiming coverage the file does not have.
This was referenced Sep 17, 2026
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughChangesCommonJS preamble optimization
Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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
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 #10349 and #10402 as v0.5.1588, on
e6dcb6274d. Three source commits, each verified to preserve its patch-id and authorship.createRequireinstance per program instead of one per module.copy_slot_hoistsmodule doc. It claimed the copying minor hoists two facts per traced object; it hoists one. The old-generation fact is deliberately not hoisted, and the doc now records why: no sabotage of that hoist could be made to fail, because sticky dirty-page coverage carries an old→young edge independently of the remembered-set re-insertion the fact controls. A future hoist needs its own witness first.Train repair
#10349's changelog fragment was keyed to the wrong PR. It shipped as
changelog.d/10307-cjs-preamble-cost.md, but10307-global-alias-member-read.mdis already onmainfrom the real #10307 (train 200). Different slug, so no collision — but fragments are PR-keyed so release notes trace back to the change, and two unrelated PRs sharing a10307-prefix defeats that. Re-keyed to10349-.Validation
Validated head
a9efe708d5. Five-package release build pinned and hash-verified, and re-verified after the gap run so nothing rebuilt underneath the fixtures.main's one known runtime failure (gc::tests::heap_generation::a_free_or_move_outside_every_scope_is_caught_in_debug_builds).main.require,cjs,module,import,weak,builtin.cjsandbuiltinare clean —cjsbeing the most direct filter for perf(cjs): cut the per-module CommonJS preamble cost by a third #10349.Every red attributed
Six fixtures were A/B'd against
main's own artifact set, and all six behave identically on both arms (node=1, main=0, train=0) with distinct build stamps asserted per arm, so neither can have fallen through to the wrong archives:test_issue_1723_require_stdlib_subnamespace,test_issue_764_state_at_module_init,test_issue_915_native_module_after_async_resume,test_perry_gc_module,test_dynamic_import_data_10104,test_issue_2656_weakref_finalization_gc. Perry is correct on both arms; node is what fails.These were re-attributed from scratch rather than carried over from the previous train's identical verdicts. #10349 rewrites the per-module preamble, so every compiled program differs — a verdict measured on binaries that predate it says nothing about these. That distinction has already cost a re-run once in this queue.
The two remaining reds need no A/B:
test_requireis listed verbatim inrun_parity_tests.sh'sSKIP_TESTSonmain(crypto.randomUUID()differs), andtest_ramda_user_importis an ext-archive link refusal of the pinned five-package set, which is symmetric and environmental.Note that
--filteris a substring match over all fixtures, so these runs selecttest_issue_*/test_perry_*names that CI's gap suite (--filter test_gap_) never runs. Everytest_gap_*fixture in scope passed.Before merging, the pushed head and unchanged main are checked again. After merging, the rewritten commits are checked for preserved authorship and the main tree must match the validated train exactly.
Summary by CodeRabbit
Performance
Bug Fixes
node:-only module names.Maintenance