From ef68e5834473841eb7693227595119a9686dc5ed Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 17 Aug 2019 23:01:10 +0200 Subject: [PATCH 1/4] benchmark: improve process.env benchmarks Benchmark different types of operations and make results comparable by normalizing process.env for enumeartion. --- benchmark/process/bench-env.js | 54 +++++++++++++++++++++--- test/benchmark/test-benchmark-process.js | 3 +- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/benchmark/process/bench-env.js b/benchmark/process/bench-env.js index a332d3cbd61895..5df521cc958389 100644 --- a/benchmark/process/bench-env.js +++ b/benchmark/process/bench-env.js @@ -3,15 +3,55 @@ const common = require('../common'); const bench = common.createBenchmark(main, { - n: [1e5], + n: [1e6], + operation: ['get', 'set', 'enumerate', 'query', 'delete'] }); -function main({ n }) { - bench.start(); - for (var i = 0; i < n; i++) { - // Access every item in object to process values. - Object.keys(process.env); +function main({ n, operation }) { + switch (operation) { + case 'get': + bench.start(); + for (let i = 0; i < n; i++) { + process.env.PATH; + } + bench.end(n); + break; + case 'set': + bench.start(); + for (let i = 0; i < n; i++) { + process.env.DUMMY = 'hello, world'; + } + bench.end(n); + break; + case 'enumerate': + // First, normalize process.env so that benchmark results are comparable. + for (const key of Object.keys(process.env)) + delete process.env[key]; + for (let i = 0; i < 64; i++) + process.env[Math.random()] = Math.random(); + + n /= 10; // Enumeration is comparatively heavy. + bench.start(); + for (let i = 0; i < n; i++) { + // Access every item in object to process values. + Object.keys(process.env); + } + bench.end(n); + break; + case 'query': + bench.start(); + for (let i = 0; i < n; i++) { + 'PATH' in process.env; + } + bench.end(n); + break; + case 'delete': + bench.start(); + for (let i = 0; i < n; i++) { + delete process.env.DUMMY; + } + bench.end(n); + break; } - bench.end(n); } diff --git a/test/benchmark/test-benchmark-process.js b/test/benchmark/test-benchmark-process.js index 15cb678017e7bb..a73fc075bfcfa6 100644 --- a/test/benchmark/test-benchmark-process.js +++ b/test/benchmark/test-benchmark-process.js @@ -7,5 +7,6 @@ const runBenchmark = require('../common/benchmark'); runBenchmark('process', [ 'n=1', - 'type=raw' + 'type=raw', + 'operation=enumerate', ], { NODEJS_BENCHMARK_ZERO_ALLOWED: 1 }); From 02fdce658d9de2b52ceab49cdd888c242c8ceca5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 17 Aug 2019 23:01:34 +0200 Subject: [PATCH 2/4] src: use libuv to get env vars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows us to remove OS-dependent code. confidence improvement accuracy (*) (**) (***) process/bench-env.js operation='delete' n=1000000 3.57 % ±10.86% ±14.46% ±18.85% process/bench-env.js operation='enumerate' n=1000000 *** -14.06 % ±7.46% ±9.94% ±12.96% process/bench-env.js operation='get' n=1000000 -7.97 % ±11.80% ±15.70% ±20.45% process/bench-env.js operation='query' n=1000000 -1.32 % ±8.38% ±11.17% ±14.58% process/bench-env.js operation='set' n=1000000 -0.98 % ±9.63% ±12.81% ±16.68% The drop in enumeration performance is likely due to the large number of extra allocations that libuv performs. However, enumerating process.env should generally not be a hot path in most applications. --- src/node_env_var.cc | 81 ++++++++++++++------------------------------- 1 file changed, 25 insertions(+), 56 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 2c1af65b867b0b..f1ebee61e1579d 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -4,13 +4,6 @@ #include // tzset(), _tzset() -#ifdef __APPLE__ -#include -#define environ (*_NSGetEnviron()) -#elif !defined(_MSC_VER) -extern char** environ; -#endif - namespace node { using v8::Array; using v8::Boolean; @@ -123,12 +116,6 @@ int32_t RealEnvStore::Query(Isolate* isolate, Local property) const { Mutex::ScopedLock lock(per_process::env_var_mutex); node::Utf8Value key(isolate, property); -#ifdef _WIN32 - if (key[0] == L'=') - return static_cast(v8::ReadOnly) | - static_cast(v8::DontDelete) | - static_cast(v8::DontEnum); -#endif char val[2]; size_t init_sz = sizeof(val); @@ -138,6 +125,14 @@ int32_t RealEnvStore::Query(Isolate* isolate, Local property) const { return -1; } +#ifdef _WIN32 + if (key[0] == L'=') { + return static_cast(v8::ReadOnly) | + static_cast(v8::DontDelete) | + static_cast(v8::DontEnum); + } +#endif + return 0; } @@ -151,54 +146,28 @@ void RealEnvStore::Delete(Isolate* isolate, Local property) { Local RealEnvStore::Enumerate(Isolate* isolate) const { Mutex::ScopedLock lock(per_process::env_var_mutex); -#ifdef __POSIX__ - int env_size = 0; - while (environ[env_size]) { - env_size++; - } - std::vector> env_v(env_size); - - for (int i = 0; i < env_size; ++i) { - const char* var = environ[i]; - const char* s = strchr(var, '='); - const int length = s ? s - var : strlen(var); - env_v[i] = String::NewFromUtf8(isolate, var, NewStringType::kNormal, length) - .ToLocalChecked(); - } -#else // _WIN32 - std::vector> env_v; - WCHAR* environment = GetEnvironmentStringsW(); - if (environment == nullptr) - return Array::New(isolate); // This should not happen. - WCHAR* p = environment; - while (*p) { - WCHAR* s; - if (*p == L'=') { - // If the key starts with '=' it is a hidden environment variable. - p += wcslen(p) + 1; - continue; - } else { - s = wcschr(p, L'='); - } - if (!s) { - s = p + wcslen(p); - } - const uint16_t* two_byte_buffer = reinterpret_cast(p); - const size_t two_byte_buffer_len = s - p; - v8::MaybeLocal rc = String::NewFromTwoByte( - isolate, two_byte_buffer, NewStringType::kNormal, two_byte_buffer_len); - if (rc.IsEmpty()) { + uv_env_item_t* items; + int count; + + OnScopeLeave cleanup([&]() { uv_os_free_environ(items, count); }); + CHECK_EQ(uv_os_environ(&items, &count), 0); + + MaybeStackBuffer, 256> env_v(count); + for (int i = 0; i < count; i++) { +#ifdef _WIN32 + // If the key starts with '=' it is a hidden environment variable. + if (items[i].name[0] == '=') continue; +#endif + MaybeLocal str = String::NewFromUtf8( + isolate, items[i].name, NewStringType::kNormal); + if (str.IsEmpty()) { isolate->ThrowException(ERR_STRING_TOO_LONG(isolate)); - FreeEnvironmentStringsW(environment); return Local(); } - env_v.push_back(rc.ToLocalChecked()); - p = s + wcslen(s) + 1; + env_v[i] = str.ToLocalChecked(); } - FreeEnvironmentStringsW(environment); -#endif - return Array::New(isolate, env_v.data(), env_v.size()); + return Array::New(isolate, env_v.out(), env_v.length()); } std::shared_ptr KVStore::Clone(v8::Isolate* isolate) const { From e75c72e1f446d99179c3a6cb1dc3c8acc0817ed4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 13 Sep 2019 22:51:31 +0200 Subject: [PATCH 3/4] fixup! src: use libuv to get env vars --- src/node_env_var.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index f1ebee61e1579d..3664c91e600154 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -156,7 +156,9 @@ Local RealEnvStore::Enumerate(Isolate* isolate) const { for (int i = 0; i < count; i++) { #ifdef _WIN32 // If the key starts with '=' it is a hidden environment variable. - if (items[i].name[0] == '=') continue; + // The '\0' check is a workaround for the bug behind + // https://github.com/libuv/libuv/pull/2473 and can be removed later. + if (items[i].name[0] == '=' || items[i].name[0] == '\0') continue; #endif MaybeLocal str = String::NewFromUtf8( isolate, items[i].name, NewStringType::kNormal); From eed28843b772f5ec8483415023b1793a90611544 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 18 Sep 2019 19:45:45 +0200 Subject: [PATCH 4/4] fixup! fixup! src: use libuv to get env vars --- src/node_env_var.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 3664c91e600154..c63cb2c37fb3e0 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -153,6 +153,7 @@ Local RealEnvStore::Enumerate(Isolate* isolate) const { CHECK_EQ(uv_os_environ(&items, &count), 0); MaybeStackBuffer, 256> env_v(count); + int env_v_index = 0; for (int i = 0; i < count; i++) { #ifdef _WIN32 // If the key starts with '=' it is a hidden environment variable. @@ -166,10 +167,10 @@ Local RealEnvStore::Enumerate(Isolate* isolate) const { isolate->ThrowException(ERR_STRING_TOO_LONG(isolate)); return Local(); } - env_v[i] = str.ToLocalChecked(); + env_v[env_v_index++] = str.ToLocalChecked(); } - return Array::New(isolate, env_v.out(), env_v.length()); + return Array::New(isolate, env_v.out(), env_v_index); } std::shared_ptr KVStore::Clone(v8::Isolate* isolate) const {