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.

Sign up for GitHub

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

Jump to bottom

lib: convert WeakMaps in cjs loader with symbol properties #52095

Merged
merged 1 commit into from
Apr 12, 2024
Merged

lib: convert WeakMaps in cjs loader with symbol properties #52095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
107 changes: 64 additions & 43 deletions lib/internal/modules/cjs/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ const {
ReflectSet,
RegExpPrototypeExec,
SafeMap,
SafeWeakMap,
String,
StringPrototypeCharAt,
StringPrototypeCharCodeAt,
Expand All @@ -62,25 +61,50 @@ const {
StringPrototypeStartsWith,
Symbol,
} = primordials;
const {
privateSymbols: {
module_source_private_symbol,
module_export_names_private_symbol,
module_circular_visited_private_symbol,
module_export_private_symbol,
module_parent_private_symbol,
},
} = internalBinding('util');

const { kEvaluated } = internalBinding('module_wrap');

// Map used to store CJS parsing data or for ESM loading.
const importedCJSCache = new SafeWeakMap();
// Internal properties for Module instances.
/**
* Cached {@link Module} source string.
*/
const kModuleSource = module_source_private_symbol;
/**
* Cached {@link Module} export names for ESM loader.
*/
const kModuleExportNames = module_export_names_private_symbol;
/**
* {@link Module} circular dependency visited flag.
*/
const kModuleCircularVisited = module_circular_visited_private_symbol;
/**
* Map of already-loaded CJS modules to use.
* {@link Module} export object snapshot for ESM loader.
*/
const cjsExportsCache = new SafeWeakMap();
const requiredESMSourceCache = new SafeWeakMap();
const kModuleExport = module_export_private_symbol;
/**
* {@link Module} parent module.
*/
const kModuleParent = module_parent_private_symbol;

const kIsMainSymbol = Symbol('kIsMainSymbol');
const kIsCachedByESMLoader = Symbol('kIsCachedByESMLoader');
const kRequiredModuleSymbol = Symbol('kRequiredModuleSymbol');
const kIsExecuting = Symbol('kIsExecuting');
// Set first due to cycle with ESM loader functions.
module.exports = {
cjsExportsCache,
importedCJSCache,
kModuleSource,
kModuleExport,
kModuleExportNames,
kModuleCircularVisited,
initializeCJS,
entryPointSource: undefined, // Set below.
Module,
Expand Down Expand Up @@ -257,8 +281,6 @@ function reportModuleNotFoundToWatchMode(basePath, extensions) {
}
}

/** @type {Map<Module, Module>} */
const moduleParentCache = new SafeWeakMap();
/**
* Create a new module instance.
* @param {string} id
Expand All @@ -268,7 +290,7 @@ function Module(id = '', parent) {
this.id = id;
this.path = path.dirname(id);
setOwnProperty(this, 'exports', {});
moduleParentCache.set(this, parent);
this[kModuleParent] = parent;
updateChildren(parent, this, false);
this.filename = null;
this.loaded = false;
Expand Down Expand Up @@ -356,17 +378,19 @@ ObjectDefineProperty(BuiltinModule.prototype, 'isPreloading', isPreloadingDesc);

/**
* Get the parent of the current module from our cache.
* @this {Module}
*/
function getModuleParent() {
return moduleParentCache.get(this);
return this[kModuleParent];
}

/**
* Set the parent of the current module in our cache.
* @this {Module}
* @param {Module} value
*/
function setModuleParent(value) {
moduleParentCache.set(this, value);
this[kModuleParent] = value;
}

let debug = require('internal/util/debuglog').debuglog('module', (fn) => {
Expand Down Expand Up @@ -955,7 +979,7 @@ function getExportsForCircularRequire(module) {
const requiredESM = module[kRequiredModuleSymbol];
if (requiredESM && requiredESM.getStatus() !== kEvaluated) {
let message = `Cannot require() ES Module ${module.id} in a cycle.`;
const parent = moduleParentCache.get(module);
const parent = module[kModuleParent];
if (parent) {
message += ` (from ${parent.filename})`;
}
Expand Down Expand Up @@ -1028,25 +1052,24 @@ Module._load = function(request, parent, isMain) {
const cachedModule = Module._cache[filename];
if (cachedModule !== undefined) {
updateChildren(parent, cachedModule, true);
if (!cachedModule.loaded) {
// If it's not cached by the ESM loader, the loading request
// comes from required CJS, and we can consider it a circular
// dependency when it's cached.
if (!cachedModule[kIsCachedByESMLoader]) {
return getExportsForCircularRequire(cachedModule);
}
// If it's cached by the ESM loader as a way to indirectly pass
// the module in to avoid creating it twice, the loading request
// come from imported CJS. In that case use the importedCJSCache
// to determine if it's loading or not.
const importedCJSMetadata = importedCJSCache.get(cachedModule);
if (importedCJSMetadata.loading) {
return getExportsForCircularRequire(cachedModule);
}
importedCJSMetadata.loading = true;
} else {
if (cachedModule.loaded) {
return cachedModule.exports;
}
// If it's not cached by the ESM loader, the loading request
// comes from required CJS, and we can consider it a circular
// dependency when it's cached.
if (!cachedModule[kIsCachedByESMLoader]) {
return getExportsForCircularRequire(cachedModule);
}
// If it's cached by the ESM loader as a way to indirectly pass
// the module in to avoid creating it twice, the loading request
// come from imported CJS. In that case use the kModuleCircularVisited
// to determine if it's loading or not.
if (cachedModule[kModuleCircularVisited]) {
return getExportsForCircularRequire(cachedModule);
}
// This is an ESM loader created cache entry, mark it as visited and fallthrough to loading the module.
cachedModule[kModuleCircularVisited] = true;
}

if (BuiltinModule.canBeRequiredWithoutScheme(filename)) {
Expand Down Expand Up @@ -1190,7 +1213,7 @@ Module._resolveFilename = function(request, parent, isMain, options) {
const requireStack = [];
for (let cursor = parent;
cursor;
cursor = moduleParentCache.get(cursor)) {
cursor = cursor[kModuleParent]) {
ArrayPrototypePush(requireStack, cursor.filename || cursor.id);
}
let message = `Cannot find module '${request}'`;
Expand Down Expand Up @@ -1268,9 +1291,7 @@ Module.prototype.load = function(filename) {
// Create module entry at load time to snapshot exports correctly
const exports = this.exports;
// Preemptively cache for ESM loader.
if (!cjsExportsCache.has(this)) {
cjsExportsCache.set(this, exports);
}
this[kModuleExport] = exports;
};

/**
Expand Down Expand Up @@ -1313,7 +1334,7 @@ function loadESMFromCJS(mod, filename) {
const isMain = mod[kIsMainSymbol];
// TODO(joyeecheung): we may want to invent optional special handling for default exports here.
// For now, it's good enough to be identical to what `import()` returns.
mod.exports = cascadedLoader.importSyncForRequire(mod, filename, source, isMain, moduleParentCache.get(mod));
mod.exports = cascadedLoader.importSyncForRequire(mod, filename, source, isMain, mod[kModuleParent]);
}

/**
Expand Down Expand Up @@ -1406,7 +1427,7 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
// Only modules being require()'d really need to avoid TLA.
if (loadAsESM) {
// Pass the source into the .mjs extension handler indirectly through the cache.
requiredESMSourceCache.set(this, content);
this[kModuleSource] = content;
loadESMFromCJS(this, filename);
return;
}
Expand Down Expand Up @@ -1467,15 +1488,15 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
* @returns {string}
*/
function getMaybeCachedSource(mod, filename) {
const cached = importedCJSCache.get(mod);
// If already analyzed the source, then it will be cached.
let content;
if (cached?.source) {
content = cached.source;
cached.source = undefined;
if (mod[kModuleSource] !== undefined) {
content = mod[kModuleSource];
mod[kModuleSource] = undefined;
} else {
// TODO(joyeecheung): we can read a buffer instead to speed up
// compilation.
content = requiredESMSourceCache.get(mod) ?? fs.readFileSync(filename, 'utf8');
content = fs.readFileSync(filename, 'utf8');
}
return content;
}
Expand All @@ -1499,7 +1520,7 @@ Module._extensions['.js'] = function(module, filename) {
}

// This is an error path because `require` of a `.js` file in a `"type": "module"` scope is not allowed.
const parent = moduleParentCache.get(module);
const parent = module[kModuleParent];
const parentPath = parent?.filename;
const packageJsonPath = path.resolve(pkg.path, 'package.json');
const usesEsm = containsModuleSyntax(content, filename);
Expand Down
21 changes: 10 additions & 11 deletions lib/internal/modules/esm/translators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,11 @@ const {
stripBOM,
} = require('internal/modules/helpers');
const {
cjsExportsCache,
importedCJSCache,
kIsCachedByESMLoader,
Module: CJSModule,
kModuleSource,
kModuleExport,
kModuleExportNames,
} = require('internal/modules/cjs/loader');
const { fileURLToPath, pathToFileURL, URL } = require('internal/url');
let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
Expand Down Expand Up @@ -285,9 +286,9 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) {
}

let exports;
if (cjsExportsCache.has(module)) {
exports = cjsExportsCache.get(module);
cjsExportsCache.delete(module);
if (module[kModuleExport] !== undefined) {
exports = module[kModuleExport];
module[kModuleExport] = undefined;
} else {
({ exports } = module);
}
Expand Down Expand Up @@ -366,18 +367,16 @@ translators.set('commonjs', async function commonjsStrategy(url, source,
function cjsPreparseModuleExports(filename, source) {
// TODO: Do we want to keep hitting the user mutable CJS loader here?
let module = CJSModule._cache[filename];
if (module) {
const cached = importedCJSCache.get(module);
if (cached) {
return { module, exportNames: cached.exportNames };
}
if (module && module[kModuleExportNames] !== undefined) {
return { module, exportNames: module[kModuleExportNames] };
}
const loaded = Boolean(module);
if (!loaded) {
module = new CJSModule(filename);
module.filename = filename;
module.paths = CJSModule._nodeModulePaths(module.path);
module[kIsCachedByESMLoader] = true;
module[kModuleSource] = source;
CJSModule._cache[filename] = module;
}

Expand All @@ -392,7 +391,7 @@ function cjsPreparseModuleExports(filename, source) {
const exportNames = new SafeSet(new SafeArrayIterator(exports));

// Set first for cycles.
importedCJSCache.set(module, { source, exportNames });
module[kModuleExportNames] = exportNames;

if (reexports.length) {
module.filename = filename;
Expand Down
5 changes: 5 additions & 0 deletions src/env_properties.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@
V(js_transferable_wrapper_private_symbol, "node:js_transferable_wrapper") \
V(entry_point_module_private_symbol, "node:entry_point_module") \
V(entry_point_promise_private_symbol, "node:entry_point_promise") \
V(module_source_private_symbol, "node:module_source") \
V(module_export_names_private_symbol, "node:module_export_names") \
V(module_circular_visited_private_symbol, "node:module_circular_visited") \
V(module_export_private_symbol, "node:module_export") \
V(module_parent_private_symbol, "node:module_parent") \
Copy link
Member

Choose a reason for hiding this comment

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

There are module-related symbols exported from node_contextify.cc too, should these come from there?

Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment, the convention is to declare private symbols in this per-isolate list because they are primitive values that can be shared across contexts. Moving them to files like node_contextify.cc works but it adds more boilerplate codes (https://github.com/nodejs/node/blob/main/src/node_util.cc#L281-L295).

Copy link
Member

@joyeecheung joyeecheung Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think there are any private symbols created in node_contextify.cc, but if there are, they should be moved here instead.

V(napi_type_tag, "node:napi:type_tag") \
V(napi_wrapper, "node:napi:wrapper") \
V(untransferable_object_private_symbol, "node:untransferableObject") \
Expand Down