timers: simplify the compareTimersLists function

[gurgunday]

Simplifies the compare function by returning directly if expiryDiff is equal to 0

We don't need to compare twice here, or return 1 or -1 -- if the ids are equal, which only happens if the same object is passed, we can just return the result of the difference as expiryDiff is 0 too

Before:

util/priority-queue.js
util/priority-queue.js n=100000: 5,113,431.506786623

After:

util/priority-queue.js
util/priority-queue.js n=100000: 5,559,752.550981263

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.34%. Comparing base (e2bc395) to head (9239cba).
Report is 113 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57110      +/-   ##
==========================================
- Coverage   90.37%   90.34%   -0.03%     
==========================================
  Files         629      629              
  Lines      184365   184362       -3     
  Branches    36016    36021       +5     
==========================================
- Hits       166612   166570      -42     
- Misses      10918    10922       +4     
- Partials     6835     6870      +35     

Files with missing lines	Coverage Δ
lib/internal/timers.js	99.58% <100.00%> (-0.01%)	⬇️

... and 21 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nodejs-github-bot]

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

[gurgunday]

Can we get this merged?

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[gurgunday]

@atlowChemi can you take a look as well?

[nodejs-github-bot]

Landed in 7c2709d

[anonrig]

This PR landed without benchmark-ci even though there was a needs-benchmark-ci label. cc @atlowChemi

[atlowChemi]

@anonrig I totally, missed that 😕

I ran now a benchmark CI, should we add a don't land until the benchmarks have run?

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1676/

[atlowChemi]

                                                             confidence improvement accuracy (*)   (**)  (***)
timers/immediate.js type='breadth' n=5000000                                 0.09 %       ±1.06% ±1.41% ±1.84%
timers/immediate.js type='breadth1' n=5000000                                0.53 %       ±1.02% ±1.35% ±1.76%
timers/immediate.js type='breadth4' n=5000000                               -0.27 %       ±0.82% ±1.09% ±1.43%
timers/immediate.js type='clear' n=5000000                                  -0.18 %       ±1.58% ±2.11% ±2.74%
timers/immediate.js type='depth' n=5000000                                   0.02 %       ±0.75% ±1.00% ±1.31%
timers/immediate.js type='depth1' n=5000000                                  0.51 %       ±0.57% ±0.76% ±1.00%
timers/set-immediate-breadth-args.js n=5000000                               0.06 %       ±2.45% ±3.27% ±4.25%
timers/set-immediate-breadth.js n=10000000                                   0.01 %       ±1.22% ±1.62% ±2.11%
timers/set-immediate-depth-args.js n=5000000                                -0.51 %       ±0.67% ±0.90% ±1.17%
timers/timers-breadth-args.js n=1000000                                      0.18 %       ±1.54% ±2.05% ±2.67%
timers/timers-breadth.js n=5000000                                          -1.04 %       ±3.07% ±4.08% ±5.32%
timers/timers-cancel-pooled.js n=5000000                                     2.45 %       ±5.63% ±7.50% ±9.76%
timers/timers-cancel-unpooled.js direction='end' n=1000000                  -0.61 %       ±3.71% ±4.94% ±6.43%
timers/timers-cancel-unpooled.js direction='start' n=1000000                -0.98 %       ±2.86% ±3.81% ±4.95%
timers/timers-depth.js n=1000                                                0.12 %       ±0.15% ±0.20% ±0.26%
timers/timers-insert-pooled.js n=5000000                                     0.97 %       ±5.39% ±7.17% ±9.33%
timers/timers-insert-unpooled.js direction='end' n=1000000                   0.36 %       ±1.03% ±1.37% ±1.78%
timers/timers-insert-unpooled.js direction='start' n=1000000                -1.49 %       ±2.66% ±3.54% ±4.61%
timers/timers-timeout-nexttick.js n=50000                                    2.38 %       ±3.87% ±5.16% ±6.71%
timers/timers-timeout-nexttick.js n=5000000                                  1.29 %       ±4.75% ±6.31% ±8.22%
timers/timers-timeout-pooled.js n=10000000                                   0.22 %       ±0.60% ±0.80% ±1.04%
timers/timers-timeout-unpooled.js n=1000000                                  2.22 %       ±3.30% ±4.39% ±5.72%

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 (***)

[anonrig]

Code is a lot simpler, and I'm fine with landing this but the performance claim seems to be invalid.

[gurgunday]

Well, locally I saw a small improvement but in any case it was clear to me this wouldn't be a 200% improvement

I just wanted to simplify the function by returning directly here, because the result will get compared with 0 anyway in the priority queue

In terms of V8 and JS bytecode, this forces a Number result with a constant subtraction operation instead of doing 2 separate comparisons at worst case, I'd say it should perform just as well or slightly better in a certain scenario where the ids are the same