timers: do less work in insert

lib/internal/timers.js

@@ -155,7 +155,7 @@ function initAsyncResource(resource, type) {
 
 // Timer constructor function.
 // The entire prototype is defined in lib/timers.js
-function Timeout(callback, after, args, isRepeat) {
+function Timeout(callback, after, args, isRepeat, isRefed) {
   after *= 1; // Coalesce to number or NaN
   if (!(after >= 1 && after <= TIMEOUT_MAX)) {
     if (after > TIMEOUT_MAX) {
@@ -179,7 +179,9 @@ function Timeout(callback, after, args, isRepeat) {
   this._repeat = isRepeat ? after : null;
   this._destroyed = false;
 
-  this[kRefed] = null;
+  if (isRefed)
+    incRefCount();
+  this[kRefed] = isRefed;
 
   initAsyncResource(this, 'Timeout');
 }
@@ -207,21 +209,23 @@ Timeout.prototype.refresh = function() {
 Timeout.prototype.unref = function() {
   if (this[kRefed]) {
     this[kRefed] = false;
-    decRefCount();
+    if (!this._destroyed)
+      decRefCount();
   }
   return this;
 };
 
 Timeout.prototype.ref = function() {
-  if (this[kRefed] === false) {
+  if (!this[kRefed]) {
     this[kRefed] = true;
-    incRefCount();
+    if (!this._destroyed)
+      incRefCount();
   }
   return this;
 };
 
 Timeout.prototype.hasRef = function() {
-  return !!this[kRefed];
+  return this[kRefed];
 };
 
 function TimersList(expiry, msecs) {
@@ -295,27 +299,47 @@ function decRefCount() {
 // Schedule or re-schedule a timer.
 // The item must have been enroll()'d first.
 function active(item) {
-  insert(item, true, getLibuvNow());
+  insertGuarded(item, true);
 }
 
 // Internal APIs that need timeouts should use `unrefActive()` instead of
 // `active()` so that they do not unnecessarily keep the process open.
 function unrefActive(item) {
-  insert(item, false, getLibuvNow());
+  insertGuarded(item, false);
 }
 
 // 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
 // list if one does not already exist for the specified timeout duration.
-function insert(item, refed, start) {
-  let msecs = item._idleTimeout;
+function insertGuarded(item, refed, start) {
+  const msecs = item._idleTimeout;
   if (msecs < 0 || msecs === undefined)
     return;
 
-  // Truncate so that accuracy of sub-millisecond timers is not assumed.
-  msecs = MathTrunc(msecs);
+  insert(item, msecs, start);
+
+  const isDestroyed = item._destroyed;
+  if (isDestroyed || !item[async_id_symbol]) {
+    item._destroyed = false;
+    initAsyncResource(item, 'Timeout');
+  }
+
+  if (isDestroyed) {
+    if (refed)
+      incRefCount();
+  } else if (refed === !item[kRefed]) {
+    if (refed)
+      incRefCount();
+    else
+      decRefCount();
+  }
+  item[kRefed] = refed;
+}
 
+function insert(item, msecs, start = getLibuvNow()) {
+  // Truncate so that accuracy of sub-milisecond timers is not assumed.
+  msecs = MathTrunc(msecs);
   item._idleStart = start;
 
   // Use an existing list if there is one, otherwise we need to make a new one.
@@ -332,19 +356,6 @@ function insert(item, refed, start) {
     }
   }
 
-  if (!item[async_id_symbol] || item._destroyed) {
-    item._destroyed = false;
-    initAsyncResource(item, 'Timeout');
-  }
-
-  if (refed === !item[kRefed]) {
-    if (refed)
-      incRefCount();
-    else
-      decRefCount();
-  }
-  item[kRefed] = refed;
-
   L.append(list, item);
 }
 
@@ -354,8 +365,8 @@ function setUnrefTimeout(callback, after) {
     throw new ERR_INVALID_CALLBACK(callback);
   }
 
-  const timer = new Timeout(callback, after, undefined, false);
-  unrefActive(timer);
+  const timer = new Timeout(callback, after, undefined, false, false);
+  insert(timer, timer._idleTimeout);
 
   return timer;
 }
@@ -514,13 +525,14 @@ function getTimerCallbacks(runNextTicks) {
       const asyncId = timer[async_id_symbol];
 
       if (!timer._onTimeout) {
-        if (timer[kRefed])
-          refCount--;
-        timer[kRefed] = null;
-
-        if (destroyHooksExist() && !timer._destroyed) {
-          emitDestroy(asyncId);
+        if (!timer._destroyed) {
           timer._destroyed = true;
+
+          if (timer[kRefed])
+            refCount--;
+
+          if (destroyHooksExist())
+            emitDestroy(asyncId);
         }
         continue;
       }
@@ -540,18 +552,15 @@ function getTimerCallbacks(runNextTicks) {
       } finally {
         if (timer._repeat && timer._idleTimeout !== -1) {
           timer._idleTimeout = timer._repeat;
-          if (start === undefined)
-            start = getLibuvNow();
-          insert(timer, timer[kRefed], start);
-        } else if (!timer._idleNext && !timer._idlePrev) {
+          insert(timer, timer._idleTimeout, start);
+        } else if (!timer._idleNext && !timer._idlePrev && !timer._destroyed) {
+          timer._destroyed = true;
+
           if (timer[kRefed])
             refCount--;
-          timer[kRefed] = null;
 
-          if (destroyHooksExist() && !timer._destroyed) {
-            emitDestroy(timer[async_id_symbol]);
-          }
-          timer._destroyed = true;
+          if (destroyHooksExist())
+            emitDestroy(asyncId);
         }
       }
 
@@ -598,6 +607,7 @@ module.exports = {
   },
   active,
   unrefActive,
+  insert,
   timerListMap,
   timerListQueue,
   decRefCount,

lib/timers.js

@@ -45,7 +45,8 @@ const {
   timerListQueue,
   immediateQueue,
   active,
-  unrefActive
+  unrefActive,
+  insert
 } = require('internal/timers');
 const {
   promisify: { custom: customPromisify },
@@ -62,13 +63,14 @@ const {
 
 // Remove a timer. Cancels the timeout and resets the relevant timer properties.
 function unenroll(item) {
+  if (item._destroyed)
+    return;
+
+  item._destroyed = true;
+
   // Fewer checks may be possible, but these cover everything.
-  if (destroyHooksExist() &&
-      item[async_id_symbol] !== undefined &&
-      !item._destroyed) {
+  if (destroyHooksExist() && item[async_id_symbol] !== undefined)
     emitDestroy(item[async_id_symbol]);
-  }
-  item._destroyed = true;
 
   L.remove(item);
 
@@ -89,7 +91,6 @@ function unenroll(item) {
 
     decRefCount();
   }
-  item[kRefed] = null;
 
   // If active is called later, then we want to make sure not to insert again
   item._idleTimeout = -1;
@@ -141,16 +142,17 @@ function setTimeout(callback, after, arg1, arg2, arg3) {
       break;
   }
 
-  const timeout = new Timeout(callback, after, args, false);
-  active(timeout);
+  const timeout = new Timeout(callback, after, args, false, true);
+  insert(timeout, timeout._idleTimeout);
 
   return timeout;
 }
 
 setTimeout[customPromisify] = function(after, value) {
   const args = value !== undefined ? [value] : value;
   return new Promise((resolve) => {
-    active(new Timeout(resolve, after, args, false));
+    const timeout = new Timeout(resolve, after, args, false, true);
+    insert(timeout, timeout._idleTimeout);
   });
 };
 
@@ -187,8 +189,8 @@ function setInterval(callback, repeat, arg1, arg2, arg3) {
       break;
   }
 
-  const timeout = new Timeout(callback, repeat, args, true);
-  active(timeout);
+  const timeout = new Timeout(callback, repeat, args, true, true);
+  insert(timeout, timeout._idleTimeout);
 
   return timeout;
 }

test/parallel/test-timers-refresh.js

@@ -72,6 +72,20 @@ const { inspect } = require('util');
   strictEqual(timer.refresh(), timer);
 }
 
+// regular timer
+{
+  let called = false;
+  const timer = setTimeout(common.mustCall(() => {
+    if (!called) {
+      called = true;
+      process.nextTick(common.mustCall(() => {
+        timer.refresh();
+        strictEqual(timer.hasRef(), true);
+      }));
+    }
+  }, 2), 1);
+}
+
 // interval
 {
   let called = 0;