From 494227bb03df3fff4a59b58f86498ab262e7d46d Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Wed, 14 Oct 2015 14:58:52 -0600
Subject: [PATCH] node: improve GetActiveRequests performance

v8 is faster at setting object properties in JS than C++. Even when it
requires calling into JS from native code. Make
process._getActiveRequests() faster by doing this when populating the
array containing request objects.

Simple benchmark:

    for (let i = 0; i < 22; i++)
      fs.open(__filename, 'r', function() { });
    let t = process.hrtime();
    for (let i = 0; i < 1e6; i++)
      process._getActiveRequests();
    t = process.hrtime(t);
    console.log((t[0] * 1e9 + t[1]) / 1e6);

Results between the two:

    Previous:  4406 ns/op
    Patched:    690 ns/op     5.4x faster

PR-URL: https://github.com/nodejs/node/pull/3375
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
---
 src/env.h                                     |  1 +
 src/node.cc                                   | 39 +++++++++++++++++--
 src/node.js                                   | 11 ++++++
 .../test-process-getactiverequests.js         | 10 +++++
 4 files changed, 57 insertions(+), 4 deletions(-)
 create mode 100644 test/parallel/test-process-getactiverequests.js

diff --git a/src/env.h b/src/env.h
index 97652146fabf0d..b79ef4ae3e6587 100644
--- a/src/env.h
+++ b/src/env.h
@@ -227,6 +227,7 @@ 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_init_function, v8::Function)                                  \
   V(async_hooks_pre_function, v8::Function)                                   \
diff --git a/src/node.cc b/src/node.cc
index 1dfa854ed5bb99..10e7da125d49a7 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1009,6 +1009,17 @@ void RunMicrotasks(const FunctionCallbackInfo<Value>& args) {
 }
 
 
+void SetupProcessObject(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+
+  CHECK(args[0]->IsFunction());
+
+  env->set_add_properties_by_index_function(args[0].As<Function>());
+  env->process_object()->Delete(
+      FIXED_ONE_BYTE_STRING(env->isolate(), "_setupProcessObject"));
+}
+
+
 void SetupNextTick(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
@@ -1546,11 +1557,30 @@ static void GetActiveRequests(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
   Local<Array> ary = Array::New(args.GetIsolate());
-  int i = 0;
+  Local<Context> ctx = env->context();
+  Local<Function> fn = env->add_properties_by_index_function();
+  static const size_t argc = 8;
+  Local<Value> argv[argc];
+  size_t i = 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<Value>();
+        }
+      }
+    }
+  }
 
-  for (auto w : *env->req_wrap_queue())
-    if (w->persistent().IsEmpty() == false)
-      ary->Set(i++, w->object());
+  const size_t remainder = i % argc;
+  if (remainder > 0) {
+    HandleScope scope(env->isolate());
+    fn->Call(ctx, ary, remainder, argv).ToLocalChecked();
+  }
 
   args.GetReturnValue().Set(ary);
 }
@@ -2979,6 +3009,7 @@ void SetupProcessObject(Environment* env,
   env->SetMethod(process, "binding", Binding);
   env->SetMethod(process, "_linkedBinding", LinkedBinding);
 
+  env->SetMethod(process, "_setupProcessObject", SetupProcessObject);
   env->SetMethod(process, "_setupNextTick", SetupNextTick);
   env->SetMethod(process, "_setupPromises", SetupPromises);
   env->SetMethod(process, "_setupDomainUse", SetupDomainUse);
diff --git a/src/node.js b/src/node.js
index 7cfd2c035962a6..d47a129635e25c 100644
--- a/src/node.js
+++ b/src/node.js
@@ -22,6 +22,8 @@
 
     process.EventEmitter = EventEmitter; // process.EventEmitter is deprecated
 
+    startup.setupProcessObject();
+
     // do this good and early, since it handles errors.
     startup.processFatal();
 
@@ -173,6 +175,15 @@
     }
   }
 
+  startup.setupProcessObject = function() {
+    process._setupProcessObject(setPropByIndex);
+
+    function setPropByIndex() {
+      for (var i = 0; i < arguments.length; i++)
+        this.push(arguments[i]);
+    }
+  };
+
   startup.globalVariables = function() {
     global.process = process;
     global.global = global;
diff --git a/test/parallel/test-process-getactiverequests.js b/test/parallel/test-process-getactiverequests.js
new file mode 100644
index 00000000000000..4b7e0df1a50240
--- /dev/null
+++ b/test/parallel/test-process-getactiverequests.js
@@ -0,0 +1,10 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs');
+
+for (let i = 0; i < 12; i++)
+  fs.open(__filename, 'r', function() { });
+
+assert.equal(12, process._getActiveRequests().length);