Skip to content

lib: refactor PriorityQueue to use private field - #43889

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
F3n67u:refactor/private-field
Jul 23, 2022
Merged

nodejs-github-bot merged 2 commits into
nodejs:mainfrom
F3n67u:refactor/private-field

Conversation

@F3n67u

@F3n67u F3n67u commented Jul 18, 2022

Copy link
Copy Markdown
Contributor

Today I am reading the code of PriorityQueue, I found use symbol to achieve private fields is not easy to read. so I refactor to use private field to improve readability.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 18, 2022
@F3n67u F3n67u added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 18, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 18, 2022
@nodejs-github-bot

ghost commented Jul 18, 2022

Copy link
Copy Markdown
Collaborator

Comment thread lib/internal/priority_queue.js Resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@F3n67u F3n67u added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Jul 18, 2022
@aduh95

ghost commented Jul 18, 2022 •

Copy link
Copy Markdown
Contributor

Bnechmark CI(timers): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1167/
Benchmark CI(util/priority-queue): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1168/ (queued, will 404 until it starts)

Results (no significant difference)
                                                             confidence improvement accuracy (*)    (**)   (***)
timers/immediate.js type='breadth1' n=5000000                               -1.35 %       ±3.56%  ±4.73%  ±6.17%
timers/immediate.js type='breadth4' n=5000000                                4.02 %       ±4.31%  ±5.73%  ±7.47%
timers/immediate.js type='breadth' n=5000000                                 0.42 %       ±5.39%  ±7.18%  ±9.37%
timers/immediate.js type='clear' n=5000000                                   2.11 %       ±2.44%  ±3.25%  ±4.24%
timers/immediate.js type='depth1' n=5000000                                 -0.42 %       ±1.17%  ±1.56%  ±2.03%
timers/immediate.js type='depth' n=5000000                                   0.39 %       ±1.42%  ±1.89%  ±2.47%
timers/set-immediate-breadth-args.js n=5000000                              -1.76 %       ±3.17%  ±4.22%  ±5.49%
timers/set-immediate-breadth.js n=10000000                                  -0.44 %       ±2.97%  ±3.95%  ±5.14%
timers/set-immediate-depth-args.js n=5000000                                 0.74 %       ±1.58%  ±2.10%  ±2.73%
timers/timers-breadth-args.js n=1000000                                      1.91 %       ±2.86%  ±3.81%  ±4.96%
timers/timers-breadth.js n=5000000                                           1.35 %       ±5.18%  ±6.91%  ±9.03%
timers/timers-cancel-pooled.js n=5000000                                     4.68 %      ±10.73% ±14.27% ±18.58%
timers/timers-cancel-unpooled.js direction='end' n=1000000                   1.86 %      ±17.91% ±23.83% ±31.02%
timers/timers-cancel-unpooled.js direction='start' n=1000000                -1.81 %       ±8.76% ±11.67% ±15.19%
timers/timers-depth.js n=1000                                               -0.04 %       ±0.32%  ±0.42%  ±0.55%
timers/timers-insert-pooled.js n=5000000                                     0.17 %       ±4.17%  ±5.55%  ±7.22%
timers/timers-insert-unpooled.js direction='end' n=1000000                   2.70 %       ±4.96%  ±6.60%  ±8.59%
timers/timers-insert-unpooled.js direction='start' n=1000000                -0.65 %       ±3.91%  ±5.20%  ±6.79%
timers/timers-timeout-nexttick.js n=50000                                    0.20 %       ±3.12%  ±4.15%  ±5.41%
timers/timers-timeout-nexttick.js n=5000000                                  5.75 %       ±6.79%  ±9.04% ±11.77%
timers/timers-timeout-pooled.js n=10000000                                   9.33 %      ±10.21% ±13.59% ±17.70%
timers/timers-timeout-unpooled.js n=1000000                                 -1.45 %       ±8.84% ±11.77% ±15.33%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 22 comparisons, you can thus
expect the following amount of false-positive results:
  1.10 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.22 false positives, when considering a   1% risk acceptance (**, ***),
  0.02 false positives, when considering a 0.1% risk acceptance (***)
                                confidence improvement accuracy (*)   (**)  (***)
util/priority-queue.js n=100000                -2.50 %       ±4.41% ±5.87% ±7.64%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 1 comparisons, you can thus
expect the following amount of false-positive results:
  0.05 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.01 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

@nodejs-github-bot

ghost commented Jul 18, 2022

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

ghost commented Jul 18, 2022

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

ghost commented Jul 18, 2022

Copy link
Copy Markdown
Collaborator

@F3n67u F3n67u added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jul 19, 2022

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. How far can we backport this change without introducing perf regressions?

@F3n67u

ghost commented Jul 19, 2022

Copy link
Copy Markdown
Contributor Author

LGTM. How far can we backport this change without introducing perf regressions?

@RaisinTen I see this pr introduces perf regression, but I don't understand what "How far can we backport this change without introducing perf regressions?" mean. Could you elaborate on this sentence?

@aduh95

ghost commented Jul 19, 2022

Copy link
Copy Markdown
Contributor

LGTM. How far can we backport this change without introducing perf regressions?

@RaisinTen I see this pr introduces perf regression, but I don't understand what "How far can we backport this change without introducing perf regressions?" mean. Could you elaborate on this sentence?

Having private fields in your source code has been for a long time a performance bottleneck because V8 wasn't able to optimize the class as efficiently as when using Symbol. However, looking at the benchmark results, it looks like it's no longer the case (I'm not sure why you say this PR introduces perf regression, AFAICT it doesn't, all the figures are below the false positive threshold). I suggest we don't backport this change to LTS to be safe.

@F3n67u

ghost commented Jul 19, 2022

Copy link
Copy Markdown
Contributor Author

confidence improvement accuracy () () ()
util/priority-queue.js n=100000 -2.50 % ±4.41% ±5.87% ±7.64%

I see the benchmark compare result shows improvement is -2.50%, this means that using private field cause performance decrease right?

@targos

ghost commented Jul 19, 2022

Copy link
Copy Markdown
Member

confidence improvement accuracy () () ()
util/priority-queue.js n=100000 -2.50 % ±4.41% ±5.87% ±7.64%

I see the benchmark compare result shows improvement is -2.50%, this means that using private field cause performance decrease right?

No, because the uncertainty of the value (±4.41%) is larger than the value itself.

@F3n67u

ghost commented Jul 19, 2022

Copy link
Copy Markdown
Contributor Author

@targos Could you show me an example that does introduce perf regression? I am not familiar with node benchmark comparison.

@aduh95

ghost commented Jul 19, 2022 •

Copy link
Copy Markdown
Contributor

@targos Could you show me an example that does introduce perf regression? I am not familiar with node benchmark comparison.

Here's an example: #41745 (comment)

TL;DR: you can disregard all the lines in the benchmark result that have no stars (*, **, or ***) in the confidence column.

The gist of it is that the difference of performance in the benchmarks is better explain by random flukes when running the CI than the actual change itself. If we were to re-run the CI, most likely we won't get -2.50%, we might get a positive figure, that wouldn't mean that this change improves performance either. ±4.41% means that we can assume that the actual results is somewhere between -2.50%-4.41% and -2.50%+4.41% and we would be wrong 5% of the times. I personally consider that a benchmark shows improvement or regression only when 0% is not in the range of the 1% risk acceptance (so when there is ** or *** on the line of the result).

@RaisinTen

ghost commented Jul 19, 2022 •

Copy link
Copy Markdown
Member

I meant that this PR doesn't introduce any perf regressions but there is a chance that it might do so if we backport it to an earlier version of node where V8 doesn't apply the required optimizations. Not backporting to LTS sounds okay too.

@F3n67u

ghost commented Jul 19, 2022

Copy link
Copy Markdown
Contributor Author

I have learnt something new. Thank you

@nodejs-github-bot

ghost commented Jul 23, 2022

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

ghost commented Jul 23, 2022

Copy link
Copy Markdown
Collaborator

@aduh95 aduh95 added the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2022
@nodejs-github-bot
nodejs-github-bot merged commit df7c49c into nodejs:main Jul 23, 2022
@nodejs-github-bot

ghost commented Jul 23, 2022

Copy link
Copy Markdown
Collaborator

Landed in df7c49c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants