From 6291c107d08569aea32831d1dc2b14ededa145ca Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 5 Oct 2023 02:11:04 +0200 Subject: [PATCH] vm: use default HDO when importModuleDynamically is not set This makes it possile to hit the in-isolate compilation cache when host-defined options are not necessary. PR-URL: https://github.com/nodejs/node/pull/49950 Backport-PR-URL: https://github.com/nodejs/node/pull/51004 Refs: https://github.com/nodejs/node/issues/35375 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chengzhong Wu Reviewed-By: Stephen Belanger --- .../vm/compile-script-in-isolate-cache.js | 35 +++++++++++++++++++ lib/internal/modules/esm/utils.js | 11 ++++++ lib/vm.js | 11 ++++-- src/env_properties.h | 1 + src/node_contextify.cc | 16 +++++++-- .../test-vm-no-dynamic-import-callback.js | 13 +++++++ 6 files changed, 81 insertions(+), 6 deletions(-) create mode 100644 benchmark/vm/compile-script-in-isolate-cache.js create mode 100644 test/parallel/test-vm-no-dynamic-import-callback.js diff --git a/benchmark/vm/compile-script-in-isolate-cache.js b/benchmark/vm/compile-script-in-isolate-cache.js new file mode 100644 index 00000000000000..7eceb0eba0d215 --- /dev/null +++ b/benchmark/vm/compile-script-in-isolate-cache.js @@ -0,0 +1,35 @@ +'use strict'; + +// This benchmarks compiling scripts that hit the in-isolate compilation +// cache (by having the same source). +const common = require('../common.js'); +const fs = require('fs'); +const vm = require('vm'); +const fixtures = require('../../test/common/fixtures.js'); +const scriptPath = fixtures.path('snapshot', 'typescript.js'); + +const bench = common.createBenchmark(main, { + type: ['with-dynamic-import-callback', 'without-dynamic-import-callback'], + n: [100], +}); + +const scriptSource = fs.readFileSync(scriptPath, 'utf8'); + +function main({ n, type }) { + let script; + bench.start(); + const options = {}; + switch (type) { + case 'with-dynamic-import-callback': + // Use a dummy callback for now until we really need to benchmark it. + options.importModuleDynamically = async () => {}; + break; + case 'without-dynamic-import-callback': + break; + } + for (let i = 0; i < n; i++) { + script = new vm.Script(scriptSource, options); + } + bench.end(n); + script.runInThisContext(); +} diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js index b6b98569849ef4..365d7ed7136b94 100644 --- a/lib/internal/modules/esm/utils.js +++ b/lib/internal/modules/esm/utils.js @@ -12,6 +12,10 @@ const { host_defined_option_symbol, }, } = internalBinding('util'); +const { + default_host_defined_options, +} = internalBinding('symbols'); + const { ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING, ERR_INVALID_ARG_VALUE, @@ -128,6 +132,13 @@ const moduleRegistries = new SafeWeakMap(); */ function registerModule(referrer, registry) { const idSymbol = referrer[host_defined_option_symbol]; + if (idSymbol === default_host_defined_options) { + // The referrer is compiled without custom callbacks, so there is + // no registry to hold on to. We'll throw + // ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING when a callback is + // needed. + return; + } // To prevent it from being GC'ed. registry.callbackReferrer ??= referrer; moduleRegistries.set(idSymbol, registry); diff --git a/lib/vm.js b/lib/vm.js index 5ca04d6fb41758..06e72114f2361a 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -87,6 +87,12 @@ class Script extends ContextifyScript { } validateBoolean(produceCachedData, 'options.produceCachedData'); + if (importModuleDynamically !== undefined) { + // Check that it's either undefined or a function before we pass + // it into the native constructor. + validateFunction(importModuleDynamically, + 'options.importModuleDynamically'); + } // Calling `ReThrow()` on a native TryCatch does not generate a new // abort-on-uncaught-exception check. A dummy try/catch in JS land // protects against that. @@ -97,14 +103,13 @@ class Script extends ContextifyScript { columnOffset, cachedData, produceCachedData, - parsingContext); + parsingContext, + importModuleDynamically !== undefined); } catch (e) { throw e; /* node-do-not-add-exception-line */ } if (importModuleDynamically !== undefined) { - validateFunction(importModuleDynamically, - 'options.importModuleDynamically'); const { importModuleDynamicallyWrap } = require('internal/vm/module'); const { registerModule } = require('internal/modules/esm/utils'); registerModule(this, { diff --git a/src/env_properties.h b/src/env_properties.h index 47d41826014c48..c00c1987457712 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -31,6 +31,7 @@ // Symbols are per-isolate primitives but Environment proxies them // for the sake of convenience. #define PER_ISOLATE_SYMBOL_PROPERTIES(V) \ + V(default_host_defined_options, "default_host_defined_options") \ V(fs_use_promises_symbol, "fs_use_promises_symbol") \ V(async_id_symbol, "async_id_symbol") \ V(handle_onclose_symbol, "handle_onclose") \ diff --git a/src/node_contextify.cc b/src/node_contextify.cc index f631d26e0d0117..8b58fa7a2bccb4 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -787,10 +787,12 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { bool produce_cached_data = false; Local parsing_context = context; + bool needs_custom_host_defined_options = false; if (argc > 2) { // new ContextifyScript(code, filename, lineOffset, columnOffset, - // cachedData, produceCachedData, parsingContext) - CHECK_EQ(argc, 7); + // cachedData, produceCachedData, parsingContext, + // needsCustomHostDefinedOptions) + CHECK_EQ(argc, 8); CHECK(args[2]->IsNumber()); line_offset = args[2].As()->Value(); CHECK(args[3]->IsNumber()); @@ -809,6 +811,9 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { CHECK_NOT_NULL(sandbox); parsing_context = sandbox->context(); } + if (args[7]->IsTrue()) { + needs_custom_host_defined_options = true; + } } ContextifyScript* contextify_script = @@ -832,7 +837,12 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { Local host_defined_options = PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); - Local id_symbol = Symbol::New(isolate, filename); + // We need a default host defined options that's the same for all scripts + // not needing custom module callbacks for so that the isolate compilation + // cache can be hit. + Local id_symbol = needs_custom_host_defined_options + ? Symbol::New(isolate, filename) + : env->default_host_defined_options(); host_defined_options->Set( isolate, loader::HostDefinedOptions::kID, id_symbol); diff --git a/test/parallel/test-vm-no-dynamic-import-callback.js b/test/parallel/test-vm-no-dynamic-import-callback.js new file mode 100644 index 00000000000000..3651f997598b21 --- /dev/null +++ b/test/parallel/test-vm-no-dynamic-import-callback.js @@ -0,0 +1,13 @@ +'use strict'; + +require('../common'); +const { Script } = require('vm'); +const assert = require('assert'); + +assert.rejects(async () => { + const script = new Script('import("fs")'); + const imported = script.runInThisContext(); + await imported; +}, { + code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING' +});