Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

timers: allow timers to be used as primitives #21152

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions doc/api/timers.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,14 @@ Calling `timeout.unref()` creates an internal timer that will wake the Node.js
event loop. Creating too many of these can adversely impact performance
of the Node.js application.

### timeout[Symbol.toPrimitive]()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is rendered as an empty link in the GitHub and in HTML:

l

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+ Should this section be placed before the timeout.unref() heading ABC-wise?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it depends on what convention we follow for symbols. I prefer it after but it could also be the first thing in the list. I wouldn't put it between ref & unref.

Will fix the link issue 👍


* Returns: {integer}

When coercing a `Timeout` to a primitive, a primitive will be generated that
can be used to clear the `Timeout`. This allows enhanced compatibility with
browser `setTimeout`, and `setInterval` implementations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`setTimeout` -> `setTimeout()`?
`setInterval` -> `setInterval()`?


## Scheduling Timers

A timer in Node.js is an internal construct that calls a given function after
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const {
// Timeout values > TIMEOUT_MAX are set to 1.
const TIMEOUT_MAX = 2 ** 31 - 1;

const kHasPrimitive = Symbol('hasPrimitive');
const kRefed = Symbol('refed');

module.exports = {
Expand All @@ -27,6 +28,7 @@ module.exports = {
async_id_symbol,
trigger_async_id_symbol,
Timeout,
kHasPrimitive,
kRefed,
initAsyncResource,
setUnrefTimeout,
Expand Down Expand Up @@ -75,6 +77,7 @@ function Timeout(callback, after, args, isRepeat) {
this._repeat = isRepeat ? after : null;
this._destroyed = false;

this[kHasPrimitive] = false;
this[kRefed] = null;

initAsyncResource(this, 'Timeout');
Expand Down
31 changes: 31 additions & 0 deletions lib/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const {
async_id_symbol,
trigger_async_id_symbol,
Timeout,
kHasPrimitive,
kRefed,
initAsyncResource,
validateTimerDuration
Expand Down Expand Up @@ -136,6 +137,11 @@ const [immediateInfo, toggleImmediateRef] =
// - value = linked list
const lists = Object.create(null);

// This stores all the known timer async ids to allow users to clearTimeout and
// clearInterval using those ids, to match the spec and the rest of the web
// platform.
const knownTimersById = Object.create(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the keys are going to be numbers, could a holey array be faster?


// This is a priority queue with a custom sorting function that first compares
// the expiry times of two lists and if they're the same then compares their
// individual IDs to determine which list was created first.
Expand Down Expand Up @@ -347,6 +353,9 @@ function tryOnTimeout(timer, start) {
refCount--;
timer[kRefed] = null;

if (timer[kHasPrimitive])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might === true be faster?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't, hopefully? I think the problem is with symbols but I don't want to do this with a public prop. Maybe @nodejs/v8 can provide some input? Any chance of symbol access getting faster in the future? From what I've seen it's currently quite a bit slower than string props.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @bnoordhuis mentioned at some point that it was a bit faster.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what output turbolizer would give for this, this still doesn't seem right so I can only imagine it would be a polymorphism problem

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's polymorphism. In the benchmark we don't call this with any values but timer objects. I'm reasonably certain it's the symbol because I've run into this a bunch while working on timers & http2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh whoops, I was thinking of the wrong function

delete knownTimersById[timer[async_id_symbol]];

if (destroyHooksExist() && !timer._destroyed) {
emitDestroy(timer[async_id_symbol]);
timer._destroyed = true;
Expand Down Expand Up @@ -385,6 +394,9 @@ function unenroll(item) {
}
item[kRefed] = null;

if (item[kHasPrimitive])
delete knownTimersById[item[async_id_symbol]];

// if active is called later, then we want to make sure not to insert again
item._idleTimeout = -1;
}
Expand Down Expand Up @@ -487,6 +499,16 @@ const clearTimeout = exports.clearTimeout = function clearTimeout(timer) {
if (timer && timer._onTimeout) {
timer._onTimeout = null;
unenroll(timer);
return;
}

const timerType = typeof timer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typeof checks are fast IIRC

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and I think they are even faster that caching the value like this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if doing the checks and then turning this into an else if would reduce speculative branching and help?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caching typeof used to be a lot slower than using it inline, at least with Crankshaft, not sure about TurboFan.

if (timerType === 'string' || timerType === 'number') {
const timerInstance = knownTimersById[timer];
if (timerInstance !== undefined) {
timerInstance._onTimeout = null;
unenroll(timerInstance);
}
Copy link
Member

@jdalton jdalton Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👆 In the browser implementations I think there is some coercion going on as:

var i = setTimeout(()=>console.log("hi"), 1000);
clearTimeout({ [Symbol.toPrimitive]() { return i } })
// will clear the time

Would it make sense to instead of type checking just coerce the value to a number
(or whatever the correct steps may be).

}
};

Expand Down Expand Up @@ -531,6 +553,15 @@ exports.clearInterval = function clearInterval(timer) {
};


Timeout.prototype[Symbol.toPrimitive] = function() {
const id = this[async_id_symbol];
if (!this[kHasPrimitive]) {
this[kHasPrimitive] = true;
knownTimersById[id] = this;
}
return id;
};

Timeout.prototype.unref = function() {
if (this[kRefed]) {
this[kRefed] = false;
Expand Down
25 changes: 25 additions & 0 deletions test/parallel/test-timers-to-primitive.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict';
const common = require('../common');
const assert = require('assert');

const timeout1 = setTimeout(common.mustNotCall(), 1);
const timeout2 = setInterval(common.mustNotCall(), 1);

assert.strictEqual(Number.isNaN(+timeout1), false);
assert.strictEqual(Number.isNaN(+timeout2), false);

assert.strictEqual(+timeout1, timeout1[Symbol.toPrimitive]());

assert.notStrictEqual(`${timeout1}`, Object.prototype.toString.call(timeout1));
assert.notStrictEqual(`${timeout2}`, Object.prototype.toString.call(timeout2));

assert.notStrictEqual(+timeout1, +timeout2);

const o = {};
o[timeout1] = timeout1;
o[timeout2] = timeout2;
const keys = Object.keys(o);
assert.deepStrictEqual(keys, [`${timeout1}`, `${timeout2}`]);

clearTimeout(keys[0]); // Works for string.
clearInterval(+timeout2); // Works for integer.