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

process: improve nextTick performance #25461

Merged
merged 0 commits into from
Aug 28, 2019

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Jan 12, 2019

                                               confidence improvement accuracy (*)    (**)   (***)
 process/next-tick-breadth-args.js n=10000000        ***     17.11 %       ±2.80%  ±3.74%  ±4.89%
 process/next-tick-breadth.js n=10000000             ***     40.88 %       ±2.72%  ±3.62%  ±4.71%
 process/next-tick-depth-args.js n=7000000           ***     21.87 %       ±2.18%  ±2.92%  ±3.82%
 process/next-tick-depth.js n=7000000                ***     23.58 %       ±7.60% ±10.12% ±13.17%
 process/next-tick-exec-args.js n=4000000            ***     39.07 %       ±1.88%  ±2.51%  ±3.28%
 process/next-tick-exec.js n=4000000                 ***     44.92 %       ±1.67%  ±2.23%  ±2.91%
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@mscdex mscdex added process Issues and PRs related to the process subsystem. performance Issues and PRs related to the performance of Node.js. labels Jan 12, 2019
@nodejs-github-bot
Copy link
Collaborator

@mscdex sadly an error occured when I tried to trigger a build :(

@@ -1,7 +1,7 @@
'use strict';
const common = require('../common.js');
const bench = common.createBenchmark(main, {
n: [5e6]
n: [4e6]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were changed to match those found in the other breadth benchmarks and it also seems to provide more stable results.

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

The fact that we’re emitting one object on init and then pushing another on the next tick queue goes against basic expectations of using async hooks and what the resource is supposed to represent.

lib/internal/process/next_tick.js Outdated Show resolved Hide resolved
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 19, 2019
@mscdex
Copy link
Contributor Author

mscdex commented Jan 21, 2019

Since there are some that believe this should be semver-major, ping @nodejs/tsc

@mscdex
Copy link
Contributor Author

mscdex commented Jan 28, 2019

ping?

@mcollina
Copy link
Member

In the context of exposing the current async resource rather than just exposing the asyncId, this change might require to be reverted/changed later. However that work is not settled yet, so we might want to land this anyway, as this code is not really ready for it yet.

As an example, we are pursuing this change: #25094 for that reason.

cc @nodejs/diagnostics

@mscdex
Copy link
Contributor Author

mscdex commented Feb 5, 2019

ping @nodejs/tsc once more

@Trott
Copy link
Member

Trott commented Feb 5, 2019

There probably won't be a meeting this week, but I'm going to throw a tsc-agenda label on this to make sure it doesn't entirely fall off the TSC radar for a third time.

@Trott Trott added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 5, 2019
@Trott
Copy link
Member

Trott commented Feb 5, 2019

(Obviously, if resolution is achieved before the next TSC meeting, that's awesome and I'll be delighted to remove the label at that time.)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Thanks for working on nextTick! I've got some questions.

I'm failing to understand why this changes improves performance, and if it may something related to our benchmarks.

Have you tried if moving just the emitInit call outside of the constructor generates the same result?
Have you verified that is using the same object for both emitInit and queue.push() is what is actually creating the performance improvements?

@mscdex have you experiences this improves things in a more realistic scenario including I/O?

Also cc @bmeurer who might provide some insights.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 8, 2019

Have you tried if moving just the emitInit call outside of the constructor generates the same result?
Have you verified that is using the same object for both emitInit and queue.push() is what is actually creating the performance improvements?

@mcollina I honestly don't remember now, I tried a lot of variations though at the time, and this was the only one that resulted in a positive improvement with no regressions.

@bmeurer
Copy link
Member

bmeurer commented Feb 8, 2019

The change looks reasonable to me (JavaScript wise), but I'm really not a good candidate to review this, since this is not my area of expertise.

@mcollina
Copy link
Member

mcollina commented Feb 8, 2019

@bmeurer have you got a clue on why this is faster than the current one?

@bmeurer
Copy link
Member

bmeurer commented Feb 8, 2019

@mcollina Nope, sorry.

@apapirovski
Copy link
Member

have you got a clue on why this is faster than the current one?

As far as I can tell, this should be mostly related to using symbols which are in my experience slower for both getting and setting properties (and for your version the fact that we use an if condition rather than just running the emitInit function). That said, happy to be corrected.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

Ping @mscdex
Please check #25461 (comment)

@mscdex mscdex requested a review from BridgeAR August 24, 2019 17:28
@mscdex
Copy link
Contributor Author

mscdex commented Aug 24, 2019

I've made different changes now to avoid the issues with domain. The new benchmark results are:

                                               confidence improvement accuracy (*)    (**)   (***)
 process/next-tick-breadth-args.js n=10000000        ***     17.11 %       ±2.80%  ±3.74%  ±4.89%
 process/next-tick-breadth.js n=10000000             ***     40.88 %       ±2.72%  ±3.62%  ±4.71%
 process/next-tick-depth-args.js n=7000000           ***     21.87 %       ±2.18%  ±2.92%  ±3.82%
 process/next-tick-depth.js n=7000000                ***     23.58 %       ±7.60% ±10.12% ±13.17%
 process/next-tick-exec-args.js n=4000000            ***     39.07 %       ±1.88%  ±2.51%  ±3.28%
 process/next-tick-exec.js n=4000000                 ***     44.92 %       ±1.67%  ±2.23%  ±2.91%

@mscdex mscdex removed semver-major PRs that contain breaking changes and should be released in the next major version. tsc-agenda Issues and PRs to discuss during the meetings of the TSC. labels Aug 24, 2019
@mscdex
Copy link
Contributor Author

mscdex commented Aug 25, 2019

/cc @nodejs/collaborators

@mcollina
Copy link
Member

@mscdex does it still fail the test in #25461 (comment)? Maybe we should add it to our suite.

@mscdex
Copy link
Contributor Author

mscdex commented Aug 25, 2019

@mcollina It should not fail it because the same object is being used now. These changes are now more or less inlining the previous custom TickObject class constructor code.

@mcollina
Copy link
Member

Would you mind adding that test to this PR? Code LGTM.

@mscdex
Copy link
Contributor Author

mscdex commented Aug 26, 2019

@mcollina Honestly I think that's probably best left to a separate issue/PR about whether we should (explicitly) support modifying behavior like that from an async hook callback.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

callback(...tock.args);
} else {
const args = tock.args;
switch (args.length) {
Copy link
Member

Choose a reason for hiding this comment

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

@hashseed @bmeurer why is this still faster? I thought this optimization of specializing the argument length explicitly in the code is no longer required?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure - how much of the improvement is accounted for by this alone (and not the other part of this CL)?

Glancing at e.g. next-tick-depth-args, it doesn't look like the arguments to the callbacks are ever used - maybe we optimize something there. We don't do analysis like that for spread calls

Copy link
Member

Choose a reason for hiding this comment

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

@psmarshall the reason I was concerned is that we removed this optimization from certain parts of the code before.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's possible to construct a microbenchmark where one or the other is faster - I don't think you could measure the difference on a larger application that does a lot of work between ticks. With that in mind my preference is for the spread-call version but I also don't really have the time to do a detailed analysis of what's going on in this specific case so I'm fine either way.

@mscdex mscdex closed this Aug 28, 2019
@mscdex mscdex merged commit 34961c7 into nodejs:master Aug 28, 2019
@mscdex mscdex deleted the process-nexttick-perf branch August 28, 2019 02:17
Trott pushed a commit that referenced this pull request Sep 2, 2019
Making `.incRef()` and `.decRef()` fail silently leads to better error
messages when trying to access the underlying value (as opposed to
crashing inside these methods).

Refs: #25461 (comment)

PR-URL: #29289
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
PR-URL: #25461
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
Making `.incRef()` and `.decRef()` fail silently leads to better error
messages when trying to access the underlying value (as opposed to
crashing inside these methods).

Refs: #25461 (comment)

PR-URL: #29289
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
BridgeAR pushed a commit that referenced this pull request Sep 4, 2019
PR-URL: #25461
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BridgeAR pushed a commit that referenced this pull request Sep 4, 2019
Making `.incRef()` and `.decRef()` fail silently leads to better error
messages when trying to access the underlying value (as opposed to
crashing inside these methods).

Refs: #25461 (comment)

PR-URL: #29289
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues and PRs related to the performance of Node.js. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.