FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

vm: use default HDO when importModuleDynamically is not set · nodejs/node@6291c10 · GitHub

/ node Public

Commit 6291c10

Browse files
joyeecheung
authored and
richardlau
committed
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: #49950 Backport-PR-URL: #51004 Refs: #35375 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
1 parent 5e42651 commit 6291c10

6 files changed

Lines changed: 81 additions & 6 deletions

File tree

‎benchmark/vm/compile-script-in-isolate-cache.js‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
'use strict';
2+
3+
// This benchmarks compiling scripts that hit the in-isolate compilation
4+
// cache (by having the same source).
5+
const common = require('../common.js');
6+
const fs = require('fs');
7+
const vm = require('vm');
8+
const fixtures = require('../../test/common/fixtures.js');
9+
const scriptPath = fixtures.path('snapshot', 'typescript.js');
10+
11+
const bench = common.createBenchmark(main, {
12+
type: ['with-dynamic-import-callback', 'without-dynamic-import-callback'],
13+
n: [100],
14+
});
15+
16+
const scriptSource = fs.readFileSync(scriptPath, 'utf8');
17+
18+
function main({ n, type }) {
19+
let script;
20+
bench.start();
21+
const options = {};
22+
switch (type) {
23+
case 'with-dynamic-import-callback':
24+
// Use a dummy callback for now until we really need to benchmark it.
25+
options.importModuleDynamically = async () => {};
26+
break;
27+
case 'without-dynamic-import-callback':
28+
break;
29+
}
30+
for (let i = 0; i < n; i++) {
31+
script = new vm.Script(scriptSource, options);
32+
}
33+
bench.end(n);
34+
script.runInThisContext();
35+
}

‎lib/internal/modules/esm/utils.js‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@ const {
1212
host_defined_option_symbol,
1313
},
1414
} = internalBinding('util');
15+
const {
16+
default_host_defined_options,
17+
} = internalBinding('symbols');
18+
1519
const {
1620
ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING,
1721
ERR_INVALID_ARG_VALUE,
@@ -128,6 +132,13 @@ const moduleRegistries = new SafeWeakMap();
128132
*/
129133
function registerModule(referrer, registry) {
130134
const idSymbol = referrer[host_defined_option_symbol];
135+
if (idSymbol === default_host_defined_options) {
136+
// The referrer is compiled without custom callbacks, so there is
137+
// no registry to hold on to. We'll throw
138+
// ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING when a callback is
139+
// needed.
140+
return;
141+
}
131142
// To prevent it from being GC'ed.
132143
registry.callbackReferrer ??= referrer;
133144
moduleRegistries.set(idSymbol, registry);

‎lib/vm.js‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,12 @@ class Script extends ContextifyScript {
8787
}
8888
validateBoolean(produceCachedData, 'options.produceCachedData');
8989

90+
if (importModuleDynamically !== undefined) {
91+
// Check that it's either undefined or a function before we pass
92+
// it into the native constructor.
93+
validateFunction(importModuleDynamically,
94+
'options.importModuleDynamically');
95+
}
9096
// Calling `ReThrow()` on a native TryCatch does not generate a new
9197
// abort-on-uncaught-exception check. A dummy try/catch in JS land
9298
// protects against that.
@@ -97,14 +103,13 @@ class Script extends ContextifyScript {
97103
columnOffset,
98104
cachedData,
99105
produceCachedData,
100-
parsingContext);
106+
parsingContext,
107+
importModuleDynamically !== undefined);
101108
} catch (e) {
102109
throw e; /* node-do-not-add-exception-line */
103110
}
104111

105112
if (importModuleDynamically !== undefined) {
106-
validateFunction(importModuleDynamically,
107-
'options.importModuleDynamically');
108113
const { importModuleDynamicallyWrap } = require('internal/vm/module');
109114
const { registerModule } = require('internal/modules/esm/utils');
110115
registerModule(this, {

‎src/env_properties.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
// Symbols are per-isolate primitives but Environment proxies them
3232
// for the sake of convenience.
3333
#define PER_ISOLATE_SYMBOL_PROPERTIES(V) \
34+
V(default_host_defined_options, "default_host_defined_options") \
3435
V(fs_use_promises_symbol, "fs_use_promises_symbol") \
3536
V(async_id_symbol, "async_id_symbol") \
3637
V(handle_onclose_symbol, "handle_onclose") \

‎src/node_contextify.cc‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -787,10 +787,12 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
787787
bool produce_cached_data = false;
788788
Local<Context> parsing_context = context;
789789

790+
bool needs_custom_host_defined_options = false;
790791
if (argc > 2) {
791792
// new ContextifyScript(code, filename, lineOffset, columnOffset,
792-
// cachedData, produceCachedData, parsingContext)
793-
CHECK_EQ(argc, 7);
793+
// cachedData, produceCachedData, parsingContext,
794+
// needsCustomHostDefinedOptions)
795+
CHECK_EQ(argc, 8);
794796
CHECK(args[2]->IsNumber());
795797
line_offset = args[2].As<Int32>()->Value();
796798
CHECK(args[3]->IsNumber());
@@ -809,6 +811,9 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
809811
CHECK_NOT_NULL(sandbox);
810812
parsing_context = sandbox->context();
811813
}
814+
if (args[7]->IsTrue()) {
815+
needs_custom_host_defined_options = true;
816+
}
812817
}
813818

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

833838
Local<PrimitiveArray> host_defined_options =
834839
PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
835-
Local<Symbol> id_symbol = Symbol::New(isolate, filename);
840+
// We need a default host defined options that's the same for all scripts
841+
// not needing custom module callbacks for so that the isolate compilation
842+
// cache can be hit.
843+
Local<Symbol> id_symbol = needs_custom_host_defined_options
844+
? Symbol::New(isolate, filename)
845+
: env->default_host_defined_options();
836846
host_defined_options->Set(
837847
isolate, loader::HostDefinedOptions::kID, id_symbol);
838848

‎test/parallel/test-vm-no-dynamic-import-callback.js‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
'use strict';
2+
3+
require('../common');
4+
const { Script } = require('vm');
5+
const assert = require('assert');
6+
7+
assert.rejects(async () => {
8+
const script = new Script('import("fs")');
9+
const imported = script.runInThisContext();
10+
await imported;
11+
}, {
12+
code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING'
13+
});

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL