Conversation
CompileFunctionForCJSLoader now compiles with kEagerCompile instead of kNoCompileOptions when there is no code cache to consume and the caller indicates the compiled script is an embedder-supplied main script (SEA or otherwise). Resolves a long-standing TODO about allowing optional eager compilation. This is scoped to embedderRunCjs's is_embedder flag: that function only ever compiles the one script an embedded process exists to run, unlike require()'d modules where much of the loaded code may never execute. is_embedder is a strict superset of the prior is_sea_main signal (SEA only ever runs through embedderRunCjs), so this also covers non-SEA embedder applications that were previously unaffected. kEagerCompile is mutually exclusive with kConsumeCodeCache at the V8 level, so the decision is made after cached_data is fully resolved (including the NODE_COMPILE_CACHE lookup), not before it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses review feedback: the trailing bool didn't self-document what it meant, unlike this file's convention for other unlabeled positional args. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses review feedback: ContainsModuleSyntax no longer needs to pass an explicit false now that the parameter defaults to it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Mirrors the existing "eagerly"/"lazily" debug line in BuiltinLoader::LookupAndCompile (node_builtins.cc), so NODE_DEBUG_NATIVE=CODE_CACHE now shows which strategy was used for CJS loader compiles too, not just builtins. Makes the eager path observable directly instead of only inferable from timing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Not part of this change — a throwaway script for comparing main vs this branch's SEA startup time on a real build. Will be removed in a follow-up commit before this PR is considered done. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…erge) Not part of this change. Reuses the binaries bench-branch.sh already built to check whether the eager-compile improvement holds at realistic (smaller) distinct-function counts, or is specific to the original 50,000-function test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Removes the CODE_CACHE debug line added during development (was useful for confirming the eager/lazy branch during validation, not needed in the final change). Adds a one-line comment on why is_embedder currently doubles as the eager-compile preference, so a future caller with different needs knows where to look. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Not part of this change — these were validation-only tooling used to compare main vs this branch on real hardware. Preserved on the bench-scripts-archive branch for reference. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. |
Author
Design
Testing
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66122 +/- ##
==========================================
- Coverage 90.29% 90.27% -0.03%
==========================================
Files 790 790
Lines 271689 271693 +4
Branches 51856 51858 +2
==========================================
- Hits 245312 245261 -51
- Misses 16878 16933 +55
Partials 9499 9499
🚀 New features to boost your workflow:
|
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.
CompileFunctionForCJSLoader has a long-standing TODO about allowing optional eager compilation for the ScriptCompiler::CompileFunction call. For the embedder-supplied main scripts (SEA or a generic embedder application): when there's no code cache to consume, compile with kEagerCompile instead of kNoCompileOptions, since we know in advance that most of an embedder main script's code is about to run — unlike an ordinary require()'d module, where much of it may never execute.
Measured against real builds of both main and this branch, using a synthetic CJS script with N top-level functions each invoked once, packaged as a SEA with no code cache:
The performance improvement scales with the number of distinct functions actually invoked — lazy compilation defers each function's real compile to its first call, eager pays that cost upfront instead. Peak memory was ~22.5% higher on the 50,000-function case, the expected trade-off for compiling more upfront.