timers: improve setImmediate() performance by mscdex · Pull Request #4169 · nodejs/node

mscdex · 2015-12-06T06:38:35Z

This commit improves setImmediate() performance by moving the try-finally block that wraps callback execution into a separate function because currently v8 never tries to optimize functions that contain try-finally blocks.

With this change, there is a ~20-40% improvement in the included setImmediate() depth benchmarks. The breadth benchmarks show a slight improvement.

JungMinu · 2015-12-06T11:24:13Z

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

JungMinu · 2015-12-06T11:57:15Z

CI is happy except for unrelated Windows test :)

JungMinu · 2015-12-06T12:13:07Z

LGTM @Fishrock123 LGTY? :)

Fishrock123 · 2015-12-06T16:21:21Z

lib/timers.js

Hah, beat me to it I see. :P

I was going to leave this until after #4007, in part so that it would look similar.

I suggest we keep the naming consistent in some way with https://github.com/nodejs/node/pull/4007/files#diff-0a5d4868b2b9b17cf9e2c11f1bd1311eR177

I've been using the try prefix with the try-catch/finally isolation PRs I've submitted in the past as that seems (to me) to make it more clear as to what the isolation functions are doing. shrug

I changed the one in #4007 to _tryOnTimeout, keeping the underspace from a prior incantation in internal timeouts. Mind adding it to this then?

jasnell · 2015-12-11T18:44:39Z

LGTM so long as @Fishrock123 is happy

bnoordhuis · 2015-12-16T15:35:42Z

benchmark/misc/set-immediate-breadth.js

Forgot a 'use strict'?

bnoordhuis · 2015-12-16T15:42:59Z

I'd rewrite the benchmarks so that the callbacks don't reference free variables (including bench.end().) Right now, they appear to be creating 5 or 10 million closures, making it as much a GC benchmark as a timers benchmark.

jasnell · 2016-01-15T00:55:59Z

@mscdex @Fishrock123 ... ping :-)

jasnell · 2016-01-15T00:56:47Z

benchmark/misc/set-immediate-breadth-args.js

mscdex · 2016-01-15T05:01:30Z

@bnoordhuis I tried to rewrite the benchmarks according to your suggestions, but at least judging by the results of --trace-gc, I didn't see any difference. Let me know if it now looks better to you.

@jasnell Updated per your suggestions too.

Fishrock123 · 2016-01-19T03:03:25Z

I'll try to look tomorrow

mscdex · 2016-02-07T05:56:12Z

@bnoordhuis @Fishrock123 ping

jasnell · 2016-02-07T21:18:28Z

@mscdex .. LGTM

bnoordhuis · 2016-02-08T17:50:06Z

LGTM

Fishrock123 · 2016-02-08T20:02:37Z

Will take a look when I get back home

jasnell · 2016-02-10T04:38:43Z

Marking this lts-watch but it would definitely need to sit for a while in master and v5 before pulling back

mscdex · 2016-02-21T07:39:46Z

@Fishrock123 Does this LGTY?

Fishrock123 · 2016-02-21T15:20:10Z

lib/timers.js

early return to avoid more indentation?

If you return early in a finally block, the exception wouldn't be thrown, which would change behavior.

I'm not sure I follow, how would that change?

The try - finally will still catch the exception?

If you do something like

function foo() { try { throw new Error('foo'); } finally { return; } }

the exception won't be thrown and execution will continue normally (as if you had done try { } catch (e) {}, but without access to the exception object). If you remove the return from the finally block, execution will continue to the end of the finally block at which point the exception will be thrown.

Hmmm I was under the impression we swallowed it for some reason, I guess this allows it to bubble up to the console?

I would assume so. Only try {} catch(e) {} or try {} finally { ... return; ... } can swallow exceptions.

Fishrock123 · 2016-02-21T15:22:59Z

New CI with rebase onto master: https://ci.nodejs.org/job/node-test-pull-request/1713/

Fishrock123 · 2016-02-21T15:24:10Z

it would definitely need to sit for a while in master and v5 before pulling back

I disagree. The implementation is effectively identical. This should land like any other patch fix.

Fishrock123 · 2016-02-21T15:26:17Z

lib/timers.js

Can you move the deopt comment to here and clarify it a tad, ala https://github.com/nodejs/node/pull/4007/files#diff-0a5d4868b2b9b17cf9e2c11f1bd1311eR178 ? (I do need to add the v8 version to mine also..)

rename to match Brian’s setImmediate() refactor in nodejs#4169

mscdex · 2016-03-18T03:18:35Z

Made suggested changes. CI: https://ci.nodejs.org/job/node-test-pull-request/1953/

mscdex · 2016-03-18T03:57:41Z

@Fishrock123 LGTY?

Fishrock123 · 2016-03-18T19:42:15Z

@mscdex lgtm

This commit improves setImmediate() performance by moving the try-finally block that wraps callback execution into a separate function because currently v8 never tries to optimize functions that contain try-finally blocks. With this change, there is a ~20-40% improvement in the included setImmediate() depth benchmarks. The breadth benchmarks show a slight improvement. PR-URL: nodejs#4169 Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

This commit improves setImmediate() performance by moving the try-finally block that wraps callback execution into a separate function because currently v8 never tries to optimize functions that contain try-finally blocks. With this change, there is a ~20-40% improvement in the included setImmediate() depth benchmarks. The breadth benchmarks show a slight improvement. PR-URL: #4169 Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

Notable changes: * buffer: Now properly throws RangeErrors on out-of-bounds writes (Matt Loring) #5605 - This effects write{Float|Double} when the noAssert option is not used. * timers: - Returned timeout objects now have a Timeout constructor name (Jeremiah Senkpiel) #5793 - Performance of Immediate processing is now ~20-40% faster (Brian White) #4169 * vm: Fixed a contextify regression introduced in v5.9.0 (Ali Ijaz Sheikh) #5800 PR-URL: #5831

Notable changes: * buffer: Now properly throws RangeErrors on out-of-bounds writes (Matt Loring) nodejs#5605 - This effects write{Float|Double} when the noAssert option is not used. * timers: - Returned timeout objects now have a Timeout constructor name (Jeremiah Senkpiel) nodejs#5793 - Performance of Immediate processing is now ~20-40% faster (Brian White) nodejs#4169 * vm: Fixed a contextify regression introduced in v5.9.0 (Ali Ijaz Sheikh) nodejs#5800 PR-URL: nodejs#5831

MylesBorins · 2016-08-30T14:10:41Z

Setting as don't land for the same reason as #4007 (comment)

Keeping the timers changes in v6. Open to discussion

mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Dec 6, 2015

Fishrock123 reviewed Dec 6, 2015
View reviewed changes

bnoordhuis reviewed Dec 16, 2015
View reviewed changes

benchmark/misc/set-immediate-breadth.js

Copy link

Member

bnoordhuis Dec 16, 2015

Choose a reason for hiding this comment

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

Forgot a 'use strict'?

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:01

jasnell reviewed Jan 15, 2016
View reviewed changes

benchmark/misc/set-immediate-breadth-args.js Outdated

Copy link

Member

jasnell Jan 15, 2016

Choose a reason for hiding this comment

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

const ?

mscdex force-pushed the perf-setimmediate branch from d3ec30d to 74adf50 Compare January 15, 2016 04:59

jasnell added the lts-watch-v4.x label Feb 10, 2016

ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Feb 16, 2016

Fishrock123 reviewed Feb 21, 2016
View reviewed changes

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Feb 25, 2016

timers: (fixup) remove _ in tryOnTimeout

4120c5e

rename to match Brian’s setImmediate() refactor in nodejs#4169

mscdex force-pushed the perf-setimmediate branch 2 times, most recently from bd485e9 to 75e02de Compare March 18, 2016 03:17

mscdex force-pushed the perf-setimmediate branch from 75e02de to 089bef0 Compare March 18, 2016 23:29

mscdex merged commit 089bef0 into nodejs:master Mar 18, 2016

mscdex deleted the perf-setimmediate branch March 18, 2016 23:30

Fishrock123 mentioned this pull request Mar 22, 2016

v5.9.1 Release Proposal #5831

Merged

mscdex mentioned this pull request Apr 28, 2016

3x faster setImmediate #6436

Closed

3 tasks

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Aug 30, 2016

Uh oh!

Conversation

mscdex commented Dec 6, 2015

Uh oh!

JungMinu commented Dec 6, 2015

Uh oh!

JungMinu commented Dec 6, 2015

Uh oh!

JungMinu commented Dec 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Dec 16, 2015

Uh oh!

jasnell commented Jan 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jan 15, 2016

Uh oh!

Fishrock123 commented Jan 19, 2016

Uh oh!

mscdex commented Feb 7, 2016

Uh oh!

jasnell commented Feb 7, 2016

Uh oh!

bnoordhuis commented Feb 8, 2016

Uh oh!

Fishrock123 commented Feb 8, 2016

Uh oh!

jasnell commented Feb 10, 2016

Uh oh!

mscdex commented Feb 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Feb 21, 2016

Uh oh!

Fishrock123 commented Feb 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Mar 18, 2016

Uh oh!

mscdex commented Mar 18, 2016

Uh oh!

Fishrock123 commented Mar 18, 2016

Uh oh!

MylesBorins commented Aug 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants