src: simplify NativeModule caching and remove redundant data

- Remove `NativeModule._source` - the compilation is now entirely done in C++ and `process.binding('natives')` is implemented directly in the binding loader so there is no need to store additional source code strings. - Instead of using an object as `NativeModule._cached` and insert into it after compilation of each native module, simply prebuild a JS map filled with all the native modules and infer the state of compilation through `mod.loading`/`mod.loaded`. - Rename `NativeModule.nonInternalExists` to `NativeModule.canBeRequiredByUsers` and precompute that property for all the native modules during bootstrap instead of branching in every require call during runtime. This also fixes the bug where `worker_threads` can be made available with `--expose-internals`. - Rename `NativeModule.requireForDeps` to `NativeModule.requireWithFallbackInDeps`. - Add a test to make sure we do not accidentally leak any module to the global namespace. Backport-PR-URL: #25964 PR-URL: #25352 Reviewed-By: Anna Henningsen <[email protected]>
nodejs · Feb 9, 2019 · b280d90 · b280d90
1 parent 508a2e7
commit b280d90
Show file tree

Hide file tree

Showing 9 changed files with 199 additions and 101 deletions.
diff --git a/lib/internal/bootstrap/cache.js b/lib/internal/bootstrap/cache.js
@@ -7,14 +7,10 @@
 
 const { NativeModule } = require('internal/bootstrap/loaders');
 const {
-  source, getCodeCache, compileFunction
+  getCodeCache, compileFunction
 } = internalBinding('native_module');
 const { hasTracing, hasInspector } = process.binding('config');
 
-const depsModule = Object.keys(source).filter(
-  (key) => NativeModule.isDepsModule(key) || key.startsWith('internal/deps')
-);
-
 // Modules with source code compiled in js2c that
 // cannot be compiled with the code cache.
 const cannotUseCache = [
@@ -29,7 +25,7 @@ const cannotUseCache = [
   // the code cache is also used when compiling these two files.
   'internal/bootstrap/loaders',
   'internal/bootstrap/node'
-].concat(depsModule);
+];
 
 // Skip modules that cannot be required when they are not
 // built into the binary.
@@ -69,11 +65,19 @@ if (!process.versions.openssl) {
   );
 }
 
+const cachableBuiltins = [];
+for (const id of NativeModule.map.keys()) {
+  if (id.startsWith('internal/deps') ||
+      id.startsWith('v8/') || id.startsWith('node-inspect/')) {
+    cannotUseCache.push(id);
+  }
+  if (!cannotUseCache.includes(id)) {
+    cachableBuiltins.push(id);
+  }
+}
+
 module.exports = {
-  cachableBuiltins: Object.keys(source).filter(
-    (key) => !cannotUseCache.includes(key)
-  ),
-  getSource(id) { return source[id]; },
+  cachableBuiltins,
   getCodeCache,
   compileFunction,
   cannotUseCache

diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js
@@ -158,6 +158,12 @@ let internalBinding;
 // Create this WeakMap in js-land because V8 has no C++ API for WeakMap.
 internalBinding('module_wrap').callbackMap = new WeakMap();
 
+// Think of this as module.exports in this file even though it is not
+// written in CommonJS style.
+const loaderExports = { internalBinding, NativeModule };
+const loaderId = 'internal/bootstrap/loaders';
+const config = internalBinding('config');
+
 // Set up NativeModule.
 function NativeModule(id) {
   this.filename = `${id}.js`;
@@ -167,34 +173,35 @@ function NativeModule(id) {
   this.exportKeys = undefined;
   this.loaded = false;
   this.loading = false;
+  if (id === loaderId) {
+    // Do not expose this to user land even with --expose-internals.
+    this.canBeRequiredByUsers = false;
+  } else if (id.startsWith('internal/')) {
+    this.canBeRequiredByUsers = config.exposeInternals;
+  } else {
+    this.canBeRequiredByUsers = true;
+  }
 }
 
 const {
-  source,
+  moduleIds,
   compileFunction
 } = internalBinding('native_module');
 
-NativeModule._source = source;
-NativeModule._cache = {};
-
-const config = internalBinding('config');
-
-// Think of this as module.exports in this file even though it is not
-// written in CommonJS style.
-const loaderExports = { internalBinding, NativeModule };
-const loaderId = 'internal/bootstrap/loaders';
+NativeModule.map = new Map();
+for (var i = 0; i < moduleIds.length; ++i) {
+  const id = moduleIds[i];
+  const mod = new NativeModule(id);
+  NativeModule.map.set(id, mod);
+}
 
 NativeModule.require = function(id) {
   if (id === loaderId) {
     return loaderExports;
   }
 
-  const cached = NativeModule.getCached(id);
-  if (cached && (cached.loaded || cached.loading)) {
-    return cached.exports;
-  }
-
-  if (!NativeModule.exists(id)) {
+  const mod = NativeModule.map.get(id);
+  if (!mod) {
     // Model the error off the internal/errors.js model, but
     // do not use that module given that it could actually be
     // the one causing the error if there's a bug in Node.js.
@@ -205,62 +212,31 @@ NativeModule.require = function(id) {
     throw err;
   }
 
-  moduleLoadList.push(`NativeModule ${id}`);
-
-  const nativeModule = new NativeModule(id);
-
-  nativeModule.cache();
-  nativeModule.compile();
-
-  return nativeModule.exports;
-};
-
-NativeModule.isDepsModule = function(id) {
-  return id.startsWith('node-inspect/') || id.startsWith('v8/');
-};
-
-NativeModule.requireForDeps = function(id) {
-  if (!NativeModule.exists(id) ||
-      // TODO(TimothyGu): remove when DEP0084 reaches end of life.
-      NativeModule.isDepsModule(id)) {
-    id = `internal/deps/${id}`;
+  if (mod.loaded || mod.loading) {
+    return mod.exports;
   }
-  return NativeModule.require(id);
-};
 
-NativeModule.getCached = function(id) {
-  return NativeModule._cache[id];
+  moduleLoadList.push(`NativeModule ${id}`);
+  mod.compile();
+  return mod.exports;
 };
 
 NativeModule.exists = function(id) {
-  return NativeModule._source.hasOwnProperty(id);
+  return NativeModule.map.has(id);
 };
 
-if (config.exposeInternals) {
-  NativeModule.nonInternalExists = function(id) {
-    // Do not expose this to user land even with --expose-internals.
-    if (id === loaderId) {
-      return false;
-    }
-    return NativeModule.exists(id);
-  };
-
-  NativeModule.isInternal = function(id) {
-    // Do not expose this to user land even with --expose-internals.
-    return id === loaderId;
-  };
-} else {
-  NativeModule.nonInternalExists = function(id) {
-    return NativeModule.exists(id) && !NativeModule.isInternal(id);
-  };
-
-  NativeModule.isInternal = function(id) {
-    return id.startsWith('internal/');
-  };
-}
+NativeModule.canBeRequiredByUsers = function(id) {
+  const mod = NativeModule.map.get(id);
+  return mod && mod.canBeRequiredByUsers;
+};
 
-NativeModule.getSource = function(id) {
-  return NativeModule._source[id];
+// Allow internal modules from dependencies to require
+// other modules from dependencies by providing fallbacks.
+NativeModule.requireWithFallbackInDeps = function(request) {
+  if (!NativeModule.map.has(request)) {
+    request = `internal/deps/${request}`;
+  }
+  return NativeModule.require(request);
 };
 
 const getOwn = (target, property, receiver) => {
@@ -334,13 +310,13 @@ NativeModule.prototype.compile = function() {
 
   try {
     const requireFn = this.id.startsWith('internal/deps/') ?
-      NativeModule.requireForDeps :
+      NativeModule.requireWithFallbackInDeps :
       NativeModule.require;
 
     const fn = compileFunction(id);
     fn(this.exports, requireFn, this, process, internalBinding);
 
-    if (config.experimentalModules && !NativeModule.isInternal(this.id)) {
+    if (config.experimentalModules && this.canBeRequiredByUsers) {
       this.proxifyExports();
     }
 
@@ -350,10 +326,6 @@ NativeModule.prototype.compile = function() {
   }
 };
 
-NativeModule.prototype.cache = function() {
-  NativeModule._cache[this.id] = this;
-};
-
 // Coverage must be turned on early, so that we can collect
 // it for Node.js' own internal libraries.
 if (process.env.NODE_V8_COVERAGE) {

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -113,8 +113,12 @@ function Module(id, parent) {
   this.children = [];
 }
 
-const builtinModules = Object.keys(NativeModule._source)
-  .filter(NativeModule.nonInternalExists);
+const builtinModules = [];
+for (const [id, mod] of NativeModule.map) {
+  if (mod.canBeRequiredByUsers) {
+    builtinModules.push(id);
+  }
+}
 
 Object.freeze(builtinModules);
 Module.builtinModules = builtinModules;
@@ -423,7 +427,7 @@ if (isWindows) {
 var indexChars = [ 105, 110, 100, 101, 120, 46 ];
 var indexLen = indexChars.length;
 Module._resolveLookupPaths = function(request, parent, newReturn) {
-  if (NativeModule.nonInternalExists(request)) {
+  if (NativeModule.canBeRequiredByUsers(request)) {
     debug('looking for %j in []', request);
     return (newReturn ? null : [request, []]);
   }
@@ -540,7 +544,7 @@ Module._load = function(request, parent, isMain) {
     return cachedModule.exports;
   }
 
-  if (NativeModule.nonInternalExists(filename)) {
+  if (NativeModule.canBeRequiredByUsers(filename)) {
     debug('load native module %s', request);
     return NativeModule.require(filename);
   }
@@ -573,7 +577,7 @@ function tryModuleLoad(module, filename) {
 }
 
 Module._resolveFilename = function(request, parent, isMain, options) {
-  if (NativeModule.nonInternalExists(request)) {
+  if (NativeModule.canBeRequiredByUsers(request)) {
     return request;
   }
 

diff --git a/lib/internal/modules/esm/default_resolve.js b/lib/internal/modules/esm/default_resolve.js
@@ -53,7 +53,7 @@ const extensionFormatMap = {
 };
 
 function resolve(specifier, parentURL) {
-  if (NativeModule.nonInternalExists(specifier)) {
+  if (NativeModule.canBeRequiredByUsers(specifier)) {
     return {
       url: specifier,
       format: 'builtin'

diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
@@ -81,7 +81,7 @@ translators.set('builtin', async (url) => {
   // slice 'node:' scheme
   const id = url.slice(5);
   NativeModule.require(id);
-  const module = NativeModule.getCached(id);
+  const module = NativeModule.map.get(id);
   return createDynamicModule(
     [...module.exportKeys, 'default'], url, (reflect) => {
       debug(`Loading BuiltinModule ${url}`);

diff --git a/src/node_native_module.cc b/src/node_native_module.cc
@@ -83,11 +83,21 @@ void NativeModuleLoader::GetCacheUsage(
   args.GetReturnValue().Set(result);
 }
 
-void NativeModuleLoader::SourceObjectGetter(
+void NativeModuleLoader::ModuleIdsGetter(
     Local<Name> property, const PropertyCallbackInfo<Value>& info) {
   Local<Context> context = info.GetIsolate()->GetCurrentContext();
-  info.GetReturnValue().Set(
-      per_process::native_module_loader.GetSourceObject(context));
+  Isolate* isolate = info.GetIsolate();
+
+  const NativeModuleRecordMap& source_ =
+      per_process::native_module_loader.source_;
+  std::vector<Local<Value>> ids;
+  ids.reserve(source_.size());
+
+  for (auto const& x : source_) {
+    ids.push_back(OneByteString(isolate, x.first.c_str(), x.first.size()));
+  }
+
+  info.GetReturnValue().Set(Array::New(isolate, ids.data(), ids.size()));
 }
 
 void NativeModuleLoader::ConfigStringGetter(
@@ -306,8 +316,8 @@ void NativeModuleLoader::Initialize(Local<Object> target,
             .FromJust());
   CHECK(target
             ->SetAccessor(env->context(),
-                          env->source_string(),
-                          SourceObjectGetter,
+                          FIXED_ONE_BYTE_STRING(env->isolate(), "moduleIds"),
+                          ModuleIdsGetter,
                           nullptr,
                           MaybeLocal<Value>(),
                           DEFAULT,

diff --git a/src/node_native_module.h b/src/node_native_module.h
@@ -58,11 +58,10 @@ class NativeModuleLoader {
 
  private:
   static void GetCacheUsage(const v8::FunctionCallbackInfo<v8::Value>& args);
-  // Passing map of builtin module source code into JS land as
-  // internalBinding('native_module').source
-  static void SourceObjectGetter(
-      v8::Local<v8::Name> property,
-      const v8::PropertyCallbackInfo<v8::Value>& info);
+  // Passing ids of builtin module source code into JS land as
+  // internalBinding('native_module').moduleIds
+  static void ModuleIdsGetter(v8::Local<v8::Name> property,
+                              const v8::PropertyCallbackInfo<v8::Value>& info);
   // Passing config.gypi into JS land as internalBinding('native_module').config
   static void ConfigStringGetter(
       v8::Local<v8::Name> property,

diff --git a/test/code-cache/test-code-cache.js b/test/code-cache/test-code-cache.js
@@ -5,15 +5,12 @@
 // and the cache is used when built in modules are compiled.
 // Otherwise, verifies that no cache is used when compiling builtins.
 
-require('../common');
+const { isMainThread } = require('../common');
 const assert = require('assert');
 const {
   cachableBuiltins,
   cannotUseCache
 } = require('internal/bootstrap/cache');
-const {
-  isMainThread
-} = require('worker_threads');
 
 const {
   internalBinding