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.

Sign up for GitHub

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

Jump to bottom

lib: remove outdated optimizations #27380

Closed
wants to merge 1 commit into from
Closed

lib: remove outdated optimizations #27380

wants to merge 1 commit into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Apr 24, 2019 •
edited
Loading

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
Copy link
Collaborator

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

@nodejs-github-bot 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 joyeecheung requested a review from apapirovski April 24, 2019 07:11
joyeecheung
joyeecheung reviewed Apr 24, 2019
View reviewed changes
lib/internal/process/task_queues.js
@@ -92,9 +92,6 @@ function processTicksAndRejections() {

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
Copy link
Member

@joyeecheung joyeecheung Apr 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@BridgeAR 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
@nodejs-github-bot
Copy link
Collaborator

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

@starkwang
Copy link
Contributor Author

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

@benjamingr
Copy link
Member

@bmeurer

@Trott
Copy link
Member

Trott commented Apr 26, 2019

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
Copy link
Contributor

Fishrock123 commented Apr 26, 2019 •
edited
Loading

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

@BridgeAR
Copy link
Member

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

@ZYSzys
Copy link
Member

ZYSzys commented Apr 27, 2019

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
Copy link
Member

Trott commented Apr 27, 2019

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
Copy link
Member

Trott commented Apr 28, 2019 •
edited
Loading

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.

@Trott Trott mentioned this pull request Apr 28, 2019
Closed
@starkwang
Copy link
Contributor Author

Landed in 8b04d5c

@starkwang starkwang closed this Apr 29, 2019
starkwang added a commit that referenced this pull request Apr 29, 2019
@starkwang
lib: remove outdated optimizations
8b04d5c 
PR-URL: #27380
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
targos pushed a commit that referenced this pull request Apr 29, 2019
@starkwang @targos
lib: remove outdated optimizations
47d311b 
PR-URL: #27380
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
@targos targos mentioned this pull request May 6, 2019
Merged
This was referenced May 7, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@ZYSzys ZYSzys ZYSzys approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@bmeurer bmeurer bmeurer approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski Awaiting requested review from apapirovski

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. process Issues and PRs related to the process subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.