From 3aa32486789827c3a99724e5bcb3562d5baad40a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 31 Dec 2018 00:59:25 +0100 Subject: [PATCH 1/2] src: improve ToV8Value() functions - Cache the `isolate` value between calls - Introduce an overload for dealing with integers/numbers - Use the vectored `v8::Array::New` constructor + `MaybeStackBuffer` for faster array creation --- src/util-inl.h | 55 +++++++++++++++++++++++++++++++++++++------------- src/util.h | 15 +++++++++++--- 2 files changed, 53 insertions(+), 17 deletions(-) diff --git a/src/util-inl.h b/src/util-inl.h index acd370e2358264..6d612f1eb7c8b7 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -377,8 +377,9 @@ inline char* UncheckedCalloc(size_t n) { return UncheckedCalloc(n); } void ThrowErrStringTooLong(v8::Isolate* isolate); v8::MaybeLocal ToV8Value(v8::Local context, - const std::string& str) { - v8::Isolate* isolate = context->GetIsolate(); + const std::string& str, + v8::Isolate* isolate) { + if (isolate == nullptr) isolate = context->GetIsolate(); if (UNLIKELY(str.size() >= static_cast(v8::String::kMaxLength))) { // V8 only has a TODO comment about adding an exception when the maximum // string size is exceeded. @@ -393,33 +394,33 @@ v8::MaybeLocal ToV8Value(v8::Local context, template v8::MaybeLocal ToV8Value(v8::Local context, - const std::vector& vec) { - v8::Isolate* isolate = context->GetIsolate(); + const std::vector& vec, + v8::Isolate* isolate) { + if (isolate == nullptr) isolate = context->GetIsolate(); v8::EscapableHandleScope handle_scope(isolate); - v8::Local arr = v8::Array::New(isolate, vec.size()); + MaybeStackBuffer, 128> arr(vec.size()); + arr.SetLength(vec.size()); for (size_t i = 0; i < vec.size(); ++i) { - v8::Local val; - if (!ToV8Value(context, vec[i]).ToLocal(&val) || - arr->Set(context, i, val).IsNothing()) { + if (!ToV8Value(context, vec[i], isolate).ToLocal(&arr[i])) return v8::MaybeLocal(); - } } - return handle_scope.Escape(arr); + return handle_scope.Escape(v8::Array::New(isolate, arr.out(), arr.length())); } template v8::MaybeLocal ToV8Value(v8::Local context, - const std::unordered_map& map) { - v8::Isolate* isolate = context->GetIsolate(); + const std::unordered_map& map, + v8::Isolate* isolate) { + if (isolate == nullptr) isolate = context->GetIsolate(); v8::EscapableHandleScope handle_scope(isolate); v8::Local ret = v8::Map::New(isolate); for (const auto& item : map) { v8::Local first, second; - if (!ToV8Value(context, item.first).ToLocal(&first) || - !ToV8Value(context, item.second).ToLocal(&second) || + if (!ToV8Value(context, item.first, isolate).ToLocal(&first) || + !ToV8Value(context, item.second, isolate).ToLocal(&second) || ret->Set(context, first, second).IsEmpty()) { return v8::MaybeLocal(); } @@ -428,6 +429,32 @@ v8::MaybeLocal ToV8Value(v8::Local context, return handle_scope.Escape(ret); } +template +v8::MaybeLocal ToV8Value(v8::Local context, + const T& number, + v8::Isolate* isolate) { + if (isolate == nullptr) isolate = context->GetIsolate(); + + using Limits = std::numeric_limits; + // Choose Uint32, Int32, or Double depending on range checks. + // These checks should all collapse at compile time. + if (static_cast(Limits::max()) <= + std::numeric_limits::max() && + static_cast(Limits::min()) >= + std::numeric_limits::min() && Limits::is_exact) { + return v8::Integer::NewFromUnsigned(isolate, static_cast(number)); + } + + if (static_cast(Limits::max()) <= + std::numeric_limits::max() && + static_cast(Limits::min()) >= + std::numeric_limits::min() && Limits::is_exact) { + return v8::Integer::New(isolate, static_cast(number)); + } + + return v8::Number::New(isolate, static_cast(number)); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/util.h b/src/util.h index d3835c7e692b7d..b1a135b46dc2a9 100644 --- a/src/util.h +++ b/src/util.h @@ -35,6 +35,7 @@ #include #include // std::function +#include #include #include #include @@ -519,13 +520,21 @@ using DeleteFnPtr = typename FunctionDeleter::Pointer; std::set ParseCommaSeparatedSet(const std::string& in); inline v8::MaybeLocal ToV8Value(v8::Local context, - const std::string& str); + const std::string& str, + v8::Isolate* isolate = nullptr); +template ::is_specialized, bool>::type> +inline v8::MaybeLocal ToV8Value(v8::Local context, + const T& number, + v8::Isolate* isolate = nullptr); template inline v8::MaybeLocal ToV8Value(v8::Local context, - const std::vector& vec); + const std::vector& vec, + v8::Isolate* isolate = nullptr); template inline v8::MaybeLocal ToV8Value(v8::Local context, - const std::unordered_map& map); + const std::unordered_map& map, + v8::Isolate* isolate = nullptr); // These macros expects a `Isolate* isolate` and a `Local context` // to be in the scope. From 1e3897b45d27d0dfadf1828fea376161714cebae Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 31 Dec 2018 01:03:16 +0100 Subject: [PATCH 2/2] src: simplify JS Array creation Use `ToV8Value()` where appropriate to reduce duplication and avoid extra `Array::Set()` calls. --- src/node.cc | 30 +++++------------------------- src/node_credentials.cc | 35 ++++++++++------------------------- src/node_crypto.cc | 16 +++------------- src/node_url.cc | 11 +---------- 4 files changed, 19 insertions(+), 73 deletions(-) diff --git a/src/node.cc b/src/node.cc index bc359903be1b7a..87abe9f613b3a6 100644 --- a/src/node.cc +++ b/src/node.cc @@ -932,28 +932,15 @@ void SetupProcessObject(Environment* env, #endif // process.argv - Local arguments = Array::New(env->isolate(), args.size()); - for (size_t i = 0; i < args.size(); ++i) { - arguments->Set(env->context(), i, - String::NewFromUtf8(env->isolate(), args[i].c_str(), - NewStringType::kNormal).ToLocalChecked()) - .FromJust(); - } process->Set(env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "argv"), - arguments).FromJust(); + ToV8Value(env->context(), args).ToLocalChecked()).FromJust(); // process.execArgv - Local exec_arguments = Array::New(env->isolate(), exec_args.size()); - for (size_t i = 0; i < exec_args.size(); ++i) { - exec_arguments->Set(env->context(), i, - String::NewFromUtf8(env->isolate(), exec_args[i].c_str(), - NewStringType::kNormal).ToLocalChecked()) - .FromJust(); - } process->Set(env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "execArgv"), - exec_arguments).FromJust(); + ToV8Value(env->context(), exec_args) + .ToLocalChecked()).FromJust(); // create process.env process @@ -1004,17 +991,10 @@ void SetupProcessObject(Environment* env, const std::vector& preload_modules = env->options()->preload_modules; if (!preload_modules.empty()) { - Local array = Array::New(env->isolate()); - for (unsigned int i = 0; i < preload_modules.size(); ++i) { - Local module = String::NewFromUtf8(env->isolate(), - preload_modules[i].c_str(), - NewStringType::kNormal) - .ToLocalChecked(); - array->Set(env->context(), i, module).FromJust(); - } READONLY_PROPERTY(process, "_preload_modules", - array); + ToV8Value(env->context(), preload_modules) + .ToLocalChecked()); } // --no-deprecation diff --git a/src/node_credentials.cc b/src/node_credentials.cc index 0f202dfb753bf7..82f6ef0dd8c0f3 100644 --- a/src/node_credentials.cc +++ b/src/node_credentials.cc @@ -15,9 +15,9 @@ using v8::Array; using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; -using v8::Integer; using v8::Isolate; using v8::Local; +using v8::MaybeLocal; using v8::Object; using v8::String; using v8::Uint32; @@ -253,36 +253,21 @@ static void GetGroups(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); int ngroups = getgroups(0, nullptr); - if (ngroups == -1) return env->ThrowErrnoException(errno, "getgroups"); - gid_t* groups = new gid_t[ngroups]; - - ngroups = getgroups(ngroups, groups); + std::vector groups(ngroups); - if (ngroups == -1) { - delete[] groups; + ngroups = getgroups(ngroups, groups.data()); + if (ngroups == -1) return env->ThrowErrnoException(errno, "getgroups"); - } - Local groups_list = Array::New(env->isolate(), ngroups); - bool seen_egid = false; + groups.resize(ngroups); gid_t egid = getegid(); - - for (int i = 0; i < ngroups; i++) { - groups_list->Set(env->context(), i, Integer::New(env->isolate(), groups[i])) - .FromJust(); - if (groups[i] == egid) seen_egid = true; - } - - delete[] groups; - - if (seen_egid == false) - groups_list - ->Set(env->context(), ngroups, Integer::New(env->isolate(), egid)) - .FromJust(); - - args.GetReturnValue().Set(groups_list); + if (std::find(groups.begin(), groups.end(), egid) == groups.end()) + groups.push_back(egid); + MaybeLocal array = ToV8Value(env->context(), groups); + if (!array.IsEmpty()) + args.GetReturnValue().Set(array.ToLocalChecked()); } static void SetGroups(const FunctionCallbackInfo& args) { diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c18432b6205105..ab355688888ee3 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -250,22 +250,12 @@ struct CryptoErrorVector : public std::vector { CHECK(!exception_v.IsEmpty()); if (!empty()) { - Local array = Array::New(env->isolate(), size()); - CHECK(!array.IsEmpty()); - - for (const std::string& string : *this) { - const size_t index = &string - &front(); - Local value = - String::NewFromUtf8(env->isolate(), string.data(), - NewStringType::kNormal, string.size()) - .ToLocalChecked(); - array->Set(env->context(), index, value).FromJust(); - } - CHECK(exception_v->IsObject()); Local exception = exception_v.As(); exception->Set(env->context(), - env->openssl_error_stack(), array).FromJust(); + env->openssl_error_stack(), + ToV8Value(env->context(), *this).ToLocalChecked()) + .FromJust(); } return exception_v; diff --git a/src/node_url.cc b/src/node_url.cc index d5dba61ae4cc84..18b4efc31c1982 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -1190,15 +1190,6 @@ inline std::vector FromJSStringArray(Environment* env, return vec; } -inline Local ToJSStringArray(Environment* env, - const std::vector& vec) { - Isolate* isolate = env->isolate(); - Local array = Array::New(isolate, vec.size()); - for (size_t n = 0; n < vec.size(); n++) - array->Set(env->context(), n, Utf8String(isolate, vec[n])).FromJust(); - return array; -} - inline url_data HarvestBase(Environment* env, Local base_obj) { url_data base; Local context = env->context(); @@ -2119,7 +2110,7 @@ static inline void SetArgs(Environment* env, if (url.port > -1) argv[ARG_PORT] = Integer::New(isolate, url.port); if (url.flags & URL_FLAGS_HAS_PATH) - argv[ARG_PATH] = ToJSStringArray(env, url.path); + argv[ARG_PATH] = ToV8Value(env->context(), url.path).ToLocalChecked(); } static void Parse(Environment* env,