lib: refactor PriorityQueue to use private field

[F3n67u]

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]

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

[aduh95]

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]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[RaisinTen]

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

[F3n67u]

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]

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]

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]

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]

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

[aduh95]

@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]

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]

I have learnt something new. Thank you

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in df7c49c