From 4d6d7ddfae65ccd2e1c868915eed4aee81a8385f Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 10 Nov 2015 02:58:51 -0700 Subject: [PATCH 1/6] node: fix erroneously named function call The initial implementation of setPropByIndex() set the value of an Array by index during development. Though the final form of the function simply pushes passed values to an array as passed by arguments. Thus the functions have been renamed to pushValueToArray() and push_values_to_array_function() respectively. Also add define for maximum number of arguments should be used before hitting the limit of performance increase. Fixes: 494227b "node: improve GetActiveRequests performance" --- src/env.h | 12 +++++++++--- src/node.cc | 4 ++-- src/node.js | 4 ++-- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/env.h b/src/env.h index 93f7c47c9919a3..8599e1495eed20 100644 --- a/src/env.h +++ b/src/env.h @@ -38,6 +38,12 @@ namespace node { #define NODE_ISOLATE_SLOT 3 #endif +// The number of items passed to push_values_to_array_function has diminishing +// returns around 8. This should be used at all call sites using said function. +#ifndef NODE_PUSH_VAL_TO_ARRAY_MAX +#define NODE_PUSH_VAL_TO_ARRAY_MAX 8 +#endif + // Strings are per-isolate primitives but Environment proxies them // for the sake of convenience. Strings should be ASCII-only. #define PER_ISOLATE_STRING_PROPERTIES(V) \ @@ -231,12 +237,11 @@ namespace node { V(zero_return_string, "ZERO_RETURN") \ #define ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) \ - V(add_properties_by_index_function, v8::Function) \ V(as_external, v8::External) \ + V(async_hooks_destroy_function, v8::Function) \ V(async_hooks_init_function, v8::Function) \ - V(async_hooks_pre_function, v8::Function) \ V(async_hooks_post_function, v8::Function) \ - V(async_hooks_destroy_function, v8::Function) \ + V(async_hooks_pre_function, v8::Function) \ V(binding_cache_object, v8::Object) \ V(buffer_constructor_function, v8::Function) \ V(buffer_prototype_object, v8::Object) \ @@ -249,6 +254,7 @@ namespace node { V(pipe_constructor_template, v8::FunctionTemplate) \ V(process_object, v8::Object) \ V(promise_reject_function, v8::Function) \ + V(push_values_to_array_function, v8::Function) \ V(script_context_constructor_template, v8::FunctionTemplate) \ V(script_data_constructor_function, v8::Function) \ V(secure_context_constructor_template, v8::FunctionTemplate) \ diff --git a/src/node.cc b/src/node.cc index 4e591ff5af6053..f8e4ff743703d4 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1027,7 +1027,7 @@ void SetupProcessObject(const FunctionCallbackInfo& args) { CHECK(args[0]->IsFunction()); - env->set_add_properties_by_index_function(args[0].As()); + env->set_push_values_to_array_function(args[0].As()); env->process_object()->Delete( FIXED_ONE_BYTE_STRING(env->isolate(), "_setupProcessObject")); } @@ -1571,7 +1571,7 @@ static void GetActiveRequests(const FunctionCallbackInfo& args) { Local ary = Array::New(args.GetIsolate()); Local ctx = env->context(); - Local fn = env->add_properties_by_index_function(); + Local fn = env->push_values_to_array_function(); static const size_t argc = 8; Local argv[argc]; size_t i = 0; diff --git a/src/node.js b/src/node.js index 35a51c80640ab5..fb7c0e39579ca0 100644 --- a/src/node.js +++ b/src/node.js @@ -178,9 +178,9 @@ } startup.setupProcessObject = function() { - process._setupProcessObject(setPropByIndex); + process._setupProcessObject(pushValueToArray); - function setPropByIndex() { + function pushValueToArray() { for (var i = 0; i < arguments.length; i++) this.push(arguments[i]); } From 5af27f6e2349230256afa211479c3dffc54c3025 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 10 Nov 2015 03:04:26 -0700 Subject: [PATCH 2/6] http_parser: use pushValueToArray for headers For performance add headers to the headers Array by pushing them on from JS. Benchmark added to demonstrate this case. --- benchmark/http/bench-parser.js | 55 ++++++++++++++++++++++++++++++++++ src/node_http_parser.cc | 23 ++++++++++---- 2 files changed, 72 insertions(+), 6 deletions(-) create mode 100644 benchmark/http/bench-parser.js diff --git a/benchmark/http/bench-parser.js b/benchmark/http/bench-parser.js new file mode 100644 index 00000000000000..989d9a994fa04e --- /dev/null +++ b/benchmark/http/bench-parser.js @@ -0,0 +1,55 @@ +'use strict'; + +const common = require('../common'); +const HTTPParser = process.binding('http_parser').HTTPParser; +const REQUEST = HTTPParser.REQUEST; +const kOnHeaders = HTTPParser.kOnHeaders | 0; +const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0; +const kOnBody = HTTPParser.kOnBody | 0; +const kOnMessageComplete = HTTPParser.kOnMessageComplete | 0; +const CRLF = '\r\n'; + +const bench = common.createBenchmark(main, { + fields: [4, 8, 16, 32], + n: [1e5], +}); + + +function main(conf) { + const fields = conf.fields >>> 0; + const n = conf.n >>> 0; + var header = `GET /hello HTTP/1.1${CRLF}Content-Type: text/plain${CRLF}`; + + for (var i = 0; i < fields; i++) { + header += `X-Filler${i}: ${Math.random().toString(36).substr(2)}${CRLF}`; + } + header += CRLF; + + processHeader(new Buffer(header), n); +} + + +function processHeader(header, n) { + const parser = newParser(REQUEST); + + bench.start(); + for (var i = 0; i < n; i++) { + parser.execute(header, 0, header.length); + parser.reinitialize(REQUEST); + } + bench.end(n); +} + + +function newParser(type) { + const parser = new HTTPParser(type); + + parser.headers = []; + + parser[kOnHeaders] = function() { }; + parser[kOnHeadersComplete] = function() { }; + parser[kOnBody] = function() { }; + parser[kOnMessageComplete] = function() { }; + + return parser; +} diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index ff3dfb26e529af..28322f95c40939 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -632,12 +632,23 @@ class Parser : public BaseObject { Local CreateHeaders() { // num_values_ is either -1 or the entry # of the last header // so num_values_ == 0 means there's a single header - Local headers = Array::New(env()->isolate(), 2 * num_values_); - - for (int i = 0; i < num_values_; ++i) { - headers->Set(2 * i, fields_[i].ToString(env())); - headers->Set(2 * i + 1, values_[i].ToString(env())); - } + Local headers = Array::New(env()->isolate()); + Local fn = env()->push_values_to_array_function(); + Local argv[NODE_PUSH_VAL_TO_ARRAY_MAX * 2]; + int i = 0; + + do { + size_t j = 0; + while (i < num_values_ && j < ARRAY_SIZE(argv) / 2) { + argv[j * 2] = fields_[i].ToString(env()); + argv[j * 2 + 1] = values_[i].ToString(env()); + i++; + j++; + } + if (j > 0) { + fn->Call(env()->context(), headers, j * 2, argv).ToLocalChecked(); + } + } while (i < num_values_); return headers; } From f229b67f0da045f62a5bfa77f299ddb9a331313e Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 10 Nov 2015 16:36:50 -0700 Subject: [PATCH 3/6] fs: use pushValueToArray for readdir(Sync) Improve performance by pushing directory entries to returned array in batches of 8 using pushValueToArray() in JS. Add benchmarks to demonstrate this improvement. --- benchmark/fs/bench-readdir.js | 22 ++++++++++++++++++ benchmark/fs/bench-readdirSync.js | 19 ++++++++++++++++ src/node_file.cc | 37 ++++++++++++++++++++++++++----- 3 files changed, 72 insertions(+), 6 deletions(-) create mode 100644 benchmark/fs/bench-readdir.js create mode 100644 benchmark/fs/bench-readdirSync.js diff --git a/benchmark/fs/bench-readdir.js b/benchmark/fs/bench-readdir.js new file mode 100644 index 00000000000000..2f0eab6a821f42 --- /dev/null +++ b/benchmark/fs/bench-readdir.js @@ -0,0 +1,22 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); + +const bench = common.createBenchmark(main, { + n: [1e4], +}); + + +function main(conf) { + const n = conf.n >>> 0; + + bench.start(); + (function r(cntr) { + if (--cntr <= 0) + return bench.end(n); + fs.readdir(__dirname + '/../../lib/', function() { + r(cntr); + }); + }(n)); +} diff --git a/benchmark/fs/bench-readdirSync.js b/benchmark/fs/bench-readdirSync.js new file mode 100644 index 00000000000000..9f89649138cd20 --- /dev/null +++ b/benchmark/fs/bench-readdirSync.js @@ -0,0 +1,19 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); + +const bench = common.createBenchmark(main, { + n: [1e4], +}); + + +function main(conf) { + const n = conf.n >>> 0; + + bench.start(); + for (var i = 0; i < n; i++) { + fs.readdirSync(__dirname + '/../../lib/'); + } + bench.end(n); +} diff --git a/src/node_file.cc b/src/node_file.cc index 2bd32d7daa2bd0..4941ab4467435b 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -214,6 +214,9 @@ static void After(uv_fs_t *req) { { int r; Local names = Array::New(env->isolate(), 0); + Local fn = env->push_values_to_array_function(); + Local name_argv[NODE_PUSH_VAL_TO_ARRAY_MAX]; + size_t name_idx = 0; for (int i = 0; ; i++) { uv_dirent_t ent; @@ -229,9 +232,19 @@ static void After(uv_fs_t *req) { break; } - Local name = String::NewFromUtf8(env->isolate(), - ent.name); - names->Set(i, name); + name_argv[name_idx++] = + String::NewFromUtf8(env->isolate(), ent.name); + + if (name_idx >= ARRAY_SIZE(name_argv)) { + fn->Call(env->context(), names, name_idx, name_argv) + .ToLocalChecked(); + name_idx = 0; + } + } + + if (name_idx > 0) { + fn->Call(env->context(), names, name_idx, name_argv) + .ToLocalChecked(); } argv[1] = names; @@ -811,6 +824,9 @@ static void ReadDir(const FunctionCallbackInfo& args) { CHECK_GE(SYNC_REQ.result, 0); int r; Local names = Array::New(env->isolate(), 0); + Local fn = env->push_values_to_array_function(); + Local name_v[NODE_PUSH_VAL_TO_ARRAY_MAX]; + size_t name_idx = 0; for (int i = 0; ; i++) { uv_dirent_t ent; @@ -821,9 +837,18 @@ static void ReadDir(const FunctionCallbackInfo& args) { if (r != 0) return env->ThrowUVException(r, "readdir", "", *path); - Local name = String::NewFromUtf8(env->isolate(), - ent.name); - names->Set(i, name); + + name_v[name_idx++] = String::NewFromUtf8(env->isolate(), ent.name); + + if (name_idx >= ARRAY_SIZE(name_v)) { + fn->Call(env->context(), names, name_idx, name_v) + .ToLocalChecked(); + name_idx = 0; + } + } + + if (name_idx > 0) { + fn->Call(env->context(), names, name_idx, name_v).ToLocalChecked(); } args.GetReturnValue().Set(names); From f3f59a402379ea6d97cfc3e4a2e33900e896a7d1 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 10 Nov 2015 17:04:56 -0700 Subject: [PATCH 4/6] node: improve GetActiveHandles performance Improve performance of process._getActiveHandles by sending handles in batches to JS to be set on the passed Array. Add test to check proper active handles are returned. Alter implementation of GetActiveRequests to match GetActiveHandles' implementation. --- src/node.cc | 40 +++++++++-------- .../parallel/test-process-getactivehandles.js | 44 +++++++++++++++++++ 2 files changed, 66 insertions(+), 18 deletions(-) create mode 100644 test/parallel/test-process-getactivehandles.js diff --git a/src/node.cc b/src/node.cc index f8e4ff743703d4..2266271e2dc736 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1572,27 +1572,21 @@ static void GetActiveRequests(const FunctionCallbackInfo& args) { Local ary = Array::New(args.GetIsolate()); Local ctx = env->context(); Local fn = env->push_values_to_array_function(); - static const size_t argc = 8; - Local argv[argc]; - size_t i = 0; + Local argv[NODE_PUSH_VAL_TO_ARRAY_MAX]; + size_t idx = 0; for (auto w : *env->req_wrap_queue()) { - if (w->persistent().IsEmpty() == false) { - argv[i++ % argc] = w->object(); - if ((i % argc) == 0) { - HandleScope scope(env->isolate()); - fn->Call(ctx, ary, argc, argv).ToLocalChecked(); - for (auto&& arg : argv) { - arg = Local(); - } - } + if (w->persistent().IsEmpty()) + continue; + argv[idx] = w->object(); + if (++idx >= ARRAY_SIZE(argv)) { + fn->Call(ctx, ary, idx, argv).ToLocalChecked(); + idx = 0; } } - const size_t remainder = i % argc; - if (remainder > 0) { - HandleScope scope(env->isolate()); - fn->Call(ctx, ary, remainder, argv).ToLocalChecked(); + if (idx > 0) { + fn->Call(ctx, ary, idx, argv).ToLocalChecked(); } args.GetReturnValue().Set(ary); @@ -1605,7 +1599,10 @@ void GetActiveHandles(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Local ary = Array::New(env->isolate()); - int i = 0; + Local ctx = env->context(); + Local fn = env->push_values_to_array_function(); + Local argv[NODE_PUSH_VAL_TO_ARRAY_MAX]; + size_t idx = 0; Local owner_sym = env->owner_string(); @@ -1616,7 +1613,14 @@ void GetActiveHandles(const FunctionCallbackInfo& args) { Local owner = object->Get(owner_sym); if (owner->IsUndefined()) owner = object; - ary->Set(i++, owner); + argv[idx] = owner; + if (++idx >= ARRAY_SIZE(argv)) { + fn->Call(ctx, ary, idx, argv).ToLocalChecked(); + idx = 0; + } + } + if (idx > 0) { + fn->Call(ctx, ary, idx, argv).ToLocalChecked(); } args.GetReturnValue().Set(ary); diff --git a/test/parallel/test-process-getactivehandles.js b/test/parallel/test-process-getactivehandles.js new file mode 100644 index 00000000000000..96464cf3b22fcd --- /dev/null +++ b/test/parallel/test-process-getactivehandles.js @@ -0,0 +1,44 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const net = require('net'); +const NUM = 8; +const connections = []; +const clients = []; +var clients_counter = 0; + +const server = net.createServer(function listener(c) { + connections.push(c); +}).listen(common.PORT, function makeConnections() { + for (var i = 0; i < NUM; i++) { + net.connect(common.PORT, function connected() { + clientConnected(this); + }); + } +}); + + +function clientConnected(client) { + clients.push(client); + if (++clients_counter >= NUM) + checkAll(); +} + + +function checkAll() { + const handles = process._getActiveHandles(); + + clients.forEach(function(item) { + assert.ok(handles.indexOf(item) > -1); + item.destroy(); + }); + + connections.forEach(function(item) { + assert.ok(handles.indexOf(item) > -1); + item.end(); + }); + + assert.ok(handles.indexOf(server) > -1); + server.close(); +} From fc143da6f1def7bd337cc4a255ef8013f2de6517 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 11 Nov 2015 00:15:15 -0700 Subject: [PATCH 5/6] node: improve performance of hrtime() process.hrtime() was performing too many operations in C++ that could be done faster in JS. Move those operations over by creating a length 4 Uint32Array and perform bitwise operations on the seconds so that it was unnecessary for the native API to do any object creation or set any fields. This has improved performance from ~350 ns/op to ~65 ns/op. Light benchmark included to demonstrate the performance change. --- benchmark/misc/bench-hrtime.js | 18 ++++++++++++++++++ src/node.cc | 25 +++++++++++++------------ src/node.js | 15 +++++++++++++++ 3 files changed, 46 insertions(+), 12 deletions(-) create mode 100644 benchmark/misc/bench-hrtime.js diff --git a/benchmark/misc/bench-hrtime.js b/benchmark/misc/bench-hrtime.js new file mode 100644 index 00000000000000..661dff43b0103c --- /dev/null +++ b/benchmark/misc/bench-hrtime.js @@ -0,0 +1,18 @@ +'use strict'; + +const common = require('../common'); + +const bench = common.createBenchmark(main, { + n: [1e6] +}); + + +function main(conf) { + const n = conf.n >>> 0; + + bench.start(); + for (var i = 0; i < n; i++) { + process.hrtime(); + } + bench.end(n); +} diff --git a/src/node.cc b/src/node.cc index 2266271e2dc736..e976eea3827a6d 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2096,22 +2096,23 @@ void Hrtime(const FunctionCallbackInfo& args) { uint64_t t = uv_hrtime(); - if (args.Length() > 0) { - // return a time diff tuple - if (!args[0]->IsArray()) { + if (!args[1]->IsUndefined()) { + if (!args[1]->IsArray()) { return env->ThrowTypeError( - "process.hrtime() only accepts an Array tuple."); + "process.hrtime() only accepts an Array tuple"); } - Local inArray = Local::Cast(args[0]); - uint64_t seconds = inArray->Get(0)->Uint32Value(); - uint64_t nanos = inArray->Get(1)->Uint32Value(); - t -= (seconds * NANOS_PER_SEC) + nanos; + args.GetReturnValue().Set(true); } - Local tuple = Array::New(env->isolate(), 2); - tuple->Set(0, Integer::NewFromUnsigned(env->isolate(), t / NANOS_PER_SEC)); - tuple->Set(1, Integer::NewFromUnsigned(env->isolate(), t % NANOS_PER_SEC)); - args.GetReturnValue().Set(tuple); + Local ab = args[0].As()->Buffer(); + uint32_t* fields = static_cast(ab->GetContents().Data()); + + // These three indices will contain the values for the hrtime tuple. The + // seconds value is broken into the upper/lower 32 bits and stored in two + // uint32 fields to be converted back in JS. + fields[0] = (t / NANOS_PER_SEC) >> 32; + fields[1] = (t / NANOS_PER_SEC) & 0xffffffff; + fields[2] = t % NANOS_PER_SEC; } extern "C" void node_module_register(void* m) { diff --git a/src/node.js b/src/node.js index fb7c0e39579ca0..71b65497f4ba38 100644 --- a/src/node.js +++ b/src/node.js @@ -178,12 +178,27 @@ } startup.setupProcessObject = function() { + const _hrtime = process.hrtime; + const hrValues = new Uint32Array(3); + process._setupProcessObject(pushValueToArray); function pushValueToArray() { for (var i = 0; i < arguments.length; i++) this.push(arguments[i]); } + + process.hrtime = function hrtime(ar) { + const ret = [0, 0]; + if (_hrtime(hrValues, ar)) { + ret[0] = (hrValues[0] * 0x100000000 + hrValues[1]) - ar[0]; + ret[1] = hrValues[2] - ar[1]; + } else { + ret[0] = hrValues[0] * 0x100000000 + hrValues[1]; + ret[1] = hrValues[2]; + } + return ret; + }; }; startup.globalVariables = function() { From 799b4409a419b97174d8b033633c0a1fa1826328 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 11 Nov 2015 12:28:41 -0700 Subject: [PATCH 6/6] node: improve accessor perf of process.env Set process.env array entries in JS. --- benchmark/misc/bench-env.js | 18 ++++++++++++++++ src/node.cc | 43 ++++++++++++++++++++++++++----------- 2 files changed, 48 insertions(+), 13 deletions(-) create mode 100644 benchmark/misc/bench-env.js diff --git a/benchmark/misc/bench-env.js b/benchmark/misc/bench-env.js new file mode 100644 index 00000000000000..66f966f587bb7f --- /dev/null +++ b/benchmark/misc/bench-env.js @@ -0,0 +1,18 @@ +'use strict'; + +const common = require('../common'); + +const bench = common.createBenchmark(main, { + n: [1e5], +}); + + +function main(conf) { + const n = conf.n >>> 0; + bench.start(); + for (var i = 0; i < n; i++) { + // Access every item in object to process values. + Object.keys(process.env); + } + bench.end(n); +} diff --git a/src/node.cc b/src/node.cc index e976eea3827a6d..321a2857a112c4 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2524,23 +2524,35 @@ static void EnvDeleter(Local property, static void EnvEnumerator(const PropertyCallbackInfo& info) { - Isolate* isolate = info.GetIsolate(); + Environment* env = Environment::GetCurrent(info); + Isolate* isolate = env->isolate(); + Local ctx = env->context(); + Local fn = env->push_values_to_array_function(); + Local argv[NODE_PUSH_VAL_TO_ARRAY_MAX]; + size_t idx = 0; + #ifdef __POSIX__ int size = 0; while (environ[size]) size++; - Local envarr = Array::New(isolate, size); + Local envarr = Array::New(isolate); for (int i = 0; i < size; ++i) { const char* var = environ[i]; const char* s = strchr(var, '='); const int length = s ? s - var : strlen(var); - Local name = String::NewFromUtf8(isolate, - var, - String::kNormalString, - length); - envarr->Set(i, name); + argv[idx] = String::NewFromUtf8(isolate, + var, + String::kNormalString, + length); + if (++idx >= ARRAY_SIZE(argv)) { + fn->Call(ctx, envarr, idx, argv).ToLocalChecked(); + idx = 0; + } + } + if (idx > 0) { + fn->Call(ctx, envarr, idx, argv).ToLocalChecked(); } #else // _WIN32 WCHAR* environment = GetEnvironmentStringsW(); @@ -2548,7 +2560,6 @@ static void EnvEnumerator(const PropertyCallbackInfo& info) { return; // This should not happen. Local envarr = Array::New(isolate); WCHAR* p = environment; - int i = 0; while (*p) { WCHAR *s; if (*p == L'=') { @@ -2563,13 +2574,19 @@ static void EnvEnumerator(const PropertyCallbackInfo& info) { } const uint16_t* two_byte_buffer = reinterpret_cast(p); const size_t two_byte_buffer_len = s - p; - Local value = String::NewFromTwoByte(isolate, - two_byte_buffer, - String::kNormalString, - two_byte_buffer_len); - envarr->Set(i++, value); + argv[idx] = String::NewFromTwoByte(isolate, + two_byte_buffer, + String::kNormalString, + two_byte_buffer_len); + if (++idx >= ARRAY_SIZE(argv)) { + fn->Call(ctx, envarr, idx, argv).ToLocalChecked(); + idx = 0; + } p = s + wcslen(s) + 1; } + if (idx > 0) { + fn->Call(ctx, envarr, idx, argv).ToLocalChecked(); + } FreeEnvironmentStringsW(environment); #endif