-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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.
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
timers: fix unsafe array iteration #37223
Conversation
Benchmark CI for timers: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/930/ Benchmark results
|
New Benchmark CI (using Benchmark results
|
Benchmark results are a bit weird, I kicked off a new benchmark CI to see if it's just a flakiness. |
I think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Benchmark CI results look good 🎉
confidence improvement accuracy (*) (**) (***)
timers/immediate.jstype='breadth1' n=5000000 1.00 % ±4.06% ±5.41% ±7.04%
timers/immediate.jstype='breadth4' n=5000000 -1.00 % ±4.15% ±5.53% ±7.21%
timers/immediate.jstype='breadth' n=5000000 3.12 % ±4.05% ±5.38% ±7.01%
timers/immediate.jstype='clear' n=5000000 -0.04 % ±2.15% ±2.86% ±3.72%
timers/immediate.jstype='depth1' n=5000000 -0.03 % ±1.74% ±2.31% ±3.02%
timers/immediate.jstype='depth' n=5000000 0.75 % ±2.22% ±2.95% ±3.85%
timers/set-immediate-breadth-args.jsn=5000000 0.19 % ±4.62% ±6.16% ±8.03%
timers/set-immediate-breadth.jsn=10000000 * 4.00 % ±3.89% ±5.18% ±6.74%
timers/set-immediate-depth-args.jsn=5000000 * 2.03 % ±1.80% ±2.40% ±3.14%
timers/timers-breadth-args.jsn=1000000 -5.37 % ±5.51% ±7.33% ±9.54%
timers/timers-breadth.jsn=5000000 0.81 % ±3.38% ±4.50% ±5.86%
timers/timers-cancel-pooled.jsn=5000000 1.91 % ±10.69% ±14.22% ±18.51%
timers/timers-cancel-unpooled.jsdirection='end' n=1000000 -0.92 % ±5.95% ±7.91% ±10.30%
timers/timers-cancel-unpooled.jsdirection='start' n=1000000 0.18 % ±6.67% ±8.89% ±11.58%
timers/timers-depth.jsn=1000 -0.13 % ±0.65% ±0.87% ±1.14%
timers/timers-insert-pooled.jsn=5000000 -0.54 % ±2.80% ±3.74% ±4.88%
timers/timers-insert-unpooled.jsdirection='end' n=1000000 1.44 % ±4.47% ±5.95% ±7.74%
timers/timers-insert-unpooled.jsdirection='start' n=1000000 0.55 % ±5.13% ±6.82% ±8.88%
timers/timers-timeout-nexttick.jsn=50000 0.14 % ±2.20% ±2.93% ±3.83%
timers/timers-timeout-nexttick.jsn=5000000 0.13 % ±3.60% ±4.80% ±6.25%
timers/timers-timeout-pooled.jsn=10000000 * 8.82 % ±8.47% ±11.29% ±14.72%
timers/timers-timeout-unpooled.jsn=1000000 0.62 % ±13.21% ±17.58% ±22.88%
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 (***)
Ping @nodejs/timers |
Fixes: nodejs#37222 PR-URL: nodejs#37223 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
89f304f
to
c0e66e3
Compare
Landed in c0e66e3 |
Fixes: #37222