From 934bfe23a16556d05bfb1844ef4d53e8c9887c3d Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Wed, 20 Aug 2014 00:08:32 -0700 Subject: [PATCH] timers: Avoid linear scan in _unrefActive. Before this change, _unrefActive would keep the unrefList sorted when adding a new timer. Because _unrefActive is called extremely frequently, this linear scan (O(n) at worse) would make _unrefActive show high in the list of contributors when profiling CPU usage. This commit changes _unrefActive so that it doesn't try to keep the unrefList sorted. The insertion thus happens in constant time. However, when a timer expires, unrefTimeout has to go through the whole unrefList because it's not ordered anymore. It is usually not large enough to have a significant impact on performance because: - Most of the time, the timers will be removed before unrefTimeout is called because their users (sockets mainly) cancel them when an I/O operation takes place. - If they're not, it means that some I/O took a long time to happen, and the initiator of subsequents I/O operations that would add more timers has to wait for them to complete. With this change, _unrefActive does not show as a significant contributor in CPU profiling reports anymore. Fixes #8160. PR-URL: #8751 Signed-off-by: Timothy J Fontaine --- lib/timers.js | 120 ++++++++++++++---------- test/simple/test-timers-unref-active.js | 71 ++++++++++++++ 2 files changed, 144 insertions(+), 47 deletions(-) create mode 100644 test/simple/test-timers-unref-active.js diff --git a/lib/timers.js b/lib/timers.js index 1893f19789c248..de3c9ed6c09f07 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -410,39 +410,89 @@ function unrefTimeout() { debug('unrefTimer fired'); - var first; - while (first = L.peek(unrefList)) { - var diff = now - first._monotonicStartTime; + var timeSinceLastActive; + var nextTimeoutTime; + var nextTimeoutDuration; + var minNextTimeoutTime; + var itemToDelete; + + // The actual timer fired and has not yet been rearmed, + // let's consider its next firing time is invalid for now. + // It may be set to a relevant time in the future once + // we scanned through the whole list of timeouts and if + // we find a timeout that needs to expire. + unrefTimer.when = -1; - if (diff < first._idleTimeout) { - diff = first._idleTimeout - diff; - unrefTimer.start(diff, 0); - unrefTimer.when = now + diff; - debug('unrefTimer rescheudling for later'); - return; + // Iterate over the list of timeouts, + // call the onTimeout callback for those expired, + // and rearm the actual timer if the next timeout to expire + // will expire before the current actual timer. + var cur = unrefList._idlePrev; + while (cur != unrefList) { + timeSinceLastActive = now - cur._monotonicStartTime; + + if (timeSinceLastActive < cur._idleTimeout) { + // This timer hasn't expired yet, but check if its expiring time is + // earlier than the actual timer's expiring time + + nextTimeoutDuration = cur._idleTimeout - timeSinceLastActive; + nextTimeoutTime = now + nextTimeoutDuration; + if (minNextTimeoutTime == null || + (nextTimeoutTime < minNextTimeoutTime)) { + // We found a timeout that will expire earlier, + // store its next timeout time now so that we + // can rearm the actual timer accordingly when + // we scanned through the whole list. + minNextTimeoutTime = nextTimeoutTime; + } + + // This timer hasn't expired yet, skipping + cur = cur._idlePrev; + continue; } - L.remove(first); + // We found a timer that expired + var domain = cur.domain; - var domain = first.domain; + if (!cur._onTimeout) continue; - if (!first._onTimeout) continue; - if (domain && domain._disposed) continue; + if (domain && domain._disposed) + continue; try { - if (domain) domain.enter(); var threw = true; + + if (domain) domain.enter(); + + itemToDelete = cur; + // Move to the previous item before calling the _onTimeout callback, + // as it can mutate the list. + cur = cur._idlePrev; + + // Remove the timeout from the list because it expired. + L.remove(itemToDelete); + debug('unreftimer firing timeout'); - first._onTimeout(); + itemToDelete._onTimeout(); + threw = false; - if (domain) domain.exit(); + + if (domain) + domain.exit(); } finally { if (threw) process.nextTick(unrefTimeout); } } - debug('unrefList is empty'); - unrefTimer.when = -1; + // Rearm the actual timer with the timeout delay + // of the earliest timeout found. + if (minNextTimeoutTime != null) { + unrefTimer.start(minNextTimeoutTime - now, 0); + unrefTimer.when = minNextTimeoutTime; + debug('unrefTimer rescheduled'); + } else if (L.isEmpty(unrefList)) { + debug('unrefList is empty'); + } } @@ -471,38 +521,14 @@ exports._unrefActive = function(item) { item._idleStart = nowDate; item._monotonicStartTime = nowMonotonicTimestamp; - if (L.isEmpty(unrefList)) { - debug('unrefList empty'); - L.append(unrefList, item); + var when = nowMonotonicTimestamp + msecs; + // If the actual timer is set to fire too late, or not set to fire at all, + // we need to make it fire earlier + if (unrefTimer.when === -1 || unrefTimer.when > when) { unrefTimer.start(msecs, 0); - unrefTimer.when = nowMonotonicTimestamp + msecs; + unrefTimer.when = when; debug('unrefTimer scheduled'); - return; - } - - var when = nowMonotonicTimestamp + msecs; - - debug('unrefList find where we can insert'); - - var cur, them; - - for (cur = unrefList._idlePrev; cur != unrefList; cur = cur._idlePrev) { - them = cur._monotonicStartTime + cur._idleTimeout; - - if (when < them) { - debug('unrefList inserting into middle of list'); - - L.append(cur, item); - - if (unrefTimer.when > when) { - debug('unrefTimer is scheduled to fire too late, reschedule'); - unrefTimer.start(msecs, 0); - unrefTimer.when = when; - } - - return; - } } debug('unrefList append to end'); diff --git a/test/simple/test-timers-unref-active.js b/test/simple/test-timers-unref-active.js new file mode 100644 index 00000000000000..ce07c2dee62194 --- /dev/null +++ b/test/simple/test-timers-unref-active.js @@ -0,0 +1,71 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +/* + * This test is aimed at making sure that unref timers queued with + * timers._unrefActive work correctly. + * + * Basically, it queues one timer in the unref queue, and then queues + * it again each time its timeout callback is fired until the callback + * has been called ten times. + * + * At that point, it unenrolls the unref timer so that its timeout callback + * is not fired ever again. + * + * Finally, a ref timeout is used with a delay large enough to make sure that + * all 10 timeouts had the time to expire. + */ + +var timers = require('timers'); +var assert = require('assert'); + +var someObject = {}; +var nbTimeouts = 0; + +/* + * libuv 0.10.x uses GetTickCount on Windows to implement timers, which uses + * system's timers whose resolution is between 10 and 16ms. See + * http://msdn.microsoft.com/en-us/library/windows/desktop/ms724408.aspx + * for more information. That's the lowest resolution for timers across all + * supported platforms. We're using it as the lowest common denominator, + * and thus expect 5 timers to be able to fire in under 100 ms. + */ +var N = 5; +var TEST_DURATION = 100; + +timers.unenroll(someObject); +timers.enroll(someObject, 1); + +someObject._onTimeout = function _onTimeout() { + ++nbTimeouts; + + if (nbTimeouts === N) timers.unenroll(someObject); + + timers._unrefActive(someObject); +} + +function startTimer() { timers._unrefActive(someObject); } + +startTimer(); + +setTimeout(function() { + assert.equal(nbTimeouts, N); +}, TEST_DURATION);