| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/11313/ |
Sorry, something went wrong.
|
Note: this will need adjustment to test/message/stdin_messages.out once #16869 lands. |
Sorry, something went wrong.
|
Ok, so it seems like the CI benchmark turned out worse than my local testing: process/next-tick-breadth-args.js millions=2 -4.39 % 0.054449966 process/next-tick-breadth.js millions=2 -0.39 % 0.757371770 process/next-tick-depth-args.js millions=12 -4.27 % ** 0.002636105 process/next-tick-depth.js millions=12 -1.81 % 0.237992442 Doing a second run just to be sure: |
Sorry, something went wrong.
|
Ok, I'm pretty sure that wasn't necessarily reflective. I think there is likely a slight performance dip but it's not consistent and very minor. process/bench-env.js n=100000 -0.48 % 0.8152647 process/bench-hrtime.js type="diff" n=1000000 -2.34 % 0.2048529 process/bench-hrtime.js type="raw" n=1000000 1.11 % 0.5847665 process/memoryUsage.js n=100000 -0.15 % 0.8392653 process/next-tick-breadth-args.js millions=2 0.95 % 0.6382626 process/next-tick-breadth.js millions=2 -0.19 % 0.8283266 process/next-tick-depth-args.js millions=12 -1.54 % 0.2947018 process/next-tick-depth.js millions=12 -0.13 % 0.9288224 Third run: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/26/ |
Sorry, something went wrong.
|
Yep, the results are worse after all. Unless we're ok with 2-4% decline on the depth benchmark I'll probably have to close this. process/bench-env.js n=100000 1.07 % 0.338564916 process/bench-hrtime.js type="diff" n=1000000 0.69 % 0.692105626 process/bench-hrtime.js type="raw" n=1000000 0.32 % 0.838770747 process/memoryUsage.js n=100000 0.54 % 0.340006562 process/next-tick-breadth-args.js millions=2 -2.65 % 0.204050747 process/next-tick-breadth.js millions=2 -0.64 % 0.706830203 process/next-tick-depth-args.js millions=12 -4.14 % *** 0.000258298 process/next-tick-depth.js millions=12 -0.19 % 0.908582943 |
Sorry, something went wrong.
|
Seems like this should probably not be backported, or only to 8.x? |
Sorry, something went wrong.
|
@Fishrock123 Definitely shouldn't. I don't even know if this can go on 9.x to be honest given the slight decline in performance. |
Sorry, something went wrong.
|
Ok, I think there's no performance regression after bumping up the iterations to remove any noise. process/next-tick-breadth-args.js millions=8 -1.39 % 0.4175407 process/next-tick-breadth.js millions=8 0.78 % 0.3573891 process/next-tick-depth-args.js millions=50 -0.69 % 0.5951104 process/next-tick-depth.js millions=50 -0.76 % 0.6042670 |
Sorry, something went wrong.
|
@apapirovski the benchmarks don't show any significance (huge p-value, except for next-tick-depth-args.js) so it should be considered as if there's no difference in performance. |
Sorry, something went wrong.
|
@refack yeah, I was just worried about process/next-tick-depth-args.js millions=12 -4.14 % *** 0.000258298 but it seems like it was noise after bumping up iterations and running it more times. |
Sorry, something went wrong.
So we got: process/next-tick-depth-args.js millions=50 -0.69 % 0.5951104 I'm trying to think about way to improve the benchmarks to see if there is a significant pattern after all. |
Sorry, something went wrong.
Sorry, I don't quite understand this. Why we can't do that? It seems that the nextTick function should be of great significance for process.nextTick update: process/next-tick-breadth-args.js millions=2 -23.57 % *** 2.046862e-50 |
Sorry, something went wrong.
|
[refack: I pushed a change, then pushed it out, because I wanted to use the benchmark machine] |
Sorry, something went wrong.
|
A couple more runs: process/next-tick-breadth-args.js millions=8 -0.26 % 0.820344067 process/next-tick-breadth.js millions=8 -0.03 % 0.921366382 process/next-tick-depth-args.js millions=50 -0.90 % 0.179757893 process/next-tick-depth.js millions=50 -2.77 % ** 0.003797703 process/next-tick-breadth-args.js millions=8 0.07 % 0.9475671 process/next-tick-breadth.js millions=8 0.05 % 0.8747827 process/next-tick-depth-args.js millions=50 0.08 % 0.9172713 process/next-tick-depth.js millions=50 -0.89 % 0.3620226 Just looking at the raw data, I can see that both the old code and the new code seem to end up in situations where they're either deoptimized or just not optimized by V8. |
Sorry, something went wrong.
|
I'll probably spend some more time looking the nextTick code. The fact that there's such huge peaks and valleys indicates that there's something off in both versions. |
Sorry, something went wrong.
|
Here's the results from @refack's experiment with many runs (and low n value): process/next-tick-breadth-args.js millions=1 -0.70 % 0.0869824873 process/next-tick-breadth.js millions=1 -0.19 % 0.5451670018 process/next-tick-depth-args.js millions=1 1.29 % *** 0.0007451827 process/next-tick-depth.js millions=1 -0.99 % 0.0669916919 |
Sorry, something went wrong.
|
I think the real issue here is basically that those benchmarks aren't really testing these particular changes, or at least not in a very representative way. Most of the difference seems to have to do with GC kicking in or not. On my local system I get none of the extreme dips and it's all pretty even. I created two benchmarks that should do a better job of testing the actual execution performance within _tickCallback. Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/39/ |
Sorry, something went wrong.
|
Ok, I think that got us more reliable info... 1st run: process/next-tick-exec-args.js millions=5 -0.45 % 6.589957e-01 process/next-tick-exec.js millions=5 -2.85 % *** 1.436700e-08 2nd run: process/next-tick-exec-args.js millions=5 -2.22 % *** 4.588545e-04 process/next-tick-exec.js millions=5 -3.11 % *** 6.527721e-17 |
Sorry, something went wrong.
|
I think at least some of the confusion here has to do with how you use statistics. In simple words: This means that the only conculsion (and this concusion will likely be wrong) that you can arrive at by running the benchmarks multiplie times, is that there is a difference. But you only conculude this because of the randomness, not because the difference is actually there. If you think there is a garbage collection issue that interfers with the results (and there may very well be). Then you need to analyse this issue without using the benchmark suite. You can use the trace_events or --expose-gc to debug this. Once you have made a benchmark that you belive accurately represents what you are trying to measure, then you can use the benchmark suite. Statisticians (especially frequentistic statisticians which is what is the base for our benchmark suite) don't like to admit that there are belifes in statestics, but the unfortunete truth is that there is :(
Since you are trying to prove that there is no difference, it will always have too high a variance. I discussed a bit in #8139 what consequences a high variance has. It is especially important that you understand that a high variance is fine if there is enogth runs. Here are some outputs of that:
Let's be super specific. Let's say we are looking for at least a 1% difference and we have 100 runs then we should have a coefficient of variation (cv) less than 0.035. In this case we have (data from https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/39/) the coefficient of variation (cv.unbiased) to be: filename configuration mean std.dev cv.biased cv.unbiased conf.int
process/next-tick-exec-args.js millions=5 1.677843 0.1206786 0.07192487 0.07201478 0.01682723
process/next-tick-exec.js millions=5 2.818856 0.1051820 0.03731372 0.03736036 0.01466641
As you can see the variance for process/next-tick-exec.js is only slightly too high. And for process/next-tick-exec-args.js it is differently too high to find a difference of 1%. An important assumption in these calculations, is that they assume the variance is the same. But this does appear to be somewhat so. You can look at the F-test (var.test in R) if you like.
Don't expect it to be normal. Time is typically gamma distributed, because of the central limit theorm it will approach a normal distribution as you increase the number of iterations. But it will never become normally distributed. This is theoretically unfortunete, as the t-test assumes a normal distribution, however in practise it is not a huge deal. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM but it needs a rebase
Sorry, something went wrong.
|
I've resolved all performance regressions in the latest version. process/next-tick-breadth-args.js millions=4 0.12 % 9.122406e-01 process/next-tick-breadth.js millions=4 2.10 % *** 4.341579e-06 process/next-tick-depth-args.js millions=12 1.31 % ** 3.382566e-03 process/next-tick-depth.js millions=12 1.52 % * 3.364892e-02 process/next-tick-exec-args.js millions=5 -1.32 % 8.180421e-02 process/next-tick-exec.js millions=5 0.84 % 1.020348e-01 PTAL @addaleax, @refack, @cjihrig and others. This should be ready to land after the weekend. |
Sorry, something went wrong.
Get rid of separate function to call callback from _tickCallback as it no longer yields worthwhile performance improvement. Move some code from nextTick & internalNextTick into TickObject constructor to minimize duplication.
Get rid of separate function to call callback from _tickCallback as it no longer yields worthwhile performance improvement. Move some code from nextTick & internalNextTick into TickObject constructor to minimize duplication. PR-URL: #16888 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
|
Landed in cbaf59c. Thanks everyone for the reviews & patience in getting this code tightened up. |
Sorry, something went wrong.
Get rid of separate function to call callback from _tickCallback as it no longer yields worthwhile performance improvement. Move some code from nextTick & internalNextTick into TickObject constructor to minimize duplication. PR-URL: #16888 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Get rid of separate function to call callback from _tickCallback as it no longer yields worthwhile performance improvement. Move some code from nextTick & internalNextTick into TickObject constructor to minimize duplication. PR-URL: #16888 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Get rid of separate function to call callback from _tickCallback as it no longer yields worthwhile performance improvement. Move some code from nextTick & internalNextTick into TickObject constructor to minimize duplication. PR-URL: #16888 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
| Back | FazBrowse Home | New Git URL |
This PR gets rid of a separate function to execute the callback from _tickCallback as it no longer yields a performance benefit.
Unlike with emit this doesn't yield an improvement on any of our benchmarks but since it also doesn't make them worse, I feel this is probably a worthwhile change (simpler code and less unnecessary internals in stack traces).
Unfortunately we still can't switch to using spread operator in nextTick instead of the manual arguments copying. We can follow up on that in the next few V8 versions.
Checklist
Affected core subsystem(s)
process