From a7c523e26df468782691c7e16b2e033c6256beb3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 13 Feb 2020 20:40:50 +0100 Subject: [PATCH] src: prefer 3-argument Array::New() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is nicer, because: 1. It reduces overall code size, 2. It’s faster, because `Object::Set()` calls are relatively slow, and 3. It helps avoid invalid `.Check()`/`.FromJust()` calls. PR-URL: https://github.com/nodejs/node/pull/31775 Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: David Carlier --- src/cares_wrap.cc | 30 +++++++++--------- src/js_stream.cc | 9 +++--- src/module_wrap.cc | 18 ++++++----- src/node_crypto.cc | 78 ++++++++++++++++++++-------------------------- src/node_file.cc | 27 ++++++---------- src/node_v8.cc | 16 +++++----- src/spawn_sync.cc | 10 +++--- 7 files changed, 85 insertions(+), 103 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index f7a02e469aa79a..8eeaeddf8300d8 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -749,16 +749,12 @@ Local AddrTTLToArray(Environment* env, const T* addrttls, size_t naddrttls) { auto isolate = env->isolate(); - EscapableHandleScope escapable_handle_scope(isolate); - auto context = env->context(); - Local ttls = Array::New(isolate, naddrttls); - for (size_t i = 0; i < naddrttls; i++) { - auto value = Integer::NewFromUnsigned(isolate, addrttls[i].ttl); - ttls->Set(context, i, value).Check(); - } + MaybeStackBuffer, 8> ttls(naddrttls); + for (size_t i = 0; i < naddrttls; i++) + ttls[i] = Integer::NewFromUnsigned(isolate, addrttls[i].ttl); - return escapable_handle_scope.Escape(ttls); + return Array::New(isolate, ttls.out(), naddrttls); } @@ -2039,6 +2035,7 @@ void GetServers(const FunctionCallbackInfo& args) { int r = ares_get_servers_ports(channel->cares_channel(), &servers); CHECK_EQ(r, ARES_SUCCESS); + auto cleanup = OnScopeLeave([&]() { ares_free_data(servers); }); ares_addr_port_node* cur = servers; @@ -2049,17 +2046,18 @@ void GetServers(const FunctionCallbackInfo& args) { int err = uv_inet_ntop(cur->family, caddr, ip, sizeof(ip)); CHECK_EQ(err, 0); - Local ret = Array::New(env->isolate(), 2); - ret->Set(env->context(), 0, OneByteString(env->isolate(), ip)).Check(); - ret->Set(env->context(), - 1, - Integer::New(env->isolate(), cur->udp_port)).Check(); + Local ret[] = { + OneByteString(env->isolate(), ip), + Integer::New(env->isolate(), cur->udp_port) + }; - server_array->Set(env->context(), i, ret).Check(); + if (server_array->Set(env->context(), i, + Array::New(env->isolate(), ret, arraysize(ret))) + .IsNothing()) { + return; + } } - ares_free_data(servers); - args.GetReturnValue().Set(server_array); } diff --git a/src/js_stream.cc b/src/js_stream.cc index a67fd37dbdb2b6..64941b1c4e4fb7 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -116,16 +116,15 @@ int JSStream::DoWrite(WriteWrap* w, HandleScope scope(env()->isolate()); Context::Scope context_scope(env()->context()); - Local bufs_arr = Array::New(env()->isolate(), count); - Local buf; + MaybeStackBuffer, 16> bufs_arr(count); for (size_t i = 0; i < count; i++) { - buf = Buffer::Copy(env(), bufs[i].base, bufs[i].len).ToLocalChecked(); - bufs_arr->Set(env()->context(), i, buf).Check(); + bufs_arr[i] = + Buffer::Copy(env(), bufs[i].base, bufs[i].len).ToLocalChecked(); } Local argv[] = { w->object(), - bufs_arr + Array::New(env()->isolate(), bufs_arr.out(), count) }; TryCatchScope try_catch(env()); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 0bc32f7846b022..68359178f4ab38 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -264,11 +264,11 @@ void ModuleWrap::Link(const FunctionCallbackInfo& args) { Local mod_context = obj->context_.Get(isolate); Local module = obj->module_.Get(isolate); - Local promises = Array::New(isolate, - module->GetModuleRequestsLength()); + const int module_requests_length = module->GetModuleRequestsLength(); + MaybeStackBuffer, 16> promises(module_requests_length); // call the dependency resolve callbacks - for (int i = 0; i < module->GetModuleRequestsLength(); i++) { + for (int i = 0; i < module_requests_length; i++) { Local specifier = module->GetModuleRequest(i); Utf8Value specifier_utf8(env->isolate(), specifier); std::string specifier_std(*specifier_utf8, specifier_utf8.length()); @@ -290,10 +290,11 @@ void ModuleWrap::Link(const FunctionCallbackInfo& args) { Local resolve_promise = resolve_return_value.As(); obj->resolve_cache_[specifier_std].Reset(env->isolate(), resolve_promise); - promises->Set(mod_context, i, resolve_promise).Check(); + promises[i] = resolve_promise; } - args.GetReturnValue().Set(promises); + args.GetReturnValue().Set( + Array::New(isolate, promises.out(), promises.length())); } void ModuleWrap::Instantiate(const FunctionCallbackInfo& args) { @@ -426,12 +427,13 @@ void ModuleWrap::GetStaticDependencySpecifiers( int count = module->GetModuleRequestsLength(); - Local specifiers = Array::New(env->isolate(), count); + MaybeStackBuffer, 16> specifiers(count); for (int i = 0; i < count; i++) - specifiers->Set(env->context(), i, module->GetModuleRequest(i)).Check(); + specifiers[i] = module->GetModuleRequest(i); - args.GetReturnValue().Set(specifiers); + args.GetReturnValue().Set( + Array::New(env->isolate(), specifiers.out(), count)); } void ModuleWrap::GetError(const FunctionCallbackInfo& args) { diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 92760fb8c8577b..2176fffc543e0b 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1011,19 +1011,19 @@ static X509_STORE* NewRootCertStore() { void GetRootCertificates(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - Local result = Array::New(env->isolate(), arraysize(root_certs)); + Local result[arraysize(root_certs)]; for (size_t i = 0; i < arraysize(root_certs); i++) { - Local value; - if (!String::NewFromOneByte(env->isolate(), - reinterpret_cast(root_certs[i]), - NewStringType::kNormal).ToLocal(&value) || - !result->Set(env->context(), i, value).FromMaybe(false)) { + if (!String::NewFromOneByte( + env->isolate(), + reinterpret_cast(root_certs[i]), + NewStringType::kNormal).ToLocal(&result[i])) { return; } } - args.GetReturnValue().Set(result); + args.GetReturnValue().Set( + Array::New(env->isolate(), result, arraysize(root_certs))); } @@ -2138,22 +2138,22 @@ static Local X509ToObject(Environment* env, X509* cert) { StackOfASN1 eku(static_cast( X509_get_ext_d2i(cert, NID_ext_key_usage, nullptr, nullptr))); if (eku) { - Local ext_key_usage = Array::New(env->isolate()); + const int count = sk_ASN1_OBJECT_num(eku.get()); + MaybeStackBuffer, 16> ext_key_usage(count); char buf[256]; int j = 0; - for (int i = 0; i < sk_ASN1_OBJECT_num(eku.get()); i++) { + for (int i = 0; i < count; i++) { if (OBJ_obj2txt(buf, sizeof(buf), sk_ASN1_OBJECT_value(eku.get(), i), 1) >= 0) { - ext_key_usage->Set(context, - j++, - OneByteString(env->isolate(), buf)).Check(); + ext_key_usage[j++] = OneByteString(env->isolate(), buf); } } eku.reset(); - info->Set(context, env->ext_key_usage_string(), ext_key_usage).Check(); + info->Set(context, env->ext_key_usage_string(), + Array::New(env->isolate(), ext_key_usage.out(), count)).Check(); } if (ASN1_INTEGER* serial_number = X509_get_serialNumber(cert)) { @@ -6799,15 +6799,8 @@ void GenerateKeyPair(const FunctionCallbackInfo& args, Local err, pubkey, privkey; job->ToResult(&err, &pubkey, &privkey); - bool (*IsNotTrue)(Maybe) = [](Maybe maybe) { - return maybe.IsNothing() || !maybe.ToChecked(); - }; - Local ret = Array::New(env->isolate(), 3); - if (IsNotTrue(ret->Set(env->context(), 0, err)) || - IsNotTrue(ret->Set(env->context(), 1, pubkey)) || - IsNotTrue(ret->Set(env->context(), 2, privkey))) - return; - args.GetReturnValue().Set(ret); + Local ret[] = { err, pubkey, privkey }; + args.GetReturnValue().Set(Array::New(env->isolate(), ret, arraysize(ret))); } void GenerateKeyPairRSA(const FunctionCallbackInfo& args) { @@ -6940,17 +6933,6 @@ void GetSSLCiphers(const FunctionCallbackInfo& args) { CHECK(ssl); STACK_OF(SSL_CIPHER)* ciphers = SSL_get_ciphers(ssl.get()); - int n = sk_SSL_CIPHER_num(ciphers); - Local arr = Array::New(env->isolate(), n); - - for (int i = 0; i < n; ++i) { - const SSL_CIPHER* cipher = sk_SSL_CIPHER_value(ciphers, i); - arr->Set(env->context(), - i, - OneByteString(args.GetIsolate(), - SSL_CIPHER_get_name(cipher))).Check(); - } - // TLSv1.3 ciphers aren't listed by EVP. There are only 5, we could just // document them, but since there are only 5, easier to just add them manually // and not have to explain their absence in the API docs. They are lower-cased @@ -6963,13 +6945,20 @@ void GetSSLCiphers(const FunctionCallbackInfo& args) { "tls_aes_128_ccm_sha256" }; + const int n = sk_SSL_CIPHER_num(ciphers); + std::vector> arr(n + arraysize(TLS13_CIPHERS)); + + for (int i = 0; i < n; ++i) { + const SSL_CIPHER* cipher = sk_SSL_CIPHER_value(ciphers, i); + arr[i] = OneByteString(env->isolate(), SSL_CIPHER_get_name(cipher)); + } + for (unsigned i = 0; i < arraysize(TLS13_CIPHERS); ++i) { const char* name = TLS13_CIPHERS[i]; - arr->Set(env->context(), - arr->Length(), OneByteString(args.GetIsolate(), name)).Check(); + arr[n + i] = OneByteString(env->isolate(), name); } - args.GetReturnValue().Set(arr); + args.GetReturnValue().Set(Array::New(env->isolate(), arr.data(), arr.size())); } @@ -7020,22 +7009,23 @@ void GetHashes(const FunctionCallbackInfo& args) { void GetCurves(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); const size_t num_curves = EC_get_builtin_curves(nullptr, 0); - Local arr = Array::New(env->isolate(), num_curves); if (num_curves) { std::vector curves(num_curves); if (EC_get_builtin_curves(curves.data(), num_curves)) { - for (size_t i = 0; i < num_curves; i++) { - arr->Set(env->context(), - i, - OneByteString(env->isolate(), - OBJ_nid2sn(curves[i].nid))).Check(); - } + std::vector> arr(num_curves); + + for (size_t i = 0; i < num_curves; i++) + arr[i] = OneByteString(env->isolate(), OBJ_nid2sn(curves[i].nid)); + + args.GetReturnValue().Set( + Array::New(env->isolate(), arr.data(), arr.size())); + return; } } - args.GetReturnValue().Set(arr); + args.GetReturnValue().Set(Array::New(env->isolate())); } diff --git a/src/node_file.cc b/src/node_file.cc index 2f66080e5caa14..4d4960fc583653 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -748,16 +748,11 @@ void AfterScanDirWithTypes(uv_fs_t* req) { type_v.emplace_back(Integer::New(isolate, ent.type)); } - Local result = Array::New(isolate, 2); - result->Set(env->context(), - 0, - Array::New(isolate, name_v.data(), - name_v.size())).Check(); - result->Set(env->context(), - 1, - Array::New(isolate, type_v.data(), - type_v.size())).Check(); - req_wrap->Resolve(result); + Local result[] = { + Array::New(isolate, name_v.data(), name_v.size()), + Array::New(isolate, type_v.data(), type_v.size()) + }; + req_wrap->Resolve(Array::New(isolate, result, arraysize(result))); } void Access(const FunctionCallbackInfo& args) { @@ -1611,13 +1606,11 @@ static void ReadDir(const FunctionCallbackInfo& args) { Local names = Array::New(isolate, name_v.data(), name_v.size()); if (with_types) { - Local result = Array::New(isolate, 2); - result->Set(env->context(), 0, names).Check(); - result->Set(env->context(), - 1, - Array::New(isolate, type_v.data(), - type_v.size())).Check(); - args.GetReturnValue().Set(result); + Local result[] = { + names, + Array::New(isolate, type_v.data(), type_v.size()) + }; + args.GetReturnValue().Set(Array::New(isolate, result, arraysize(result))); } else { args.GetReturnValue().Set(names); } diff --git a/src/node_v8.cc b/src/node_v8.cc index d1fb3666d1abb6..00bfaf796b13c2 100644 --- a/src/node_v8.cc +++ b/src/node_v8.cc @@ -218,19 +218,19 @@ void Initialize(Local target, // Heap space names are extracted once and exposed to JavaScript to // avoid excessive creation of heap space name Strings. HeapSpaceStatistics s; - const Local heap_spaces = Array::New(env->isolate(), - number_of_heap_spaces); + MaybeStackBuffer, 16> heap_spaces(number_of_heap_spaces); for (size_t i = 0; i < number_of_heap_spaces; i++) { env->isolate()->GetHeapSpaceStatistics(&s, i); - Local heap_space_name = String::NewFromUtf8(env->isolate(), - s.space_name(), - NewStringType::kNormal) - .ToLocalChecked(); - heap_spaces->Set(env->context(), i, heap_space_name).Check(); + heap_spaces[i] = String::NewFromUtf8(env->isolate(), + s.space_name(), + NewStringType::kNormal) + .ToLocalChecked(); } target->Set(env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "kHeapSpaces"), - heap_spaces).Check(); + Array::New(env->isolate(), + heap_spaces.out(), + number_of_heap_spaces)).Check(); env->SetMethod(target, "updateHeapSpaceStatisticsArrayBuffer", diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc index 3b277ad70adb66..589b77f6c1eb95 100644 --- a/src/spawn_sync.cc +++ b/src/spawn_sync.cc @@ -721,18 +721,18 @@ Local SyncProcessRunner::BuildOutputArray() { CHECK(!stdio_pipes_.empty()); EscapableHandleScope scope(env()->isolate()); - Local context = env()->context(); - Local js_output = Array::New(env()->isolate(), stdio_count_); + MaybeStackBuffer, 8> js_output(stdio_pipes_.size()); for (uint32_t i = 0; i < stdio_pipes_.size(); i++) { SyncProcessStdioPipe* h = stdio_pipes_[i].get(); if (h != nullptr && h->writable()) - js_output->Set(context, i, h->GetOutputAsBuffer(env())).Check(); + js_output[i] = h->GetOutputAsBuffer(env()); else - js_output->Set(context, i, Null(env()->isolate())).Check(); + js_output[i] = Null(env()->isolate()); } - return scope.Escape(js_output); + return scope.Escape( + Array::New(env()->isolate(), js_output.out(), js_output.length())); } Maybe SyncProcessRunner::ParseOptions(Local js_value) {