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

node: improve nextTick performance #1571

Closed
wants to merge 1 commit into from
Closed

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented May 1, 2015

This commit uses separate functions to isolate deopts caused by
try-finallys and avoids fn.apply() for callbacks with small numbers
of arguments.

These changes improve performance by ~2-42% in the various
nextTick benchmarks.

@mscdex mscdex force-pushed the nexttick-perf branch 2 times, most recently from d09cb2f to 1802b8d Compare May 1, 2015 06:04
@mscdex
Copy link
Contributor Author

mscdex commented May 1, 2015

/cc @trevnorris @bnoordhuis

callback();
threw = false;
} finally {}
return threw;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of a try { ... } finally {} with an empty finalizer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied from the original function (_tickCallback/_tickDomainCallback). I can safely delete it I guess? Unless it was originally supposed to catch exceptions?

Copy link
Contributor

Choose a reason for hiding this comment

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

in _tickCallback there's code in the finalizer – if control passes to the finalizer, tickDone is called before the exception propagates as usual. It should be replicable by moving the try {} finally { if (threw) tickDone() } back up into _tickCallback, wrapping these lines in the TryClause.

@mscdex
Copy link
Contributor Author

mscdex commented May 1, 2015

@chrisdickinson I've adjusted the benchmarks, is that what you had in mind?

else if (i % 2 === 0)
process.nextTick(cb2, false, 5.1);
else
process.nextTick(cb1, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

might add a case for process.nextTick(cb) by itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I should merge the pre-existing non-args benchmark with this one?

@chrisdickinson
Copy link
Contributor

@mscdex re: benchmarks, yep! I had one other pony request relating to them (related to including process.nextTick(cb) with no curried args.)

@@ -0,0 +1,36 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

'use strict'?

@trevnorris
Copy link
Contributor

A bit verbose, but if it offers the performance improvements you said then I'm okay with the change.

} finally {
if (threw)
tickDone();
args = tock.args;
Copy link
Contributor

Choose a reason for hiding this comment

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

a nit: would you mind adding a comment briefly explaining why this call is separated into a switch statement, for future readers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

This commit uses separate functions to isolate deopts caused by
try-catches and avoids fn.apply() for callbacks with small numbers
of arguments.

These changes improve performance by ~1-40% in the various
nextTick benchmarks.
@chrisdickinson
Copy link
Contributor

Cool – after the changes, are the numbers still ~2-42% better than the previous implementation? If so, LGTM!

@mscdex
Copy link
Contributor Author

mscdex commented May 1, 2015

@chrisdickinson Yep, performance is still in the same range.

mscdex added a commit that referenced this pull request May 2, 2015
This commit uses separate functions to isolate deopts caused by
try-catches and avoids fn.apply() for callbacks with small numbers
of arguments.

These changes improve performance by ~1-40% in the various
nextTick benchmarks.

PR-URL: #1571
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
@mscdex
Copy link
Contributor Author

mscdex commented May 2, 2015

Landed in c7782c0.

@mscdex mscdex closed this May 2, 2015
@mscdex mscdex deleted the nexttick-perf branch May 2, 2015 01:31
@rvagg rvagg mentioned this pull request May 2, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
This commit uses separate functions to isolate deopts caused by
try-catches and avoids fn.apply() for callbacks with small numbers
of arguments.

These changes improve performance by ~1-40% in the various
nextTick benchmarks.

PR-URL: nodejs#1571
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants