timers: refactor timer list processing

Instead of using kOnTimeout index to track a special list
processing function, just pass in a function to C++ at
startup that executes all handles and determines which
function to call.

This change improves the performance of unpooled timeouts
by roughly 20%, as well as makes the unref/ref processing
easier to follow.

PR-URL: #18582
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>

Author: apapirovski

benchmark/timers/timers-timeout-unpooled.js

@@ -0,0 +1,36 @@
+'use strict';
+const common = require('../common.js');
+
+// The following benchmark sets up n * 1e6 unpooled timeouts,
+// then measures their execution on the next uv tick
+
+const bench = common.createBenchmark(main, {
+  n: [1e6],
+});
+
+function main({ n }) {
+  let count = 0;
+
+  // Function tracking on the hidden class in V8 can cause misleading
+  // results in this benchmark if only a single function is used —
+  // alternate between two functions for a fairer benchmark
+
+  function cb() {
+    count++;
+    if (count === n)
+      bench.end(n);
+  }
+  function cb2() {
+    count++;
+    if (count === n)
+      bench.end(n);
+  }
+
+  for (var i = 0; i < n; i++) {
+    // unref().ref() will cause each of these timers to
+    // allocate their own handle
+    setTimeout(i % 2 ? cb : cb2, 1).unref().ref();
+  }
+
+  bench.start();
+}

lib/timers.js

@@ -24,7 +24,7 @@
 const async_wrap = process.binding('async_wrap');
 const {
   Timer: TimerWrap,
-  setImmediateCallback,
+  setupTimers,
 } = process.binding('timer_wrap');
 const L = require('internal/linkedlist');
 const timerInternals = require('internal/timers');
@@ -34,7 +34,6 @@ const assert = require('assert');
 const util = require('util');
 const errors = require('internal/errors');
 const debug = util.debuglog('timer');
-const kOnTimeout = TimerWrap.kOnTimeout | 0;
 // Two arrays that share state between C++ and JS.
 const { async_hook_fields, async_id_fields } = async_wrap;
 const {
@@ -57,7 +56,7 @@ const kRefCount = 1;
 const kHasOutstanding = 2;
 
 const [immediateInfo, toggleImmediateRef] =
-  setImmediateCallback(processImmediate);
+  setupTimers(processImmediate, processTimers);
 
 const kRefed = Symbol('refed');
 
@@ -221,10 +220,14 @@ function TimersList(msecs, unrefed) {
   timer.start(msecs);
 }
 
-// adds listOnTimeout to the C++ object prototype, as
-// V8 would not inline it otherwise.
-TimerWrap.prototype[kOnTimeout] = function listOnTimeout(now) {
-  const list = this._list;
+function processTimers(now) {
+  if (this.owner)
+    return unrefdHandle(this.owner, now);
+  return listOnTimeout(this, now);
+}
+
+function listOnTimeout(handle, now) {
+  const list = handle._list;
   const msecs = list.msecs;
 
   debug('timeout callback %d', msecs);
@@ -241,7 +244,7 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout(now) {
       if (timeRemaining <= 0) {
         timeRemaining = 1;
       }
-      this.start(timeRemaining);
+      handle.start(timeRemaining);
       debug('%d list wait because diff is %d', msecs, diff);
       return true;
     }
@@ -280,11 +283,11 @@ TimerWrap.prototype[kOnTimeout] = function listOnTimeout(now) {
 
   // Do not close the underlying handle if its ownership has changed
   // (e.g it was unrefed in its callback).
-  if (!this.owner)
-    this.close();
+  if (!handle.owner)
+    handle.close();
 
   return true;
-};
+}
 
 
 // An optimization so that the try/finally only de-optimizes (since at least v8
@@ -516,18 +519,17 @@ exports.clearInterval = function(timer) {
 };
 
 
-function unrefdHandle(now) {
+function unrefdHandle(timer, now) {
   try {
     // Don't attempt to call the callback if it is not a function.
-    if (typeof this.owner._onTimeout === 'function') {
-      tryOnTimeout(this.owner, now);
+    if (typeof timer._onTimeout === 'function') {
+      tryOnTimeout(timer, now);
     }
   } finally {
     // Make sure we clean up if the callback is no longer a function
     // even if the timer is an interval.
-    if (!this.owner._repeat ||
-        typeof this.owner._onTimeout !== 'function') {
-      this.owner.close();
+    if (!timer._repeat || typeof timer._onTimeout !== 'function') {
+      timer.close();
     }
   }
 
@@ -557,7 +559,6 @@ Timeout.prototype.unref = function() {
 
     this._handle = handle || new TimerWrap();
     this._handle.owner = this;
-    this._handle[kOnTimeout] = unrefdHandle;
     this._handle.start(delay);
     this._handle.unref();
   }
@@ -581,7 +582,6 @@ Timeout.prototype.close = function() {
     }
 
     this._idleTimeout = -1;
-    this._handle[kOnTimeout] = null;
     this._handle.close();
   } else {
     unenroll(this);

src/env.h

@@ -308,6 +308,7 @@ class ModuleWrap;
   V(secure_context_constructor_template, v8::FunctionTemplate)                \
   V(tcp_constructor_template, v8::FunctionTemplate)                           \
   V(tick_callback_function, v8::Function)                                     \
+  V(timers_callback_function, v8::Function)                                   \
   V(tls_wrap_constructor_function, v8::Function)                              \
   V(tty_constructor_template, v8::FunctionTemplate)                           \
   V(udp_constructor_function, v8::Function)                                   \

src/timer_wrap.cc

@@ -41,8 +41,6 @@ using v8::Object;
 using v8::String;
 using v8::Value;
 
-const uint32_t kOnTimeout = 0;
-
 class TimerWrap : public HandleWrap {
  public:
   static void Initialize(Local<Object> target,
@@ -53,8 +51,6 @@ class TimerWrap : public HandleWrap {
     Local<String> timerString = FIXED_ONE_BYTE_STRING(env->isolate(), "Timer");
     constructor->InstanceTemplate()->SetInternalFieldCount(1);
     constructor->SetClassName(timerString);
-    constructor->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kOnTimeout"),
-                     Integer::New(env->isolate(), kOnTimeout));
 
     env->SetTemplateMethod(constructor, "now", Now);
 
@@ -71,18 +67,22 @@ class TimerWrap : public HandleWrap {
     target->Set(timerString, constructor->GetFunction());
 
     target->Set(env->context(),
-                FIXED_ONE_BYTE_STRING(env->isolate(), "setImmediateCallback"),
-                env->NewFunctionTemplate(SetImmediateCallback)
+                FIXED_ONE_BYTE_STRING(env->isolate(), "setupTimers"),
+                env->NewFunctionTemplate(SetupTimers)
                    ->GetFunction(env->context()).ToLocalChecked()).FromJust();
   }
 
   size_t self_size() const override { return sizeof(*this); }
 
  private:
-  static void SetImmediateCallback(const FunctionCallbackInfo<Value>& args) {
+  static void SetupTimers(const FunctionCallbackInfo<Value>& args) {
     CHECK(args[0]->IsFunction());
+    CHECK(args[1]->IsFunction());
     auto env = Environment::GetCurrent(args);
+
     env->set_immediate_callback_function(args[0].As<Function>());
+    env->set_timers_callback_function(args[1].As<Function>());
+
     auto toggle_ref_cb = [] (const FunctionCallbackInfo<Value>& args) {
       Environment::GetCurrent(args)->ToggleImmediateRef(args[0]->IsTrue());
     };
@@ -142,7 +142,8 @@ class TimerWrap : public HandleWrap {
     Local<Value> args[1];
     do {
       args[0] = env->GetNow();
-      ret = wrap->MakeCallback(kOnTimeout, 1, args).ToLocalChecked();
+      ret = wrap->MakeCallback(env->timers_callback_function(), 1, args)
+                .ToLocalChecked();
     } while (ret->IsUndefined() &&
              !env->tick_info()->has_thrown() &&
              wrap->object()->Get(env->context(),

test/message/timeout_throw.out

@@ -5,4 +5,5 @@ ReferenceError: undefined_reference_error_maker is not defined
     at Timeout._onTimeout (*test*message*timeout_throw.js:*:*)
     at ontimeout (timers.js:*:*)
     at tryOnTimeout (timers.js:*:*)
-    at Timer.listOnTimeout (timers.js:*:*)
+    at listOnTimeout (timers.js:*:*)
+    at Timer.processTimers (timers.js:*:*)