From 65640c2c42856e352b46cd77c1822fdacedca6d1 Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Thu, 1 Sep 2016 13:19:33 -0400 Subject: [PATCH 1/2] timers: optimize same-tick unref The logic behind this revolves around the following: 1. A timer will never fire before nextTick. - Thus it is ok to schedule it during nextTick. 2. Timer properties are unimportant until scheduled. See discussion in https://github.com/nodejs/node/pull/6699 for more background. --- lib/timers.js | 79 ++++++++++++++------- test/parallel/test-timers-unref-after-nt.js | 12 ++++ 2 files changed, 65 insertions(+), 26 deletions(-) create mode 100644 test/parallel/test-timers-unref-after-nt.js diff --git a/lib/timers.js b/lib/timers.js index f6ecf3e100a81e..9b7ec38855c768 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -97,30 +97,44 @@ const TIMEOUT_MAX = 2147483647; // 2^31-1 const refedLists = {}; const unrefedLists = {}; +const queuedTimers = L.create(); +let insertNTScheduled = false; // Schedule or re-schedule a timer. // The item must have been enroll()'d first. const active = exports.active = function(item) { - insert(item, false); + L.append(queuedTimers, [item, TimerWrap.now()]); + if (!insertNTScheduled) { + process.nextTick(insertNT); + } }; // Internal APIs that need timeouts should use `_unrefActive()` instead of // `active()` so that they do not unnecessarily keep the process open. exports._unrefActive = function(item) { - insert(item, true); + insert(item, true, TimerWrap.now()); }; +function insertNT() { + insertNTScheduled = false; + while (!L.isEmpty(queuedTimers)) { + const timer = L.shift(queuedTimers); + insert(timer[0], timer[0]._unrefed, timer[1]); + } +} + + // The underlying logic for scheduling or re-scheduling a timer. // // Appends a timer onto the end of an existing timers list, or creates a new // TimerWrap backed list if one does not already exist for the specified timeout // duration. -function insert(item, unrefed) { +function insert(item, unrefed, now) { const msecs = item._idleTimeout; if (msecs < 0 || msecs === undefined) return; - item._idleStart = TimerWrap.now(); + item._idleStart = now; const lists = unrefed === true ? unrefedLists : refedLists; @@ -455,46 +469,59 @@ function Timeout(after) { this._idleStart = null; this._onTimeout = null; this._repeat = null; + this._unrefed = false; } - -function unrefdHandle() { +function ownHandle() { this.owner._onTimeout(); if (!this.owner._repeat) this.owner.close(); } +function createOwnHandle() { + var now = TimerWrap.now(); + if (!this._idleStart) this._idleStart = now; + var delay = this._idleStart + this._idleTimeout - now; + if (delay < 0) delay = 0; + + // Prevent running cb again when unref() is called during the same cb + if (this._called && !this._repeat) { + unenroll(this); + return; + } + + var handle = reuse(this); + + this._handle = handle || new TimerWrap(); + this._handle.owner = this; + this._handle[kOnTimeout] = ownHandle; + this._handle.start(delay); + this._handle.domain = this.domain; +} Timeout.prototype.unref = function() { + this._unrefed = true; if (this._handle) { this._handle.unref(); - } else if (typeof this._onTimeout === 'function') { - var now = TimerWrap.now(); - if (!this._idleStart) this._idleStart = now; - var delay = this._idleStart + this._idleTimeout - now; - if (delay < 0) delay = 0; - - // Prevent running cb again when unref() is called during the same cb - if (this._called && !this._repeat) { - unenroll(this); - return; - } - - var handle = reuse(this); - - this._handle = handle || new TimerWrap(); - this._handle.owner = this; - this._handle[kOnTimeout] = unrefdHandle; - this._handle.start(delay); - this._handle.domain = this.domain; + } else if (typeof this._onTimeout === 'function' && + this._idleNext !== this/* Has been inserted */ && + this._idleNext) { + createOwnHandle.call(this); this._handle.unref(); } return this; }; Timeout.prototype.ref = function() { - if (this._handle) + this._unrefed = false; + if (this._handle) { this._handle.ref(); + } else if (typeof this._onTimeout === 'function' && + this._idleNext !== this/* Has been inserted */ && + this._idleNext) { // TODO: not guarenteed correct for ref() + createOwnHandle.call(this); + this._handle.ref(); + } return this; }; diff --git a/test/parallel/test-timers-unref-after-nt.js b/test/parallel/test-timers-unref-after-nt.js new file mode 100644 index 00000000000000..5fd2580c603503 --- /dev/null +++ b/test/parallel/test-timers-unref-after-nt.js @@ -0,0 +1,12 @@ +'use strict'; +const common = require('../common'); + +const timer = setTimeout(() => { + common.fail('nope'); +}, 200); + +setImmediate(() => { + timer.unref(); +}); + +setTimeout(common.fail, 500).unref(); From 620412b6ca8af8e5640add000e6fc1064f855dfe Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Fri, 2 Sep 2016 11:15:14 -0400 Subject: [PATCH 2/2] tests: update for timers unref optimization --- test/parallel/test-handle-wrap-isrefed.js | 16 ++++---- test/parallel/test-timers-active.js | 18 +++++---- ...-timers-same-timeout-wrong-list-deleted.js | 40 ++++++++++--------- test/parallel/test-timers-unref-leak.js | 14 ++++--- 4 files changed, 49 insertions(+), 39 deletions(-) diff --git a/test/parallel/test-handle-wrap-isrefed.js b/test/parallel/test-handle-wrap-isrefed.js index b5dbeb23bfd63a..e52f27505e3bc6 100644 --- a/test/parallel/test-handle-wrap-isrefed.js +++ b/test/parallel/test-handle-wrap-isrefed.js @@ -91,11 +91,13 @@ function makeAssert(message) { { const assert = makeAssert('hasRef() not working on timer_wrap'); const timer = setTimeout(() => {}, 500); - timer.unref(); - assert(Object.getPrototypeOf(timer._handle).hasOwnProperty('hasRef'), true); - assert(timer._handle.hasRef(), false); - timer.ref(); - assert(timer._handle.hasRef(), true); - timer._handle.close( - common.mustCall(() => assert(timer._handle.hasRef(), false))); + setImmediate(() => { + timer.unref(); + assert(Object.getPrototypeOf(timer._handle).hasOwnProperty('hasRef'), true); + assert(timer._handle.hasRef(), false); + timer.ref(); + assert(timer._handle.hasRef(), true); + timer._handle.close( + common.mustCall(() => assert(timer._handle.hasRef(), false))); + }); } diff --git a/test/parallel/test-timers-active.js b/test/parallel/test-timers-active.js index d8faa1f5a33483..7e3a55ecf4438c 100644 --- a/test/parallel/test-timers-active.js +++ b/test/parallel/test-timers-active.js @@ -12,11 +12,13 @@ var legitTimers = [ legitTimers.forEach(function(legit) { const savedTimeout = legit._idleTimeout; active(legit); - // active() should mutate these objects - assert(legit._idleTimeout === savedTimeout); - assert(Number.isInteger(legit._idleStart)); - assert(legit._idleNext); - assert(legit._idlePrev); + process.nextTick(() => { + // active() should mutate these objects + assert(legit._idleTimeout === savedTimeout); + assert(Number.isInteger(legit._idleStart)); + assert(legit._idleNext); + assert(legit._idlePrev); + }); }); @@ -29,6 +31,8 @@ var bogusTimers = [ bogusTimers.forEach(function(bogus) { const savedTimeout = bogus._idleTimeout; active(bogus); - // active() should not mutate these objects - assert.deepStrictEqual(bogus, {_idleTimeout: savedTimeout}); + process.nextTick(() => { + // active() should not mutate these objects + assert.deepStrictEqual(bogus, {_idleTimeout: savedTimeout}); + }); }); diff --git a/test/parallel/test-timers-same-timeout-wrong-list-deleted.js b/test/parallel/test-timers-same-timeout-wrong-list-deleted.js index 05c0233e124b83..187c8f04ad524a 100644 --- a/test/parallel/test-timers-same-timeout-wrong-list-deleted.js +++ b/test/parallel/test-timers-same-timeout-wrong-list-deleted.js @@ -56,25 +56,27 @@ const handle1 = setTimeout(common.mustCall(function() { })); }), 10); - // Make sure our timers got added to the list. - const activeHandles = process._getActiveHandles(); - const activeTimers = activeHandles.filter(function(handle) { - return handle instanceof Timer; - }); - const shortTimer = activeTimers.find(function(handle) { - return handle._list.msecs === 10; - }); - const longTimers = activeTimers.filter(function(handle) { - return handle._list.msecs === TIMEOUT; - }); + setImmediate(() => { + // Make sure our timers got added to the list. + const activeHandles = process._getActiveHandles(); + const activeTimers = activeHandles.filter(function(handle) { + return handle instanceof Timer; + }); + const shortTimer = activeTimers.find(function(handle) { + return handle._list.msecs === 10; + }); + const longTimers = activeTimers.filter(function(handle) { + return handle._list.msecs === TIMEOUT; + }); - // Make sure our clearTimeout succeeded. One timer finished and - // the other was canceled, so none should be active. - assert.equal(activeTimers.length, 3, 'There are 3 timers in the list.'); - assert(shortTimer instanceof Timer, 'The shorter timer is in the list.'); - assert.equal(longTimers.length, 2, 'Both longer timers are in the list.'); + // Make sure our clearTimeout succeeded. One timer finished and + // the other was canceled, so none should be active. + assert.equal(activeTimers.length, 3, 'There are 3 timers in the list.'); + assert(shortTimer instanceof Timer, 'The shorter timer is in the list.'); + assert.equal(longTimers.length, 2, 'Both longer timers are in the list.'); - // When this callback completes, `listOnTimeout` should now look at the - // correct list and refrain from removing the new TIMEOUT list which - // contains the reference to the newer timer. + // When this callback completes, `listOnTimeout` should now look at the + // correct list and refrain from removing the new TIMEOUT list which + // contains the reference to the newer timer. + }); }), TIMEOUT); diff --git a/test/parallel/test-timers-unref-leak.js b/test/parallel/test-timers-unref-leak.js index a1b1265763bf1d..5c33ca57011875 100644 --- a/test/parallel/test-timers-unref-leak.js +++ b/test/parallel/test-timers-unref-leak.js @@ -8,14 +8,16 @@ var closed = 0; var timeout = setTimeout(function() { called++; }, 10); -timeout.unref(); // Wrap `close` method to check if the handle was closed -var close = timeout._handle.close; -timeout._handle.close = function() { - closed++; - return close.apply(this, arguments); -}; +process.nextTick(() => { + timeout.unref(); + var close = timeout._handle.close; + timeout._handle.close = function() { + closed++; + return close.apply(this, arguments); + }; +}); // Just to keep process alive and let previous timer's handle die setTimeout(function() {