timers: attach listOnTimeout function to TimerWrap

[mcollina]

This avoid continuous deoptimizations cycles in V8 for the listOnTimeout function by attaching it to the TimeWrap prototype.
It improves insertion and cancellation time of unpooled timers by 18% and 28% respectively.

$ cat timewrap.csv | Rscript benchmark/compare.R
                                                    improvement confidence      p.value
 timers/immediate.js type="breadth" thousands=5000      -1.23 %            6.471883e-01
 timers/immediate.js type="breadth1" thousands=5000      6.23 %          * 4.589025e-02
 timers/immediate.js type="breadth4" thousands=5000     -0.28 %            8.716988e-01
 timers/immediate.js type="clear" thousands=5000         1.61 %            2.157496e-01
 timers/immediate.js type="depth" thousands=5000         0.68 %            2.223863e-01
 timers/immediate.js type="depth1" thousands=5000       -0.26 %            7.391597e-01
 timers/set-immediate-breadth-args.js millions=5        -1.31 %            4.186285e-01
 timers/set-immediate-breadth.js millions=10             3.01 %            2.234333e-01
 timers/set-immediate-depth-args.js millions=5           0.92 %            6.154973e-01
 timers/timers-breadth.js thousands=5000                 1.78 %            4.422721e-01
 timers/timers-cancel-pooled.js millions=5               0.78 %            5.467502e-01
 timers/timers-cancel-unpooled.js millions=1            27.60 %        *** 1.806596e-59
 timers/timers-depth.js thousands=1                      0.32 %            5.568216e-01
 timers/timers-insert-pooled.js millions=5               0.47 %            7.908426e-01
 timers/timers-insert-unpooled.js millions=1            18.19 %        *** 4.666303e-46
 timers/timers-timeout-pooled.js millions=10            -0.69 %            4.534725e-01

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

Affected core subsystem(s)

timers

[mcollina]

This was done at JsConf.Asia at the code in the dark even with @lrlna.

[mcollina]

@bmeurer what do you think?

[hashseed]

I just ran timers/timers-cancel-unpooled.js on fa33f02 with and without the patch and counted the same number of deopts .

But the performance indeed seems to have improved.

I think this is not due to deopts, but rather due to inlining. I ran the same benchmark with and without the patch like this:

node --trace-opt --trace-turbo-inlining benchmark/timers/timers-cancel-unpooled.js | grep Inlining

And diff'ed the output:

1d0
< Inlining remove into append
10a10,19
> Inlining insert into exports.active
> Inlining small function(s) at call site #138:JSCall
> Inlining debugs.(anonymous function) into exports.active
> Inlining small function(s) at call site #231:JSCall
> Inlining isEmpty into exports.active
> Inlining small function(s) at call site #239:JSCall
> Inlining ok into exports.active
> Inlining append into exports.active
> Inlining TimersList into exports.active
> Inlining remove into exports.active
11a21
> Inlining remove into append

[hashseed]

I would suggest to only make this change if it semantically makes sense to. It does from what I can tell.

If you are only making this change to improve performance, I'd wait until we have figured out whether we can improve that in V8.

[hashseed]

Also filed V8 bug here.

[mcollina]

@hashseed I can reproduce the problem on Node 8, which has the same line (

node/lib/timers.js

Line 197 in 96e8250

list._timer[kOnTimeout] = listOnTimeout;

). This is a good candidate for a backport down to 8 and 9, as I'm not sure if we could update the V8 further on that line.

I'll update the description and the comment to note that it is related to inlining rather than deopts.

[mcollina]

CI: https://ci.nodejs.org/job/node-test-pull-request/12746/

[hashseed]

What I'm saying is that this is an observable change. If this is fine from a semantic point of view, fine. If that's not intended, but only to reap performance benefits, it might be better to wait for V8 to fix this?

[mcollina]

What I'm saying is that this is an observable change. If this is fine from a semantic point of view, fine. If that's not intended, but only to reap performance benefits, it might be better to wait for V8 to fix this?

This is fine from a semantic point of view, but I would not have touched this if it wasn't for the performance benefits. I do not think we could backport the future version of V8 that would have a fix for this in Node 8 - and it would be handy to land this patch there sooner rather than later.

I think fixing it in V8 would be great, as this is a pattern that I have seen several times.

[Fishrock123]

I would suggest to only make this change if it semantically makes sense to. It does from what I can tell.

It's fine semantically.

It improves insertion and cancellation time of new timers by 18% and 28% respectively.

Ok so, this is not 100% true.

Your results say this:

 timers/timers-cancel-pooled.js millions=5               0.78 %            5.467502e-01
 timers/timers-cancel-unpooled.js millions=1            27.60 %        *** 1.806596e-59
 timers/timers-insert-pooled.js millions=5               0.47 %            7.908426e-01
 timers/timers-insert-unpooled.js millions=1            18.19 %        *** 4.666303e-46

Those are unpooled timers that benefit from this. Unpooled timers almost never exist in the wild like this. No one (to my best understanding) continuously schedules timers with unique durations.

So really, It's hard to be convinced that inlining is the issue here either - perhaps it's more like an initial bind kind of thing. This function should be called just as much for the pooled benchmarks as the unpooled benchmarks afaik. (Not that I know more about @hashseed about this, lol)

tl;dr the perf benefit is likely next to none in any reality unfortunately

[mcollina]

tl;dr the perf benefit is likely next to none in any reality unfortunately

Yes and no. This came out from testing an HTTP route when adding some delay, and it yielded a nice 10% throughput increase. However, I agree this is a case that very seldomly applies to real world applications and more with synthethic benchmarks.

I've updated the commit message and the PR description.

[mcollina]

Landed as eb34278.

[MylesBorins]

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.