Skip to content

Commit f14a616

Browse files
joyeecheungjoyeecheung
committed
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.
1 parent c829c03 commit f14a616

File tree

4 files changed

+111
-38
lines changed

4 files changed

+111
-38
lines changed

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

+35
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+
let 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/vm.js

+8-3
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,12 @@ class Script extends ContextifyScript {
8686
}
8787
validateBoolean(produceCachedData, 'options.produceCachedData');
8888

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

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

‎src/crypto/crypto_context.cc

+51-25
Original file line numberDiff line numberDiff line change
@@ -1052,34 +1052,60 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
10521052
EVP_PKEY* pkey_ptr = nullptr;
10531053
X509* cert_ptr = nullptr;
10541054
STACK_OF(X509)* extra_certs_ptr = nullptr;
1055-
if (d2i_PKCS12_bio(in.get(), &p12_ptr) &&
1056-
(p12.reset(p12_ptr), true) && // Move ownership to the smart pointer.
1057-
PKCS12_parse(p12.get(), pass.data(),
1058-
&pkey_ptr,
1059-
&cert_ptr,
1060-
&extra_certs_ptr) &&
1061-
(pkey.reset(pkey_ptr), cert.reset(cert_ptr),
1062-
extra_certs.reset(extra_certs_ptr), true) && // Move ownership.
1063-
SSL_CTX_use_certificate_chain(sc->ctx_.get(),
1064-
std::move(cert),
1065-
extra_certs.get(),
1066-
&sc->cert_,
1067-
&sc->issuer_) &&
1068-
SSL_CTX_use_PrivateKey(sc->ctx_.get(), pkey.get())) {
1069-
// Add CA certs too
1070-
for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) {
1071-
X509* ca = sk_X509_value(extra_certs.get(), i);
1072-
1073-
if (cert_store == GetOrCreateRootCertStore()) {
1074-
cert_store = NewRootCertStore();
1075-
SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store);
1076-
}
1077-
X509_STORE_add_cert(cert_store, ca);
1078-
SSL_CTX_add_client_CA(sc->ctx_.get(), ca);
1055+
1056+
if (!d2i_PKCS12_bio(in.get(), &p12_ptr)) {
1057+
goto done;
1058+
}
1059+
1060+
// Move ownership to the smart pointer:
1061+
p12.reset(p12_ptr);
1062+
1063+
if (!PKCS12_parse(
1064+
p12.get(), pass.data(), &pkey_ptr, &cert_ptr, &extra_certs_ptr)) {
1065+
goto done;
1066+
}
1067+
1068+
// Move ownership of the parsed data:
1069+
pkey.reset(pkey_ptr);
1070+
cert.reset(cert_ptr);
1071+
extra_certs.reset(extra_certs_ptr);
1072+
1073+
if (!pkey) {
1074+
return THROW_ERR_CRYPTO_OPERATION_FAILED(
1075+
env, "Unable to load private key from PFX data");
1076+
}
1077+
1078+
if (!cert) {
1079+
return THROW_ERR_CRYPTO_OPERATION_FAILED(
1080+
env, "Unable to load certificate from PFX data");
1081+
}
1082+
1083+
if (!SSL_CTX_use_certificate_chain(sc->ctx_.get(),
1084+
std::move(cert),
1085+
extra_certs.get(),
1086+
&sc->cert_,
1087+
&sc->issuer_)) {
1088+
goto done;
1089+
}
1090+
1091+
if (!SSL_CTX_use_PrivateKey(sc->ctx_.get(), pkey.get())) {
1092+
goto done;
1093+
}
1094+
1095+
// Add CA certs too
1096+
for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) {
1097+
X509* ca = sk_X509_value(extra_certs.get(), i);
1098+
1099+
if (cert_store == GetOrCreateRootCertStore()) {
1100+
cert_store = NewRootCertStore();
1101+
SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store);
10791102
}
1080-
ret = true;
1103+
X509_STORE_add_cert(cert_store, ca);
1104+
SSL_CTX_add_client_CA(sc->ctx_.get(), ca);
10811105
}
1106+
ret = true;
10821107

1108+
done:
10831109
if (!ret) {
10841110
// TODO(@jasnell): Should this use ThrowCryptoError?
10851111
unsigned long err = ERR_get_error(); // NOLINT(runtime/int)

‎src/node_contextify.cc

+17-10
Original file line numberDiff line numberDiff line change
@@ -771,10 +771,13 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
771771
bool produce_cached_data = false;
772772
Local<Context> parsing_context = context;
773773

774+
Local<Symbol> host_defined_options_id;
775+
Local<PrimitiveArray> host_defined_options;
774776
if (argc > 2) {
775777
// new ContextifyScript(code, filename, lineOffset, columnOffset,
776-
// cachedData, produceCachedData, parsingContext)
777-
CHECK_EQ(argc, 7);
778+
// cachedData, produceCachedData, parsingContext,
779+
// needsHostDefinedOptions)
780+
CHECK_EQ(argc, 8);
778781
CHECK(args[2]->IsNumber());
779782
line_offset = args[2].As<Int32>()->Value();
780783
CHECK(args[3]->IsNumber());
@@ -793,6 +796,13 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
793796
CHECK_NOT_NULL(sandbox);
794797
parsing_context = sandbox->context();
795798
}
799+
if (!args[7]->IsUndefined()) {
800+
host_defined_options_id = Symbol::New(isolate, filename);
801+
host_defined_options =
802+
PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
803+
host_defined_options->Set(
804+
isolate, loader::HostDefinedOptions::kID, host_defined_options_id);
805+
}
796806
}
797807

798808
ContextifyScript* contextify_script =
@@ -814,12 +824,6 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
814824
data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength());
815825
}
816826

817-
Local<PrimitiveArray> host_defined_options =
818-
PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
819-
Local<Symbol> id_symbol = Symbol::New(isolate, filename);
820-
host_defined_options->Set(
821-
isolate, loader::HostDefinedOptions::kID, id_symbol);
822-
823827
ScriptOrigin origin(isolate,
824828
filename,
825829
line_offset, // line offset
@@ -865,8 +869,11 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
865869
new_cached_data.reset(ScriptCompiler::CreateCodeCache(v8_script));
866870
}
867871

868-
if (contextify_script->object()
869-
->SetPrivate(context, env->host_defined_option_symbol(), id_symbol)
872+
if (!host_defined_options_id.IsEmpty() &&
873+
contextify_script->object()
874+
->SetPrivate(context,
875+
env->host_defined_option_symbol(),
876+
host_defined_options_id)
870877
.IsNothing()) {
871878
return;
872879
}

0 commit comments

Comments
 (0)