From 0f9efef05deb11dbbdf5adf96460839f5b332207 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 5 Feb 2018 14:29:32 -0500 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/18582 Reviewed-By: Ruben Bridgewater Reviewed-By: Matteo Collina Reviewed-By: Vladimir de Turckheim Reviewed-By: Evan Lucas --- benchmark/timers/timers-timeout-unpooled.js | 36 +++++++++++++++++++ lib/timers.js | 38 ++++++++++----------- src/env.h | 1 + src/timer_wrap.cc | 17 ++++----- test/message/timeout_throw.out | 3 +- 5 files changed, 67 insertions(+), 28 deletions(-) create mode 100644 benchmark/timers/timers-timeout-unpooled.js diff --git a/benchmark/timers/timers-timeout-unpooled.js b/benchmark/timers/timers-timeout-unpooled.js new file mode 100644 index 00000000000000..19f0f6a4af4d0d --- /dev/null +++ b/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(); +} diff --git a/lib/timers.js b/lib/timers.js index 39a22454ac906b..8e914d751a93d8 100644 --- a/lib/timers.js +++ b/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); diff --git a/src/env.h b/src/env.h index b621a54e3780cd..95548c0900ea13 100644 --- a/src/env.h +++ b/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) \ diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc index 441974ae77b5db..02c0b8166981ab 100644 --- a/src/timer_wrap.cc +++ b/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 target, @@ -53,8 +51,6 @@ class TimerWrap : public HandleWrap { Local 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& args) { + static void SetupTimers(const FunctionCallbackInfo& args) { CHECK(args[0]->IsFunction()); + CHECK(args[1]->IsFunction()); auto env = Environment::GetCurrent(args); + env->set_immediate_callback_function(args[0].As()); + env->set_timers_callback_function(args[1].As()); + auto toggle_ref_cb = [] (const FunctionCallbackInfo& args) { Environment::GetCurrent(args)->ToggleImmediateRef(args[0]->IsTrue()); }; @@ -142,7 +142,8 @@ class TimerWrap : public HandleWrap { Local 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(), diff --git a/test/message/timeout_throw.out b/test/message/timeout_throw.out index 9ef4f63e3d8b97..382cf5a054942c 100644 --- a/test/message/timeout_throw.out +++ b/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:*:*)