timers: allow timers to be used as primitives #21152

apapirovski · 2018-06-05T15:48:10Z

This continues on @bmeck's excellent idea and work in #19683. With their permission, I made some additional changes to improve the performance and fix a memory leak. This way we shouldn't see any (or certainly very minimal) regression in our benchmarks, at least judging by running them locally.

For web compatibility this allows timers to be stored as the key of an Object property and be passed back to corresponding method to clear the timer.

Refs: #19683
Co-authored-by: Bradley Farias bradley.meck@gmail.com

@bmeck I had to give you credit as co-author using github's metadata as it was too much work to rebase the original. Hopefully not a huge deal (but definitely let me know)?

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

For web compatibility this allows timers to be stored as the key of an Object property and be passed back to corresponding method to clear the timer. Co-authored-by: Bradley Farias <bradley.meck@gmail.com>

apapirovski · 2018-06-05T15:50:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15261/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1449/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/205/

bmeck · 2018-06-05T15:51:55Z

+1, don't worry about attribution here.

mscdex · 2018-06-05T18:00:43Z

The double digit performance regressions still worry me.

apapirovski · 2018-06-05T18:03:09Z

@mscdex Let me see what's going on. This doesn't match what I'm seeing locally... I'm also noticing a wide spread in the results. Let's run it again, not convinced the timers benchmarks are precise enough on the linux machine for some reason.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/206/

jdalton · 2018-06-06T04:24:42Z

lib/timers.js

+    if (timerInstance !== undefined) {
+      timerInstance._onTimeout = null;
+      unenroll(timerInstance);
+    }


👆 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).

vsemozhetbyt · 2018-06-06T21:51:01Z

doc/api/timers.md

@@ -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]()


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

An example of how to fix such cases:

https://github.com/nodejs/node/blame/7588ceaf353af0f257d4d832bace4600edac704e/doc/api/stream.md#L1214

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

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 👍

vsemozhetbyt · 2018-06-06T22:00:15Z

doc/api/timers.md

+
+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.


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

mscdex · 2018-06-06T23:21:16Z

Benchmark results still seem to be about the same (double digit regressions), despite the increased number of runs.

apapirovski · 2018-06-07T10:56:50Z

Yeah, I'm going to investigate at some point this week and see if it can be fixed. It's frustrating that this doesn't match what I'm seeing locally.

Fishrock123 · 2018-06-07T13:23:53Z

lib/timers.js

@@ -347,6 +353,9 @@ function tryOnTimeout(timer, start) {
        refCount--;
      timer[kRefed] = null;

+      if (timer[kHasPrimitive])


Might === true be faster?

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.

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

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

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.

Oh whoops, I was thinking of the wrong function

Fishrock123 · 2018-06-07T13:24:30Z

lib/timers.js

+    return;
+  }
+
+  const timerType = typeof timer;


typeof checks are fast IIRC

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

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

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

joyeecheung · 2018-06-08T07:20:52Z

lib/timers.js

+// 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);


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

jasnell · 2018-09-10T18:18:20Z

What's the status on this one?

BridgeAR · 2019-03-10T00:07:51Z

Ping @apapirovski it would be great to move this forward.

apapirovski · 2019-03-21T16:12:28Z

@BridgeAR ya, it got held up on performance concerns. Will revisit again.

apapirovski · 2019-12-13T20:39:52Z

This will PR will land one day... maybe... I hope 😞

BridgeAR · 2020-01-12T23:48:31Z

@apapirovski do you think you might find some time to have a look at this some time soon again?

jasnell · 2020-06-19T14:54:08Z

There's been no further activity, unfortunately. Closing. Can reopen if someone wants to pick it back up

This allows timers to be matched to numeric Ids and therefore used as keys of an Object, passed and stored without storing the Timer instance. clearTimeout/clearInterval is modified to support numeric/string Ids. Co-authored-by: Bradley Farias <bradley.meck@gmail.com> Co-authored-by: Anatoli Papirovski <apapirovski@mac.com> Refs: nodejs#21152

This allows timers to be matched to numeric Ids and therefore used as keys of an Object, passed and stored without storing the Timer instance. clearTimeout/clearInterval is modified to support numeric/string Ids. Co-authored-by: Bradley Farias <bradley.meck@gmail.com> Co-authored-by: Anatoli Papirovski <apapirovski@mac.com> Refs: #21152 PR-URL: #34017 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

This allows timers to be matched to numeric Ids and therefore used as keys of an Object, passed and stored without storing the Timer instance. clearTimeout/clearInterval is modified to support numeric/string Ids. Co-authored-by: Bradley Farias <bradley.meck@gmail.com> Co-authored-by: Anatoli Papirovski <apapirovski@mac.com> Refs: nodejs#21152 PR-URL: nodejs#34017 Backport-PR-URL: nodejs#34482 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Signed-off-by: Denys Otrishko <shishugi@gmail.com>

This allows timers to be matched to numeric Ids and therefore used as keys of an Object, passed and stored without storing the Timer instance. clearTimeout/clearInterval is modified to support numeric/string Ids. Co-authored-by: Bradley Farias <bradley.meck@gmail.com> Co-authored-by: Anatoli Papirovski <apapirovski@mac.com> Refs: #21152 Backport-PR-URL: #34482 PR-URL: #34017 Backport-PR-URL: #34482 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Signed-off-by: Denys Otrishko <shishugi@gmail.com>

timers: allow timers to be used as primitives

d72b7ba

For web compatibility this allows timers to be stored as the key of an Object property and be passed back to corresponding method to clear the timer. Co-authored-by: Bradley Farias <bradley.meck@gmail.com>

apapirovski added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jun 5, 2018

apapirovski added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 5, 2018

jdalton reviewed Jun 6, 2018

View reviewed changes

vsemozhetbyt reviewed Jun 6, 2018

View reviewed changes

Fishrock123 reviewed Jun 7, 2018

View reviewed changes

joyeecheung reviewed Jun 8, 2018

View reviewed changes

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:52

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:20

jasnell closed this Jun 19, 2020

lundibundi mentioned this pull request Jun 22, 2020

timers: allow timers to be used as primitives #34017

Closed

4 tasks

lundibundi mentioned this pull request Jul 22, 2020

[v14.x backport] timers: allow timers to be used as primitives #34482

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: allow timers to be used as primitives #21152

timers: allow timers to be used as primitives #21152

apapirovski commented Jun 5, 2018

apapirovski commented Jun 5, 2018

bmeck commented Jun 5, 2018

mscdex commented Jun 5, 2018

apapirovski commented Jun 5, 2018

jdalton Jun 6, 2018 •

edited

Loading

vsemozhetbyt Jun 6, 2018

vsemozhetbyt Jun 6, 2018 •

edited

Loading

vsemozhetbyt Jun 6, 2018

apapirovski Jun 7, 2018

vsemozhetbyt Jun 6, 2018

mscdex commented Jun 6, 2018 •

edited

Loading

apapirovski commented Jun 7, 2018

Fishrock123 Jun 7, 2018

apapirovski Jun 7, 2018

Fishrock123 Jun 7, 2018

Fishrock123 Jun 7, 2018

apapirovski Jun 7, 2018

Fishrock123 Jun 7, 2018

Fishrock123 Jun 7, 2018

targos Jun 7, 2018

Fishrock123 Jun 7, 2018

mscdex Jun 7, 2018

joyeecheung Jun 8, 2018

jasnell commented Sep 10, 2018

BridgeAR commented Mar 10, 2019

apapirovski commented Mar 21, 2019

apapirovski commented Dec 13, 2019

BridgeAR commented Jan 12, 2020

jasnell commented Jun 19, 2020

timers: allow timers to be used as primitives #21152

timers: allow timers to be used as primitives #21152

Conversation

apapirovski commented Jun 5, 2018

Checklist

apapirovski commented Jun 5, 2018

bmeck commented Jun 5, 2018

mscdex commented Jun 5, 2018

apapirovski commented Jun 5, 2018

jdalton Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jun 6, 2018 • edited Loading

apapirovski commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Sep 10, 2018

BridgeAR commented Mar 10, 2019

apapirovski commented Mar 21, 2019

apapirovski commented Dec 13, 2019

BridgeAR commented Jan 12, 2020

jasnell commented Jun 19, 2020

jdalton Jun 6, 2018 •

edited

Loading

vsemozhetbyt Jun 6, 2018 •

edited

Loading

mscdex commented Jun 6, 2018 •

edited

Loading