lib: remove outdated optimizations by starkwang · Pull Request #27380 · nodejs/node

starkwang · 2019-04-24T06:42:58Z

These two optimizations is outdated in current v8.

Benchmark results:

 confidence improvement accuracy (*) (**) (***)
process/next-tick-breadth-args.js n=4000000 -0.02 % ±0.75% ±1.00% ±1.31%
process/next-tick-breadth.js n=4000000 -0.23 % ±1.78% ±2.38% ±3.13%
process/next-tick-depth-args.js n=12000000 -0.34 % ±0.69% ±0.93% ±1.21%
process/next-tick-depth.js n=12000000 -0.12 % ±1.46% ±1.95% ±2.54%
process/next-tick-exec-args.js n=5000000 2.18 % ±4.03% ±5.37% ±7.01%
process/next-tick-exec.js n=5000000 0.43 % ±2.18% ±2.91% ±3.79%
timers/immediate.js type='breadth' n=5000000 0.39 % ±1.85% ±2.46% ±3.22%
timers/immediate.js type='breadth1' n=5000000 -1.19 % ±2.43% ±3.24% ±4.23%
timers/immediate.js type='breadth4' n=5000000 -2.37 % ±3.80% ±5.06% ±6.60%
timers/immediate.js type='clear' n=5000000 0.36 % ±1.93% ±2.57% ±3.35%
timers/immediate.js type='depth' n=5000000 -0.40 % ±0.69% ±0.91% ±1.19%
timers/immediate.js type='depth1' n=5000000 -0.46 % ±0.76% ±1.01% ±1.32%
timers/set-immediate-breadth-args.js n=5000000 3.25 % ±3.84% ±5.11% ±6.65%
timers/set-immediate-breadth.js n=10000000 1.62 % ±2.37% ±3.16% ±4.13%
timers/set-immediate-depth-args.js n=5000000 0.02 % ±0.44% ±0.59% ±0.77%

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2019-04-24T06:43:00Z

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3349

joyeecheung · 2019-04-24T07:12:29Z

 class TickObject {
 constructor(callback, args, triggerAsyncId) {
- // This must be set to null first to avoid function tracking
- // on the hidden class, revisit in V8 versions after 6.2


Are these related to https://bugs.chromium.org/p/v8/issues/detail?id=5456 ? @apapirovski
AFAIK constant field tracking is not yet shipped in V8 but the issue is closed so I assume the deopt storm issue has been alleviated by other patches in the upstream.

Yes but last I tried this, it was still an issue. We should create a benchmark that actually tests this if we want to be certain. (i recently tried this with timers and still saw perf penalty.)

joyeecheung · 2019-04-24T07:19:28Z

Benchmark CI for process/next-tick* https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/355/

nodejs-github-bot · 2019-04-24T15:11:05Z

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

starkwang · 2019-04-25T02:21:27Z

Benchmark CI for timers: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/357/

benjamingr · 2019-04-25T11:11:32Z

@bmeurer

Trott · 2019-04-26T08:16:30Z

Benchmark CI for timers: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/357/

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/359/ (scheduled)

Fishrock123 · 2019-04-26T23:45:11Z

This should not be backported. (I'm not quite sure what labels are necessary for that.)

BridgeAR · 2019-04-27T00:01:27Z

@Fishrock123 I just added the necessary labels. v12.x should be fine though.

ZYSzys · 2019-04-27T06:43:03Z

The benchmark showed some significant improvement in performance:
https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/359/console

01:14:48 confidence improvement accuracy (*) (**) (***)
01:14:48 timers/immediate.js type='breadth1' n=5000000 -2.15 % ±3.43% ±4.57% ±5.95%
01:14:48 timers/immediate.js type='breadth4' n=5000000 1.38 % ±4.12% ±5.48% ±7.14%
01:14:48 timers/immediate.js type='breadth' n=5000000 4.05 % ±4.08% ±5.44% ±7.09%
01:14:48 timers/immediate.js type='clear' n=5000000 *** 24.00 % ±4.58% ±6.12% ±8.01%
01:14:48 timers/immediate.js type='depth1' n=5000000 1.08 % ±1.11% ±1.48% ±1.92%
01:14:48 timers/immediate.js type='depth' n=5000000 0.32 % ±1.60% ±2.13% ±2.77%
01:14:48 timers/set-immediate-breadth-args.js n=5000000 0.34 % ±3.54% ±4.72% ±6.15%
01:14:48 timers/set-immediate-breadth.js n=10000000 -0.78 % ±6.73% ±8.95% ±11.65%
01:14:48 timers/set-immediate-depth-args.js n=5000000 0.42 % ±1.23% ±1.64% ±2.14%
01:14:48 timers/timers-breadth.js n=5000000 -1.77 % ±4.12% ±5.48% ±7.13%
01:14:48 timers/timers-cancel-pooled.js n=5000000 -2.73 % ±10.37% ±13.80% ±17.98%
01:14:48 timers/timers-cancel-unpooled.js direction='end' n=1000000 15.46 % ±25.61% ±34.10% ±44.42%
01:14:48 timers/timers-cancel-unpooled.js direction='start' n=1000000 6.28 % ±9.53% ±12.68% ±16.51%
01:14:48 timers/timers-depth.js n=1000 0.06 % ±0.09% ±0.12% ±0.16%
01:14:48 timers/timers-insert-pooled.js n=5000000 -0.43 % ±3.14% ±4.20% ±5.50%
01:14:48 timers/timers-insert-unpooled.js direction='end' n=1000000 *** 20.17 % ±5.71% ±7.64% ±10.02%
01:14:48 timers/timers-insert-unpooled.js direction='start' n=1000000 2.18 % ±4.79% ±6.37% ±8.29%
01:14:48 timers/timers-timeout-nexttick.js n=50000 3.27 % ±4.56% ±6.07% ±7.91%
01:14:48 timers/timers-timeout-nexttick.js n=5000000 -4.58 % ±5.64% ±7.56% ±9.98%
01:14:48 timers/timers-timeout-pooled.js n=10000000 2.30 % ±5.23% ±6.96% ±9.07%
01:14:48 timers/timers-timeout-unpooled.js n=1000000 0.39 % ±5.18% ±6.89% ±8.97%

Trott · 2019-04-27T23:19:34Z

The benchmark showed some significant improvement in performance:
https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/359/console

01:14:48 confidence improvement accuracy (*) (**) (***)
01:14:48 timers/immediate.js type='breadth1' n=5000000 -2.15 % ±3.43% ±4.57% ±5.95%
01:14:48 timers/immediate.js type='breadth4' n=5000000 1.38 % ±4.12% ±5.48% ±7.14%
01:14:48 timers/immediate.js type='breadth' n=5000000 4.05 % ±4.08% ±5.44% ±7.09%
01:14:48 timers/immediate.js type='clear' n=5000000 *** 24.00 % ±4.58% ±6.12% ±8.01%
01:14:48 timers/immediate.js type='depth1' n=5000000 1.08 % ±1.11% ±1.48% ±1.92%
01:14:48 timers/immediate.js type='depth' n=5000000 0.32 % ±1.60% ±2.13% ±2.77%
01:14:48 timers/set-immediate-breadth-args.js n=5000000 0.34 % ±3.54% ±4.72% ±6.15%
01:14:48 timers/set-immediate-breadth.js n=10000000 -0.78 % ±6.73% ±8.95% ±11.65%
01:14:48 timers/set-immediate-depth-args.js n=5000000 0.42 % ±1.23% ±1.64% ±2.14%
01:14:48 timers/timers-breadth.js n=5000000 -1.77 % ±4.12% ±5.48% ±7.13%
01:14:48 timers/timers-cancel-pooled.js n=5000000 -2.73 % ±10.37% ±13.80% ±17.98%
01:14:48 timers/timers-cancel-unpooled.js direction='end' n=1000000 15.46 % ±25.61% ±34.10% ±44.42%
01:14:48 timers/timers-cancel-unpooled.js direction='start' n=1000000 6.28 % ±9.53% ±12.68% ±16.51%
01:14:48 timers/timers-depth.js n=1000 0.06 % ±0.09% ±0.12% ±0.16%
01:14:48 timers/timers-insert-pooled.js n=5000000 -0.43 % ±3.14% ±4.20% ±5.50%
01:14:48 timers/timers-insert-unpooled.js direction='end' n=1000000 *** 20.17 % ±5.71% ±7.64% ±10.02%
01:14:48 timers/timers-insert-unpooled.js direction='start' n=1000000 2.18 % ±4.79% ±6.37% ±8.29%
01:14:48 timers/timers-timeout-nexttick.js n=50000 3.27 % ±4.56% ±6.07% ±7.91%
01:14:48 timers/timers-timeout-nexttick.js n=5000000 -4.58 % ±5.64% ±7.56% ±9.98%
01:14:48 timers/timers-timeout-pooled.js n=10000000 2.30 % ±5.23% ±6.96% ±9.07%
01:14:48 timers/timers-timeout-unpooled.js n=1000000 0.39 % ±5.18% ±6.89% ±8.97%

Those improvements are so large that they seem suspicious. Just to be sure, here's a benchmark comparing master against a PR that does nothing but move some lines in the README.md file. (If it shows a similar difference, then the benchmark results are not valid and there's a problem in the benchmark or the benchmark CI. I don't expect that to be case, but I've seen it once before, so just to be sure... https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/362/)

Trott · 2019-04-28T00:36:35Z

Yup. Alarmingly, the benchmark when run with basically the same binary produces erroneous results:

17:25:58 confidence improvement accuracy (*) (**) (***)17:25:58 timers/immediate.js type='breadth1' n=5000000 -0.18 % ±3.99% ±5.32% ±6.92%17:25:58 timers/immediate.js type='breadth4' n=5000000 0.93 % ±3.61% ±4.80% ±6.25%17:25:58 timers/immediate.js type='breadth' n=5000000 1.75 % ±3.76% ±5.01% ±6.53%17:25:58 timers/immediate.js type='clear' n=5000000 *** 21.68 % ±4.59% ±6.13% ±8.03%17:25:58 timers/immediate.js type='depth1' n=5000000 -0.65 % ±1.88% ±2.51% ±3.29%17:25:58 timers/immediate.js type='depth' n=5000000 0.87 % ±1.45% ±1.93% ±2.52%17:25:58 timers/set-immediate-breadth-args.js n=5000000 -0.53 % ±3.59% ±4.78% ±6.22%17:25:58 timers/set-immediate-breadth.js n=10000000 -3.25 % ±7.07% ±9.41% ±12.25%17:25:58 timers/set-immediate-depth-args.js n=5000000 -0.28 % ±1.31% ±1.75% ±2.28%17:25:58 timers/timers-breadth.js n=5000000 -0.78 % ±4.00% ±5.32% ±6.93%17:25:58 timers/timers-cancel-pooled.js n=5000000 -2.57 % ±9.54% ±12.69% ±16.52%17:25:58 timers/timers-cancel-unpooled.js direction='end' n=1000000 13.34 % ±26.15% ±34.86% ±45.51%17:25:58 timers/timers-cancel-unpooled.js direction='start' n=1000000 2.66 % ±8.58% ±11.42% ±14.88%17:25:58 timers/timers-depth.js n=1000 -0.04 % ±0.12% ±0.16% ±0.21%17:25:58 timers/timers-insert-pooled.js n=5000000 0.10 % ±2.70% ±3.60% ±4.70%17:25:58 timers/timers-insert-unpooled.js direction='end' n=1000000 *** 19.37 % ±4.58% ±6.10% ±7.97%17:25:58 timers/timers-insert-unpooled.js direction='start' n=1000000 1.52 % ±3.56% ±4.74% ±6.18%17:25:58 timers/timers-timeout-nexttick.js n=50000 3.89 % ±4.50% ±5.99% ±7.80%17:25:58 timers/timers-timeout-nexttick.js n=5000000 0.72 % ±1.48% ±1.97% ±2.56%17:25:58 timers/timers-timeout-pooled.js n=10000000 -1.61 % ±7.30% ±9.72% ±12.66%17:25:58 timers/timers-timeout-unpooled.js n=1000000 2.95 % ±4.86% ±6.51% ±8.53%

So, the benchmark should be run locally because I'm pretty sure this is an issue with the benchmark CI. This is now the second time I have seen this sort of thing. It's disconcerting.

starkwang · 2019-04-29T12:47:42Z

Landed in 8b04d5c

PR-URL: #27380 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>

lib: remove outdated optimizations
dfb8ca9

nodejs-github-bot added process Issues and PRs related to the process subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Apr 24, 2019

joyeecheung requested a review from apapirovski April 24, 2019 07:11

joyeecheung reviewed Apr 24, 2019
View reviewed changes

BridgeAR approved these changes Apr 24, 2019
View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 24, 2019

jasnell approved these changes Apr 26, 2019
View reviewed changes

BridgeAR added dont-land-on-v10.x labels Apr 27, 2019

ZYSzys approved these changes Apr 27, 2019
View reviewed changes

Trott mentioned this pull request Apr 28, 2019
Benchmarks in CI produce erroneous results nodejs/build#1793
Closed

bmeurer approved these changes Apr 28, 2019
View reviewed changes

starkwang closed this Apr 29, 2019

targos mentioned this pull request May 6, 2019
v12.2.0 proposal #27578
Merged

Uh oh!

Uh oh!

Conversation

starkwang commented Apr 24, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Apr 24, 2019

Uh oh!

joyeecheungApr 24, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovskiApr 24, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Apr 24, 2019

Uh oh!

nodejs-github-bot commented Apr 24, 2019

Uh oh!

starkwang commented Apr 25, 2019

Uh oh!

benjamingr commented Apr 25, 2019

Uh oh!

Trott commented Apr 26, 2019

Uh oh!

Fishrock123 commented Apr 26, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Apr 27, 2019

Uh oh!

ZYSzys commented Apr 27, 2019

Uh oh!

Trott commented Apr 27, 2019

Uh oh!

Trott commented Apr 28, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

starkwang commented Apr 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

starkwang commented Apr 24, 2019•
edited
Loading

joyeecheungApr 24, 2019•
edited
Loading

Fishrock123 commented Apr 26, 2019•
edited
Loading

Trott commented Apr 28, 2019•
edited
Loading