Skip to content

Commit

Permalink
module: do less CJS module loader initialization at run time
Browse files Browse the repository at this point in the history 
This patch:

- Builds the set of modules that can be required by users with/without
  the `node:` prefix at snapshot building time. We only modify it when
  `--expose-internals` but the default set is now in the snapshot. At
  run time the CJS module loader only creates a frozen array out of it.
- `BuiltinModule.canBeRequiredWithoutScheme()` is now enough to
  determine if an id can be required without `node:` without an
  additional call to `BuiltinModule.canBeRequiredByUsers()`
- Replace the pending-to-deprecate methods on `Module` with an internal
  implementation that only queries the CLI flags when being invoked.
  So we can install these methods in the snapshot.

PR-URL: nodejs/node#47194
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
  • Loading branch information
@sercher
sercher committed Apr 24, 2024
1 parent 110bd4b commit 6e13d32
Show file tree
Hide file tree
Showing 8 changed files with 143 additions and 93 deletions.
59 changes: 47 additions & 12 deletions graal-nodejs/lib/internal/bootstrap/realm.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,15 @@ const legacyWrapperList = new SafeSet([
'util',
]);

// The code bellow assumes that the two lists must not contain any modules
// beginning with "internal/".
// Modules that can only be imported via the node: scheme.
const schemelessBlockList = new SafeSet([
'test',
'test/reporters',
]);
// Modules that will only be enabled at run time.
const experimentalModuleList = new SafeSet();

// Set up process.binding() and process._linkedBinding().
{
Expand Down Expand Up @@ -199,6 +203,20 @@ const getOwn = (target, property, receiver) => {
undefined;
};

const publicBuiltinIds = builtinIds
.filter((id) =>
!StringPrototypeStartsWith(id, 'internal/') &&
!experimentalModuleList.has(id),
);
// Do not expose the loaders to user land even with --expose-internals.
const internalBuiltinIds = builtinIds
.filter((id) => StringPrototypeStartsWith(id, 'internal/') && id !== selfId);

// When --expose-internals is on we'll add the internal builtin ids to these.
const canBeRequiredByUsersList = new SafeSet(publicBuiltinIds);
const canBeRequiredByUsersWithoutSchemeList =
new SafeSet(publicBuiltinIds.filter((id) => !schemelessBlockList.has(id)));

/**
* An internal abstraction for the built-in JavaScript modules of Node.js.
* Be careful not to expose this to user land unless --expose-internals is
Expand All @@ -216,7 +234,6 @@ class BuiltinModule {
constructor(id) {
this.filename = `${id}.js`;
this.id = id;
this.canBeRequiredByUsers = !StringPrototypeStartsWith(id, 'internal/');

// The CJS exports object of the module.
this.exports = {};
Expand All @@ -238,14 +255,23 @@ class BuiltinModule {
this.exportKeys = undefined;
}

static allowRequireByUsers(id) {
if (id === selfId) {
// No code because this is an assertion against bugs.
// eslint-disable-next-line no-restricted-syntax
throw new Error(`Should not allow ${id}`);
}
canBeRequiredByUsersList.add(id);
if (!schemelessBlockList.has(id)) {
canBeRequiredByUsersWithoutSchemeList.add(id);
}
}

// To be called during pre-execution when --expose-internals is on.
// Enables the user-land module loader to access internal modules.
static exposeInternals() {
for (const { 0: id, 1: mod } of BuiltinModule.map) {
// Do not expose this to user land even with --expose-internals.
if (id !== selfId) {
mod.canBeRequiredByUsers = true;
}
for (let i = 0; i < internalBuiltinIds.length; ++i) {
BuiltinModule.allowRequireByUsers(internalBuiltinIds[i]);
}
}

Expand All @@ -254,14 +280,23 @@ class BuiltinModule {
}

static canBeRequiredByUsers(id) {
const mod = BuiltinModule.map.get(id);
return mod && mod.canBeRequiredByUsers;
return canBeRequiredByUsersList.has(id);
}

// Determine if a core module can be loaded without the node: prefix. This
// function does not validate if the module actually exists.
static canBeRequiredWithoutScheme(id) {
return !schemelessBlockList.has(id);
return canBeRequiredByUsersWithoutSchemeList.has(id);
}

static isBuiltin(id) {
return BuiltinModule.canBeRequiredWithoutScheme(id) || (
typeof id === 'string' &&
StringPrototypeStartsWith(id, 'node:') &&
BuiltinModule.canBeRequiredByUsers(StringPrototypeSlice(id, 5))
);
}

static getCanBeRequiredByUsersWithoutSchemeList() {
return ArrayFrom(canBeRequiredByUsersWithoutSchemeList);
}

static getSchemeOnlyModuleNames() {
Expand All @@ -270,7 +305,7 @@ class BuiltinModule {

// Used by user-land module loaders to compile and load builtins.
compileForPublicLoader() {
if (!this.canBeRequiredByUsers) {
if (!BuiltinModule.canBeRequiredByUsers(this.id)) {
// No code because this is an assertion against bugs
// eslint-disable-next-line no-restricted-syntax
throw new Error(`Should not compile ${this.id} for public use`);
Expand Down
75 changes: 27 additions & 48 deletions graal-nodejs/lib/internal/modules/cjs/loader.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ const {
ArrayPrototypeSplice,
ArrayPrototypeUnshift,
ArrayPrototypeUnshiftApply,
ArrayPrototypeFlatMap,
Boolean,
Error,
JSONParse,
Expand All @@ -52,7 +51,6 @@ const {
ReflectSet,
RegExpPrototypeExec,
SafeMap,
SafeSet,
SafeWeakMap,
String,
StringPrototypeCharAt,
Expand Down Expand Up @@ -82,7 +80,7 @@ const {
} = require('internal/source_map/source_map_cache');
const { pathToFileURL, fileURLToPath, isURL } = require('internal/url');
const {
deprecate,
pendingDeprecate,
emitExperimentalWarning,
kEmptyObject,
filterOwnProperties,
Expand Down Expand Up @@ -310,44 +308,29 @@ let debug = require('internal/util/debuglog').debuglog('module', (fn) => {
debug = fn;
});

const builtinModules = [];
ObjectDefineProperty(Module.prototype, 'parent', {
__proto__: null,
get: pendingDeprecate(
getModuleParent,
'module.parent is deprecated due to accuracy issues. Please use ' +
'require.main to find program entry point instead.',
'DEP0144',
),
set: pendingDeprecate(
setModuleParent,
'module.parent is deprecated due to accuracy issues. Please use ' +
'require.main to find program entry point instead.',
'DEP0144',
),
});
Module._debug = pendingDeprecate(debug, 'Module._debug is deprecated.', 'DEP0077');
Module.isBuiltin = BuiltinModule.isBuiltin;

// This function is called during pre-execution, before any user code is run.
function initializeCJS() {
const pendingDeprecation = getOptionValue('--pending-deprecation');
ObjectDefineProperty(Module.prototype, 'parent', {
__proto__: null,
get: pendingDeprecation ? deprecate(
getModuleParent,
'module.parent is deprecated due to accuracy issues. Please use ' +
'require.main to find program entry point instead.',
'DEP0144',
) : getModuleParent,
set: pendingDeprecation ? deprecate(
setModuleParent,
'module.parent is deprecated due to accuracy issues. Please use ' +
'require.main to find program entry point instead.',
'DEP0144',
) : setModuleParent,
});
Module._debug = deprecate(debug, 'Module._debug is deprecated.', 'DEP0077');

for (const { 0: id, 1: mod } of BuiltinModule.map) {
if (mod.canBeRequiredByUsers &&
BuiltinModule.canBeRequiredWithoutScheme(id)) {
ArrayPrototypePush(builtinModules, id);
}
}

const allBuiltins = new SafeSet(
ArrayPrototypeFlatMap(builtinModules, (bm) => [bm, `node:${bm}`]),
);
BuiltinModule.getSchemeOnlyModuleNames().forEach((builtin) => allBuiltins.add(`node:${builtin}`));
ObjectFreeze(builtinModules);
Module.builtinModules = builtinModules;

Module.isBuiltin = function isBuiltin(moduleName) {
return allBuiltins.has(moduleName);
};
// This need to be done at runtime in case --expose-internals is set.
const builtinModules = BuiltinModule.getCanBeRequiredByUsersWithoutSchemeList();
Module.builtinModules = ObjectFreeze(builtinModules);

initializeCjsConditions();

Expand Down Expand Up @@ -803,7 +786,6 @@ Module._resolveLookupPaths = function(request, parent) {
StringPrototypeStartsWith(request, 'node:') &&
BuiltinModule.canBeRequiredByUsers(StringPrototypeSlice(request, 5))
) || (
BuiltinModule.canBeRequiredByUsers(request) &&
BuiltinModule.canBeRequiredWithoutScheme(request)
)) {
debug('looking for %j in []', request);
Expand Down Expand Up @@ -925,11 +907,11 @@ Module._load = function(request, parent, isMain) {
// Slice 'node:' prefix
const id = StringPrototypeSlice(request, 5);

const module = loadBuiltinModule(id, request);
if (!module?.canBeRequiredByUsers) {
if (!BuiltinModule.canBeRequiredByUsers(id)) {
throw new ERR_UNKNOWN_BUILTIN_MODULE(request);
}

const module = loadBuiltinModule(id, request);
return module.exports;
}

Expand All @@ -947,9 +929,8 @@ Module._load = function(request, parent, isMain) {
}
}

const mod = loadBuiltinModule(filename, request);
if (mod?.canBeRequiredByUsers &&
BuiltinModule.canBeRequiredWithoutScheme(filename)) {
if (BuiltinModule.canBeRequiredWithoutScheme(filename)) {
const mod = loadBuiltinModule(filename, request);
return mod.exports;
}

Expand Down Expand Up @@ -1003,7 +984,6 @@ Module._resolveFilename = function(request, parent, isMain, options) {
StringPrototypeStartsWith(request, 'node:') &&
BuiltinModule.canBeRequiredByUsers(StringPrototypeSlice(request, 5))
) || (
BuiltinModule.canBeRequiredByUsers(request) &&
BuiltinModule.canBeRequiredWithoutScheme(request)
)
) {
Expand Down Expand Up @@ -1459,8 +1439,7 @@ Module._preloadModules = function(requests) {

Module.syncBuiltinESMExports = function syncBuiltinESMExports() {
for (const mod of BuiltinModule.map.values()) {
if (mod.canBeRequiredByUsers &&
BuiltinModule.canBeRequiredWithoutScheme(mod.id)) {
if (BuiltinModule.canBeRequiredWithoutScheme(mod.id)) {
mod.syncExports();
}
}
Expand Down
3 changes: 1 addition & 2 deletions graal-nodejs/lib/internal/modules/esm/hooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,7 @@ class Hooks {
globalThis,
// Param getBuiltin
(builtinName) => {
if (BuiltinModule.canBeRequiredByUsers(builtinName) &&
BuiltinModule.canBeRequiredWithoutScheme(builtinName)) {
if (BuiltinModule.canBeRequiredWithoutScheme(builtinName)) {
return require(builtinName);
}
throw new ERR_INVALID_ARG_VALUE('builtinName', builtinName);
Expand Down
6 changes: 2 additions & 4 deletions graal-nodejs/lib/internal/modules/esm/resolve.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -803,8 +803,7 @@ function parsePackageName(specifier, base) {
* @returns {resolved: URL, format? : string}
*/
function packageResolve(specifier, base, conditions) {
if (BuiltinModule.canBeRequiredByUsers(specifier) &&
BuiltinModule.canBeRequiredWithoutScheme(specifier)) {
if (BuiltinModule.canBeRequiredWithoutScheme(specifier)) {
return new URL('node:' + specifier);
}

Expand Down Expand Up @@ -990,8 +989,7 @@ function checkIfDisallowedImport(specifier, parsed, parsedParentURL) {

return { url: parsed.href };
}
if (BuiltinModule.canBeRequiredByUsers(specifier) &&
BuiltinModule.canBeRequiredWithoutScheme(specifier)) {
if (BuiltinModule.canBeRequiredWithoutScheme(specifier)) {
throw new ERR_NETWORK_IMPORT_DISALLOWED(
specifier,
parsedParentURL,
Expand Down
18 changes: 10 additions & 8 deletions graal-nodejs/lib/internal/modules/helpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,14 @@ function getCjsConditions() {
}

function loadBuiltinModule(filename, request) {
const mod = BuiltinModule.map.get(filename);
if (mod?.canBeRequiredByUsers) {
debug('load built-in module %s', request);
// compileForPublicLoader() throws if mod.canBeRequiredByUsers is false:
mod.compileForPublicLoader();
return mod;
if (!BuiltinModule.canBeRequiredByUsers(filename)) {
return;
}
const mod = BuiltinModule.map.get(filename);
debug('load built-in module %s', request);
// compileForPublicLoader() throws if canBeRequiredByUsers is false:
mod.compileForPublicLoader();
return mod;
}

let $Module = null;
Expand Down Expand Up @@ -99,8 +100,9 @@ function makeRequireFunction(mod, redirects) {
const { href, protocol } = destination;
if (protocol === 'node:') {
const specifier = destination.pathname;
const mod = loadBuiltinModule(specifier, href);
if (mod && mod.canBeRequiredByUsers) {

if (BuiltinModule.canBeRequiredByUsers(specifier)) {
const mod = loadBuiltinModule(specifier, href);
return mod.exports;
}
throw new ERR_UNKNOWN_BUILTIN_MODULE(specifier);
Expand Down
4 changes: 2 additions & 2 deletions graal-nodejs/lib/internal/process/warning.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,8 @@ function emitWarning(warning, type, code, ctor) {
process.nextTick(doEmitWarning, warning);
}

function emitWarningSync(warning) {
process.emit('warning', createWarningObject(warning));
function emitWarningSync(warning, type, code, ctor) {
process.emit('warning', createWarningObject(warning, type, code, ctor));
}

function createWarningObject(warning, type, code, ctor, detail) {
Expand Down
Loading

0 comments on commit 6e13d32

Please sign in to comment.