timers: enable timers to be used as primitives #19683

bmeck · 2018-03-29T20:25:31Z

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.

Note that this is using the async_id_symbol to get a numeric ID to correspond with timers.

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.

addaleax · 2018-03-29T20:32:29Z

lib/timers.js


 // Schedule or re-schedule a timer.
 // The item must have been enroll()'d first.
 const active = exports.active = function(item) {
+  KNOWN_TIMERS[item[async_id_symbol]] = item;


Can we do the caching in [@@toPrimitive]()? I doubt this is a very frequently used feature, so it might be good if we could avoid the storage overhead for the common case?

seems fine to change that.

not sure how we could easily avoid the delete though when clearing without doing some funny stuff. the cache should swap into dictionary mode really fast though.

mscdex · 2018-03-29T20:34:05Z

Did you check benchmarks?

bmeck · 2018-03-29T20:37:41Z

@mscdex I did not. I would be very surprised if it has any significant impact.

addaleax

LGTM if the benchmarks are happy

This is semver-minor, right? Should we document this?

addaleax · 2018-03-29T20:52:20Z

lib/timers.js

@@ -497,6 +505,12 @@ exports.setInterval = function(callback, repeat, arg1, arg2, arg3) {
 };

 exports.clearInterval = function(timer) {
+  if (typeof timer === 'number' || typeof timer === 'string') {
+    if (timer in KNOWN_TIMERS) {
+      clearInterval(KNOWN_TIMERS[timer]);


I think we may want to avoid accessing global.clearInterval here … maybe doing something like we do for clearTimeout, i.e. const clearInterval = exports.clearInterval = … is a good idea here too?

ah yes, that would be a bug if that happened. I'm not 100% sure on why these functions are not named looking at them...

bmeck · 2018-03-29T21:15:23Z

I ran ./node benchmark/run.js timers but am not seeing anything changing within margin of error.

Trott · 2018-03-29T21:34:29Z

@bmeck You may already know this, but in case not: If you have a binary compiled from master called node-old and a binary compiled with your changes called node-new, you can have it do the statistical computation for you with something like:

node benchmark/compare.js --old node-old --new node-new timers > my-benchmark.csv

...followed by...

cat my-benchmark.csv | Rscript compare.R

See https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md#comparing-nodejs-versions for more information.

Trott · 2018-03-29T21:35:04Z

(Edited above to include correct command...)

addaleax

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

addaleax · 2018-03-29T21:34:48Z

doc/api/timers.md

@@ -90,6 +90,12 @@ of the Node.js application.

 Returns a reference to the `Timeout`.

+### timeout[Symbol.toPrimitive]()
+
+When coercing a `Timeout` to a primitive, a primitive will be generated that can be used with the appropriate function that can be used to clear the `Timeout`. This allows enhanced compatibility with browser `setTimeout`, and `setInterval` implementations.


Nit: Please wrap at 80 characters :)

TimothyGu · 2018-03-29T21:36:38Z

lib/timers.js

+const clearInterval = exports.clearInterval = function(timer) {
+  if (typeof timer === 'number' || typeof timer === 'string') {
+    if (timer in KNOWN_TIMERS) {
+      clearInterval(KNOWN_TIMERS[timer]);


Would timer = KNOWN_TIMERS[timer] work? This would avoid a recursion.

TimothyGu · 2018-03-29T21:37:20Z

doc/api/timers.md

@@ -90,6 +90,12 @@ of the Node.js application.

 Returns a reference to the `Timeout`.

+### timeout[Symbol.toPrimitive]()
+
+When coercing a `Timeout` to a primitive, a primitive will be generated that can be used with the appropriate function that can be used to clear the `Timeout`. This allows enhanced compatibility with browser `setTimeout`, and `setInterval` implementations.


Wrap at 80 cols.

TimothyGu · 2018-03-29T21:37:53Z

doc/api/timers.md

+
+When coercing a `Timeout` to a primitive, a primitive will be generated that can be used with the appropriate function that can be used to clear the `Timeout`. This allows enhanced compatibility with browser `setTimeout`, and `setInterval` implementations.
+
+Returns a `number`.


Put

* Returns: {integer}

right below the heading.

TimothyGu · 2018-03-29T21:38:03Z

lib/timers.js

@@ -145,6 +145,8 @@ const kRefed = Symbol('refed');
 const refedLists = Object.create(null);
 const unrefedLists = Object.create(null);

+const KNOWN_TIMERS = Object.create(null);
+


Extraneous line.

TimothyGu · 2018-03-29T21:38:21Z

lib/timers.js

@@ -521,6 +535,11 @@ function unrefdHandle(timer, now) {
  return true;
 }

+Timeout.prototype[Symbol.toPrimitive] = function() {


Why @@toPrimitive rather than valueOf?

see https://codepen.io/bradleymeck/pen/eMMpdR?editors=0012 , .valueOf is not safe for using as a key. toPrimitive handles both string and number coercion.

addaleax · 2018-03-29T23:07:55Z

There's one significant drop showing in the benchmark CI:

                                          confidence improvement accuracy (*)    (**)   (***)
[...]
timers/timers-cancel-pooled.js millions=5        ***    -48.32 %      ±11.22% ±15.03% ±19.77%

I guess we have to make a call deciding whether we're okay with that?

mscdex

-1 if we're going to see a performance regression like that

bmeck · 2018-03-30T01:01:33Z

It is probably from the uncondition delete in the cancel. We could replace timer.close when toPrimitive is called and make that more conditional at the cost of some odd things possibly showing up if people replace timer.close with a custom function and don't expect us to wrap it. I'll munge with this a bit and see if we can fix up that cliff. Not sure why my test wasn't showing it.

TimothyGu · 2018-03-30T06:52:20Z

lib/timers.js

+      delete KNOWN_TIMERS[this[async_id_symbol]];
+      this.close = $close;
+      this.close();
+    }


This won't pass linting due to missing semicolon. Also, can the function be factored out?

fixed / it cannot be factored out since it closes over $close.

$close is effectively always equal to Timeout.prototype.close, right?

since it is public and mutable, that is not certain.

@bmeck I think it’s fine to just ignore that. You could still run into it anyway, when .close is changed after the [@@toPrimitive]() call…

if they replace it they are probably wrapping it, I'm already ignoring all the ways to avoid getter/setters but don't feel comfortable with completely removing .close this seems like one of the metrics people could be profiling handle lifetimes.

TimothyGu · 2018-03-30T06:53:03Z

lib/timers.js

@@ -496,7 +504,15 @@ exports.setInterval = function(callback, repeat, arg1, arg2, arg3) {
  return timeout;
 };

-exports.clearInterval = function(timer) {
+const clearInterval = exports.clearInterval = function(timer) {


Why the addition of clearInterval variable? Doesn't seem used anywhere.

was added in e32b969 as a bug fix to previous iteration

TimothyGu · 2018-03-30T06:53:14Z

lib/timers.js

+    if (timer in KNOWN_TIMERS) {
+      timer = KNOWN_TIMERS[timer];
+    }
+    else {


This fails linting.

bmeck · 2018-04-02T14:32:00Z

@mscdex with the most change can you recheck your perf concerns?

mscdex · 2018-04-03T13:57:02Z

Benchmarks again: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/151/

Fishrock123

So - this does seem more useful than I thought. (With a request, like a session, you might need to store it in e.g. Redis.)

I'm going to guess the perf drop is due to some polymorphism? Maybe we should run turbolizer against it. I'm not yet going to unblock this yet but I wonder if the perf hit is really that much overall.

We should stress HTTP and see if cancel ticks go up significantly, I suppose.

Fishrock123 · 2018-04-11T22:55:49Z

test/parallel/test-timers-toPrimitive.js

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


Can you check that +timeout1 === timeout1[Symbol.toPrimitive]()?

Fishrock123 · 2018-04-25T15:42:17Z

@bmeck I have a suggestion to make, if I may:

We can still get the functionality here while delaying perf concerns in the following way:

Keep the toPrimitive
Remove the id detection / polymorphism from existing timers methods
Add a getTimerById (or similar) to get a timer object by the primitive id

This has the following advantages:

Allows the funcionality
Avoids polymorphism & existing perf concerns
Allows timers to be refreshed from id (see [DEP0096] DeprecationWarning: timers.unenroll() question #20261 (comment))

If you're still interested, I would gladly review that kind of update. If not, mind if I take the existing commits and adjust?

bmeck · 2018-04-25T23:04:30Z

Im on vacation but would object to those suggested differences.

…

On Thu, Apr 26, 2018, 12:43 AM Jeremiah Senkpiel ***@***.***> wrote: @bmeck <https://github.com/bmeck> I have a suggestion to make, if I may: We can still get the functionality here while delaying perf concerns in the following way: - Keep the toPrimitive - Remove the id detection / polymorphism from existing timers methods - Add a getTimerById (or similar) to get a timer object by the primitive id This has the following advantages: - Allows the funcionality - Avoids polymorphism & existing perf concerns - Allows timers to be refreshed from id (see #20261 (comment) <#20261 (comment)>) If you're still interested, I would gladly review that kind of update. If not, mind if I take the existing commits and adjust? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19683 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOUo9g2Dh_7r20OAuAmEcJDcQNZO8aUks5tsJmtgaJpZM4TA7q-> .

apapirovski · 2018-05-14T16:20:30Z

@bmeck if you still want to pursue this, instead of doing the more expensive in check — you could just set a flag when a timer is converted to a number. In addition, the check within clearTimeout could be reversed. First check whether timer is true & has _onTimeout, then do the typeof check (get the timer, etc.) and call clearTimeout again with the result.

Fishrock123 · 2018-05-18T14:39:30Z

I'd love to see how @apapirovski's suggestions work out.

apapirovski · 2018-06-01T11:09:31Z

@bmeck Do you want to continue on this or can I take over basing on top of your commit (with credit preserved)? I like the idea but I think I've got some solid ideas on how to make it faster, as expressed above.

bmeck · 2018-06-01T13:02:15Z

@apapirovski please take it over, your work is quite exciting :)

apapirovski · 2018-06-01T13:05:36Z

@bmeck thanks! I'll pull in your commit and build on top of it. 👍 I'll preserve the credit when/if I open a PR, depending on how the perf works out.

apapirovski · 2018-06-25T05:28:13Z

I'll close this out since there's a more up-to-date / rebased version. Hopefully we can sort out the remaining issues to land it.

timers: enable timers to be used as primitives

9ac2bd4

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.

nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Mar 29, 2018

bmeck requested review from trevnorris and addaleax March 29, 2018 20:25

addaleax reviewed Mar 29, 2018

View reviewed changes

move cache assignment to toPrimitive call

6101d52

addaleax approved these changes Mar 29, 2018

View reviewed changes

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 29, 2018

doc && fix bug

e32b969

addaleax approved these changes Mar 29, 2018

View reviewed changes

TimothyGu reviewed Mar 29, 2018

View reviewed changes

bmeck added 2 commits March 29, 2018 17:41

nits

c4ec0e5

nits

d963047

mscdex suggested changes Mar 29, 2018

View reviewed changes

only delete if put into cache

54b8894

TimothyGu reviewed Mar 30, 2018

View reviewed changes

linting

d7ae747

Fishrock123 reviewed Apr 11, 2018

View reviewed changes

Fishrock123 mentioned this pull request Apr 25, 2018

[DEP0096] DeprecationWarning: timers.unenroll() question #20261

Closed

apapirovski mentioned this pull request Jun 5, 2018

timers: allow timers to be used as primitives #21152

Closed

4 tasks

apapirovski closed this Jun 25, 2018


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

		Returns a `number`.


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

timers: enable timers to be used as primitives #19683

timers: enable timers to be used as primitives #19683

Conversation

bmeck commented Mar 29, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Mar 29, 2018

bmeck commented Mar 29, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck commented Mar 29, 2018

Trott commented Mar 29, 2018 • edited Loading

Trott commented Mar 29, 2018

addaleax left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Mar 29, 2018

mscdex left a comment

Choose a reason for hiding this comment

bmeck commented Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

bmeck Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck Mar 30, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck commented Apr 2, 2018

mscdex commented Apr 3, 2018 • edited Loading

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Apr 25, 2018

bmeck commented Apr 25, 2018 via email

apapirovski commented May 14, 2018

Fishrock123 commented May 18, 2018

apapirovski commented Jun 1, 2018

bmeck commented Jun 1, 2018

apapirovski commented Jun 1, 2018

apapirovski commented Jun 25, 2018

bmeck commented Mar 29, 2018 •

edited

Loading

Trott commented Mar 29, 2018 •

edited

Loading

bmeck commented Mar 30, 2018 •

edited

Loading

bmeck Mar 30, 2018 •

edited

Loading

bmeck Mar 30, 2018 •

edited

Loading

mscdex commented Apr 3, 2018 •

edited

Loading