Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: eliminate performance cost of ESM syntax detection for CommonJS entry points #52093

Merged
merged 16 commits into from
Mar 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions benchmark/misc/startup-core.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const bench = common.createBenchmark(main, {
script: [
'benchmark/fixtures/require-builtins',
'test/fixtures/semicolon',
'test/fixtures/snapshot/typescript',
],
mode: ['process', 'worker'],
count: [30],
Expand Down
12 changes: 10 additions & 2 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ module.exports = {
cjsExportsCache,
cjsSourceCache,
initializeCJS,
entryPointSource: undefined, // Set below.
Module,
wrapSafe,
};
Expand Down Expand Up @@ -1337,8 +1338,15 @@ function wrapSafe(filename, content, cjsModuleInstance, codeCache) {
return result;
} catch (err) {
if (process.mainModule === cjsModuleInstance) {
const { enrichCJSError } = require('internal/modules/esm/translators');
enrichCJSError(err, content, filename);
if (getOptionValue('--experimental-detect-module')) {
// For the main entry point, cache the source to potentially retry as ESM.
module.exports.entryPointSource = content;
} else {
// We only enrich the error (print a warning) if we're sure we're going to for-sure throw it; so if we're
// retrying as ESM, wait until we know whether we're going to retry before calling `enrichCJSError`.
const { enrichCJSError } = require('internal/modules/esm/translators');
enrichCJSError(err, content, filename);
}
}
throw err;
}
Expand Down
35 changes: 35 additions & 0 deletions lib/internal/modules/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@ const {
} = require('internal/errors').codes;
const { BuiltinModule } = require('internal/bootstrap/realm');

const {
shouldRetryAsESM: contextifyShouldRetryAsESM,
constants: {
syntaxDetectionErrors: {
esmSyntaxErrorMessages,
throwsOnlyInCommonJSErrorMessages,
},
},
} = internalBinding('contextify');
const { validateString } = require('internal/validators');
const fs = require('fs'); // Import all of `fs` so that it can be monkey-patched.
const internalFS = require('internal/fs/utils');
Expand Down Expand Up @@ -320,6 +329,31 @@ function normalizeReferrerURL(referrerName) {
}


GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
let esmSyntaxErrorMessagesSet; // Declared lazily in shouldRetryAsESM
let throwsOnlyInCommonJSErrorMessagesSet; // Declared lazily in shouldRetryAsESM
/**
* After an attempt to parse a module as CommonJS throws an error, should we try again as ESM?
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
* We only want to try again as ESM if the error is due to syntax that is only valid in ESM; and if the CommonJS parse
* throws on an error that would not have been a syntax error in ESM (like via top-level `await` or a lexical
* redeclaration of one of the CommonJS variables) then we need to parse again to see if it would have thrown in ESM.
* @param {string} errorMessage The string message thrown by V8 when attempting to parse as CommonJS
* @param {string} source Module contents
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
*/
function shouldRetryAsESM(errorMessage, source) {
esmSyntaxErrorMessagesSet ??= new SafeSet(esmSyntaxErrorMessages);
if (esmSyntaxErrorMessagesSet.has(errorMessage)) {
return true;
}

throwsOnlyInCommonJSErrorMessagesSet ??= new SafeSet(throwsOnlyInCommonJSErrorMessages);
if (throwsOnlyInCommonJSErrorMessagesSet.has(errorMessage)) {
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
return /** @type {boolean} */(contextifyShouldRetryAsESM(source));
}

return false;
}


// Whether we have started executing any user-provided CJS code.
// This is set right before we call the wrapped CJS code (not after,
// in case we are half-way in the execution when internals check this).
Expand All @@ -339,6 +373,7 @@ module.exports = {
loadBuiltinModule,
makeRequireFunction,
normalizeReferrerURL,
shouldRetryAsESM,
stripBOM,
toRealPath,
hasStartedUserCJSExecution() {
Expand Down
42 changes: 32 additions & 10 deletions lib/internal/modules/run_main.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ const {
globalThis,
} = primordials;

const { containsModuleSyntax } = internalBinding('contextify');
const { getNearestParentPackageJSONType } = internalBinding('modules');
const { getOptionValue } = require('internal/options');
const { checkPackageJSONIntegrity } = require('internal/modules/package_json_reader');
Expand Down Expand Up @@ -87,10 +86,6 @@ function shouldUseESMLoader(mainPath) {

// No package.json or no `type` field.
if (response === undefined || response[0] === 'none') {
if (getOptionValue('--experimental-detect-module')) {
// If the first argument of `containsModuleSyntax` is undefined, it will read `mainPath` from the file system.
return containsModuleSyntax(undefined, mainPath);
}
return false;
}

Expand Down Expand Up @@ -157,12 +152,43 @@ function runEntryPointWithESMLoader(callback) {
* by `require('module')`) even when the entry point is ESM.
* This monkey-patchable code is bypassed under `--experimental-default-type=module`.
* Because of backwards compatibility, this function is exposed publicly via `import { runMain } from 'node:module'`.
* When `--experimental-detect-module` is passed, this function will attempt to run ambiguous (no explicit extension, no
* `package.json` type field) entry points as CommonJS first; under certain conditions, it will retry running as ESM.
* @param {string} main - First positional CLI argument, such as `'entry.js'` from `node entry.js`
*/
function executeUserEntryPoint(main = process.argv[1]) {
const resolvedMain = resolveMainPath(main);
const useESMLoader = shouldUseESMLoader(resolvedMain);
if (useESMLoader) {

GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
// Unless we know we should use the ESM loader to handle the entry point per the checks in `shouldUseESMLoader`, first
// try to run the entry point via the CommonJS loader; and if that fails under certain conditions, retry as ESM.
let retryAsESM = false;
if (!useESMLoader) {
const cjsLoader = require('internal/modules/cjs/loader');
const { Module } = cjsLoader;
if (getOptionValue('--experimental-detect-module')) {
try {
// Module._load is the monkey-patchable CJS module loader.
Module._load(main, null, true);
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
} catch (error) {
const source = cjsLoader.entryPointSource;
const { shouldRetryAsESM } = require('internal/modules/helpers');
retryAsESM = shouldRetryAsESM(error.message, source);
// In case the entry point is a large file, such as a bundle,
// ensure no further references can prevent it being garbage-collected.
cjsLoader.entryPointSource = undefined;
if (!retryAsESM) {
const { enrichCJSError } = require('internal/modules/esm/translators');
enrichCJSError(error, source, resolvedMain);
throw error;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you solve the minor issue you were asking about where this re-throw alters the stack-trace?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I solved it for the purposes of this PR by putting the try/catch within the check for the detect-module flag. #52093 (comment) should solve it generally, for when we unflag this.

}
}
} else { // `--experimental-detect-module` is not passed
Module._load(main, null, true);
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
}
}

if (useESMLoader || retryAsESM) {
const mainPath = resolvedMain || main;
const mainURL = pathToFileURL(mainPath).href;

Expand All @@ -171,10 +197,6 @@ function executeUserEntryPoint(main = process.argv[1]) {
// even after the event loop stops running.
return cascadedLoader.import(mainURL, undefined, { __proto__: null }, true);
});
} else {
// Module._load is the monkey-patchable CJS module loader.
const { Module } = require('internal/modules/cjs/loader');
Module._load(main, null, true);
}
}

Expand Down
Loading
Loading