vm: don't set host-defined options when it's not necessary

This makes it possile to hit the in-isolate compilation cache when
host-defined options are not necessary.

Author: joyeecheung

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();
+  let 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();
+}

lib/vm.js

@@ -86,6 +86,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.
@@ -96,14 +102,13 @@ class Script extends ContextifyScript {
             columnOffset,
             cachedData,
             produceCachedData,
-            parsingContext);
+            parsingContext,
+            importModuleDynamically);
     } 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, {

src/node_contextify.cc

@@ -771,10 +771,13 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
   bool produce_cached_data = false;
   Local<Context> parsing_context = context;
 
+  Local<Symbol> host_defined_options_id;
+  Local<PrimitiveArray> host_defined_options;
   if (argc > 2) {
     // new ContextifyScript(code, filename, lineOffset, columnOffset,
-    //                      cachedData, produceCachedData, parsingContext)
-    CHECK_EQ(argc, 7);
+    //                      cachedData, produceCachedData, parsingContext,
+    //                      needsHostDefinedOptions)
+    CHECK_EQ(argc, 8);
     CHECK(args[2]->IsNumber());
     line_offset = args[2].As<Int32>()->Value();
     CHECK(args[3]->IsNumber());
@@ -793,6 +796,13 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
       CHECK_NOT_NULL(sandbox);
       parsing_context = sandbox->context();
     }
+    if (!args[7]->IsUndefined()) {
+      host_defined_options_id = Symbol::New(isolate, filename);
+      host_defined_options =
+          PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
+      host_defined_options->Set(
+          isolate, loader::HostDefinedOptions::kID, host_defined_options_id);
+    }
   }
 
   ContextifyScript* contextify_script =
@@ -814,12 +824,6 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
         data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength());
   }
 
-  Local<PrimitiveArray> host_defined_options =
-      PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
-  Local<Symbol> id_symbol = Symbol::New(isolate, filename);
-  host_defined_options->Set(
-      isolate, loader::HostDefinedOptions::kID, id_symbol);
-
   ScriptOrigin origin(isolate,
                       filename,
                       line_offset,                          // line offset
@@ -865,8 +869,11 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
     new_cached_data.reset(ScriptCompiler::CreateCodeCache(v8_script));
   }
 
-  if (contextify_script->object()
-          ->SetPrivate(context, env->host_defined_option_symbol(), id_symbol)
+  if (!host_defined_options_id.IsEmpty() &&
+      contextify_script->object()
+          ->SetPrivate(context,
+                       env->host_defined_option_symbol(),
+                       host_defined_options_id)
           .IsNothing()) {
     return;
   }