Commit 6284b49
src: use libuv to get env vars
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.
PR-URL: #29188
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>1 parent b9c7c90 commit 6284b49
1 file changed
Lines changed: 28 additions & 56 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -2,13 +2,6 @@ | |||
| 2 | 2 | #include "node_errors.h" | |
| 3 | 3 | #include "node_process.h" | |
| 4 | 4 | ||
| 5 | - #ifdef __APPLE__ | ||
| 6 | - #include <crt_externs.h> | ||
| 7 | - #define environ (*_NSGetEnviron()) | ||
| 8 | - #elif !defined(_MSC_VER) | ||
| 9 | - extern char** environ; | ||
| 10 | - #endif | ||
| 11 | - | ||
| 12 | 5 | namespace node { | |
| 13 | 6 | using v8::Array; | |
| 14 | 7 | using v8::Boolean; | |
@@ -107,12 +100,6 @@ int32_t RealEnvStore::Query(Isolate* isolate, Local<String> property) const { | |||
| 107 | 100 | Mutex::ScopedLock lock(per_process::env_var_mutex); | |
| 108 | 101 | ||
| 109 | 102 | node::Utf8Value key(isolate, property); | |
| 110 | - #ifdef _WIN32 | ||
| 111 | - if (key[0] == L'=') | ||
| 112 | - return static_cast<int32_t>(v8::ReadOnly) | | ||
| 113 | - static_cast<int32_t>(v8::DontDelete) | | ||
| 114 | - static_cast<int32_t>(v8::DontEnum); | ||
| 115 | - #endif | ||
| 116 | 103 | ||
| 117 | 104 | char val[2]; | |
| 118 | 105 | size_t init_sz = sizeof(val); | |
@@ -122,6 +109,14 @@ int32_t RealEnvStore::Query(Isolate* isolate, Local<String> property) const { | |||
| 122 | 109 | return -1; | |
| 123 | 110 | } | |
| 124 | 111 | ||
| 112 | + #ifdef _WIN32 | ||
| 113 | + if (key[0] == L'=') { | ||
| 114 | + return static_cast<int32_t>(v8::ReadOnly) | | ||
| 115 | + static_cast<int32_t>(v8::DontDelete) | | ||
| 116 | + static_cast<int32_t>(v8::DontEnum); | ||
| 117 | + } | ||
| 118 | + #endif | ||
| 119 | + | ||
| 125 | 120 | return 0; | |
| 126 | 121 | } | |
| 127 | 122 | ||
@@ -134,54 +129,31 @@ void RealEnvStore::Delete(Isolate* isolate, Local<String> property) { | |||
| 134 | 129 | ||
| 135 | 130 | Local<Array> RealEnvStore::Enumerate(Isolate* isolate) const { | |
| 136 | 131 | Mutex::ScopedLock lock(per_process::env_var_mutex); | |
| 137 | - #ifdef __POSIX__ | ||
| 138 | - int env_size = 0; | ||
| 139 | - while (environ[env_size]) { | ||
| 140 | - env_size++; | ||
| 141 | - } | ||
| 142 | - std::vector<Local<Value>> env_v(env_size); | ||
| 143 | - | ||
| 144 | - for (int i = 0; i < env_size; ++i) { | ||
| 145 | - const char* var = environ[i]; | ||
| 146 | - const char* s = strchr(var, '='); | ||
| 147 | - const int length = s ? s - var : strlen(var); | ||
| 148 | - env_v[i] = String::NewFromUtf8(isolate, var, NewStringType::kNormal, length) | ||
| 149 | - .ToLocalChecked(); | ||
| 150 | - } | ||
| 151 | - #else // _WIN32 | ||
| 152 | - std::vector<Local<Value>> env_v; | ||
| 153 | - WCHAR* environment = GetEnvironmentStringsW(); | ||
| 154 | - if (environment == nullptr) | ||
| 155 | - return Array::New(isolate); // This should not happen. | ||
| 156 | - WCHAR* p = environment; | ||
| 157 | - while (*p) { | ||
| 158 | - WCHAR* s; | ||
| 159 | - if (*p == L'=') { | ||
| 160 | - // If the key starts with '=' it is a hidden environment variable. | ||
| 161 | - p += wcslen(p) + 1; | ||
| 162 | - continue; | ||
| 163 | - } else { | ||
| 164 | - s = wcschr(p, L'='); | ||
| 165 | - } | ||
| 166 | - if (!s) { | ||
| 167 | - s = p + wcslen(p); | ||
| 168 | - } | ||
| 169 | - const uint16_t* two_byte_buffer = reinterpret_cast<const uint16_t*>(p); | ||
| 170 | - const size_t two_byte_buffer_len = s - p; | ||
| 171 | - v8::MaybeLocal<String> rc = String::NewFromTwoByte( | ||
| 172 | - isolate, two_byte_buffer, NewStringType::kNormal, two_byte_buffer_len); | ||
| 173 | - if (rc.IsEmpty()) { | ||
| 132 | + uv_env_item_t* items; | ||
| 133 | + int count; | ||
| 134 | + | ||
| 135 | + OnScopeLeave cleanup([&]() { uv_os_free_environ(items, count); }); | ||
| 136 | + CHECK_EQ(uv_os_environ(&items, &count), 0); | ||
| 137 | + | ||
| 138 | + MaybeStackBuffer<Local<Value>, 256> env_v(count); | ||
| 139 | + int env_v_index = 0; | ||
| 140 | + for (int i = 0; i < count; i++) { | ||
| 141 | + #ifdef _WIN32 | ||
| 142 | + // If the key starts with '=' it is a hidden environment variable. | ||
| 143 | + // The '\0' check is a workaround for the bug behind | ||
| 144 | + // https://github.com/libuv/libuv/pull/2473 and can be removed later. | ||
| 145 | + if (items[i].name[0] == '=' || items[i].name[0] == '\0') continue; | ||
| 146 | + #endif | ||
| 147 | + MaybeLocal<String> str = String::NewFromUtf8( | ||
| 148 | + isolate, items[i].name, NewStringType::kNormal); | ||
| 149 | + if (str.IsEmpty()) { | ||
| 174 | 150 | isolate->ThrowException(ERR_STRING_TOO_LONG(isolate)); | |
| 175 | - FreeEnvironmentStringsW(environment); | ||
| 176 | 151 | return Local<Array>(); | |
| 177 | 152 | } | |
| 178 | - env_v.push_back(rc.ToLocalChecked()); | ||
| 179 | - p = s + wcslen(s) + 1; | ||
| 153 | + env_v[env_v_index++] = str.ToLocalChecked(); | ||
| 180 | 154 | } | |
| 181 | - FreeEnvironmentStringsW(environment); | ||
| 182 | - #endif | ||
| 183 | 155 | ||
| 184 | - return Array::New(isolate, env_v.data(), env_v.size()); | ||
| 156 | + return Array::New(isolate, env_v.out(), env_v_index); | ||
| 185 | 157 | } | |
| 186 | 158 | ||
| 187 | 159 | std::shared_ptr<KVStore> KVStore::Clone(v8::Isolate* isolate) const { | |
0 commit comments