Skip to content

Commit

Permalink
vm: use default HDO when importModuleDynamically is not set
Browse files Browse the repository at this point in the history
This makes it possile to hit the in-isolate compilation cache when
host-defined options are not necessary.

PR-URL: #49950
Backport-PR-URL: #51004
Refs: #35375
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
  • Loading branch information
joyeecheung authored and richardlau committed Mar 15, 2024
1 parent 5e42651 commit 6291c10
Show file tree
Hide file tree
Showing 6 changed files with 81 additions and 6 deletions.
35 changes: 35 additions & 0 deletions benchmark/vm/compile-script-in-isolate-cache.js
Original file line number Diff line number Diff line change
@@ -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();
}
11 changes: 11 additions & 0 deletions lib/internal/modules/esm/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
11 changes: 8 additions & 3 deletions lib/vm.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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, {
Expand Down
1 change: 1 addition & 0 deletions src/env_properties.h
Original file line number Diff line number Diff line change
Expand Up @@ -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") \
Expand Down
16 changes: 13 additions & 3 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -787,10 +787,12 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
bool produce_cached_data = false;
Local<Context> 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<Int32>()->Value();
CHECK(args[3]->IsNumber());
Expand All @@ -809,6 +811,9 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
CHECK_NOT_NULL(sandbox);
parsing_context = sandbox->context();
}
if (args[7]->IsTrue()) {
needs_custom_host_defined_options = true;
}
}

ContextifyScript* contextify_script =
Expand All @@ -832,7 +837,12 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {

Local<PrimitiveArray> host_defined_options =
PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
Local<Symbol> 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<Symbol> 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);

Expand Down
13 changes: 13 additions & 0 deletions test/parallel/test-vm-no-dynamic-import-callback.js
Original file line number Diff line number Diff line change
@@ -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'
});

0 comments on commit 6291c10

Please sign in to comment.