From 6693cfb663f2ecd1a6e18d31443e6764135f49a9 Mon Sep 17 00:00:00 2001 From: Denys Otrishko Date: Thu, 13 Feb 2020 20:24:18 +0200 Subject: [PATCH 1/2] src: improve KVStore API This adds `const char*` based APIs to KVStore to avoid multiple string conversions (char -> Utf8 -> Local -> char etc.) when possible. --- src/env.h | 2 ++ src/node_env_var.cc | 66 +++++++++++++++++++++++++++++++-------------- 2 files changed, 48 insertions(+), 20 deletions(-) diff --git a/src/env.h b/src/env.h index e100802ee74cc9..864ffb68c21ec2 100644 --- a/src/env.h +++ b/src/env.h @@ -605,11 +605,13 @@ class KVStore { virtual v8::MaybeLocal Get(v8::Isolate* isolate, v8::Local key) const = 0; + virtual v8::Maybe Get(const char* key) const = 0; virtual void Set(v8::Isolate* isolate, v8::Local key, v8::Local value) = 0; virtual int32_t Query(v8::Isolate* isolate, v8::Local key) const = 0; + virtual int32_t Query(const char* key) const = 0; virtual void Delete(v8::Isolate* isolate, v8::Local key) = 0; virtual v8::Local Enumerate(v8::Isolate* isolate) const = 0; diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 02f24fff205ce6..6928b595ae5652 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -30,8 +30,10 @@ using v8::Value; class RealEnvStore final : public KVStore { public: MaybeLocal Get(Isolate* isolate, Local key) const override; + Maybe Get(const char* key) const override; void Set(Isolate* isolate, Local key, Local value) override; int32_t Query(Isolate* isolate, Local key) const override; + int32_t Query(const char* key) const override; void Delete(Isolate* isolate, Local key) override; Local Enumerate(Isolate* isolate) const override; }; @@ -39,8 +41,10 @@ class RealEnvStore final : public KVStore { class MapKVStore final : public KVStore { public: MaybeLocal Get(Isolate* isolate, Local key) const override; + Maybe Get(const char* key) const override; void Set(Isolate* isolate, Local key, Local value) override; int32_t Query(Isolate* isolate, Local key) const override; + int32_t Query(const char* key) const override; void Delete(Isolate* isolate, Local key) override; Local Enumerate(Isolate* isolate) const override; @@ -72,26 +76,36 @@ void DateTimeConfigurationChangeNotification(Isolate* isolate, const T& key) { } } -MaybeLocal RealEnvStore::Get(Isolate* isolate, - Local property) const { +Maybe RealEnvStore::Get(const char* key) const { Mutex::ScopedLock lock(per_process::env_var_mutex); - node::Utf8Value key(isolate, property); size_t init_sz = 256; MaybeStackBuffer val; - int ret = uv_os_getenv(*key, *val, &init_sz); + int ret = uv_os_getenv(key, *val, &init_sz); if (ret == UV_ENOBUFS) { // Buffer is not large enough, reallocate to the updated init_sz // and fetch env value again. val.AllocateSufficientStorage(init_sz); - ret = uv_os_getenv(*key, *val, &init_sz); + ret = uv_os_getenv(key, *val, &init_sz); } if (ret >= 0) { // Env key value fetch success. - MaybeLocal value_string = - String::NewFromUtf8(isolate, *val, NewStringType::kNormal, init_sz); - return value_string; + return v8::Just(std::string(*val, init_sz)); + } + + return v8::Nothing(); +} + +MaybeLocal RealEnvStore::Get(Isolate* isolate, + Local property) const { + node::Utf8Value key(isolate, property); + Maybe value = Get(*key); + + if (value.IsJust()) { + std::string val = value.FromJust(); + return String::NewFromUtf8( + isolate, val.data(), NewStringType::kNormal, val.size()); } return MaybeLocal(); @@ -112,14 +126,12 @@ void RealEnvStore::Set(Isolate* isolate, DateTimeConfigurationChangeNotification(isolate, key); } -int32_t RealEnvStore::Query(Isolate* isolate, Local property) const { +int32_t RealEnvStore::Query(const char* key) const { Mutex::ScopedLock lock(per_process::env_var_mutex); - node::Utf8Value key(isolate, property); - char val[2]; size_t init_sz = sizeof(val); - int ret = uv_os_getenv(*key, val, &init_sz); + int ret = uv_os_getenv(key, val, &init_sz); if (ret == UV_ENOENT) { return -1; @@ -136,6 +148,11 @@ int32_t RealEnvStore::Query(Isolate* isolate, Local property) const { return 0; } +int32_t RealEnvStore::Query(Isolate* isolate, Local property) const { + node::Utf8Value key(isolate, property); + return Query(*key); +} + void RealEnvStore::Delete(Isolate* isolate, Local property) { Mutex::ScopedLock lock(per_process::env_var_mutex); @@ -190,13 +207,19 @@ std::shared_ptr KVStore::Clone(v8::Isolate* isolate) const { return copy; } -MaybeLocal MapKVStore::Get(Isolate* isolate, Local key) const { +Maybe MapKVStore::Get(const char* key) const { Mutex::ScopedLock lock(mutex_); + auto it = map_.find(key); + return it == map_.end() ? v8::Nothing() : v8::Just(it->second); +} + +MaybeLocal MapKVStore::Get(Isolate* isolate, Local key) const { Utf8Value str(isolate, key); - auto it = map_.find(std::string(*str, str.length())); - if (it == map_.end()) return Local(); - return String::NewFromUtf8(isolate, it->second.data(), - NewStringType::kNormal, it->second.size()); + Maybe value = Get(*str); + if (value.IsNothing()) return Local(); + std::string val = value.FromJust(); + return String::NewFromUtf8( + isolate, val.data(), NewStringType::kNormal, val.size()); } void MapKVStore::Set(Isolate* isolate, Local key, Local value) { @@ -209,11 +232,14 @@ void MapKVStore::Set(Isolate* isolate, Local key, Local value) { } } -int32_t MapKVStore::Query(Isolate* isolate, Local key) const { +int32_t MapKVStore::Query(const char* key) const { Mutex::ScopedLock lock(mutex_); + return map_.find(key) == map_.end() ? -1 : 0; +} + +int32_t MapKVStore::Query(Isolate* isolate, Local key) const { Utf8Value str(isolate, key); - auto it = map_.find(std::string(*str, str.length())); - return it == map_.end() ? -1 : 0; + return Query(*str); } void MapKVStore::Delete(Isolate* isolate, Local key) { From d38ed6ce4b67e86764aef5be257c729530a595fc Mon Sep 17 00:00:00 2001 From: Denys Otrishko Date: Thu, 13 Feb 2020 21:42:42 +0200 Subject: [PATCH 2/2] src: simplify node_worker.cc using new KVStore API --- src/node_worker.cc | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/node_worker.cc b/src/node_worker.cc index 22c69ea6fb0690..969bbb52ab8dcc 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -31,6 +31,7 @@ using v8::Integer; using v8::Isolate; using v8::Local; using v8::Locker; +using v8::Maybe; using v8::MaybeLocal; using v8::Null; using v8::Number; @@ -482,14 +483,9 @@ void Worker::New(const FunctionCallbackInfo& args) { if (args[1]->IsObject() || args[2]->IsArray()) { per_isolate_opts.reset(new PerIsolateOptions()); - HandleEnvOptions( - per_isolate_opts->per_env, [isolate, &env_vars](const char* name) { - MaybeLocal value = - env_vars->Get(isolate, OneByteString(isolate, name)); - return value.IsEmpty() ? std::string{} - : std::string(*String::Utf8Value( - isolate, value.ToLocalChecked())); - }); + HandleEnvOptions(per_isolate_opts->per_env, [&env_vars](const char* name) { + return env_vars->Get(name).FromMaybe(""); + }); #ifndef NODE_WITHOUT_NODE_OPTIONS MaybeLocal maybe_node_opts =