lib: (timers) make arguments array PACKED in all cases #54771

gurgunday · 2024-09-04T17:18:00Z

When timers have less than 4 arguments, the args array is PACKED, but it's HOLEY when there are more than 4 arguments, I think it might result in performance inconsistencies

aduh95 · 2024-09-05T08:21:23Z

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

Results (no significant perf regression)

                                                             confidence improvement accuracy (*)   (**)   (***)
timers/immediate.js type='breadth' n=5000000                                 0.53 %       ±3.50% ±4.66%  ±6.06%
timers/immediate.js type='breadth1' n=5000000                         *      4.23 %       ±4.22% ±5.61%  ±7.31%
timers/immediate.js type='breadth4' n=5000000                                0.23 %       ±2.12% ±2.82%  ±3.67%
timers/immediate.js type='clear' n=5000000                                   0.01 %       ±0.81% ±1.08%  ±1.41%
timers/immediate.js type='depth' n=5000000                                   0.18 %       ±0.31% ±0.42%  ±0.54%
timers/immediate.js type='depth1' n=5000000                                  0.10 %       ±0.42% ±0.57%  ±0.74%
timers/set-immediate-breadth-args.js n=5000000                               0.45 %       ±2.56% ±3.41%  ±4.44%
timers/set-immediate-breadth.js n=10000000                                  -0.61 %       ±0.84% ±1.12%  ±1.46%
timers/set-immediate-depth-args.js n=5000000                                 0.26 %       ±0.55% ±0.73%  ±0.95%
timers/timers-breadth-args.js n=1000000                                     -0.18 %       ±2.03% ±2.70%  ±3.52%
timers/timers-breadth.js n=5000000                                           0.96 %       ±3.25% ±4.32%  ±5.64%
timers/timers-cancel-pooled.js n=5000000                                    -4.75 %       ±6.06% ±8.06% ±10.50%
timers/timers-cancel-unpooled.js direction='end' n=1000000                  -0.17 %       ±0.78% ±1.04%  ±1.35%
timers/timers-cancel-unpooled.js direction='start' n=1000000                -0.15 %       ±1.42% ±1.90%  ±2.47%
timers/timers-depth.js n=1000                                               -0.04 %       ±0.13% ±0.17%  ±0.22%
timers/timers-insert-pooled.js n=5000000                                    -2.90 %       ±4.57% ±6.09%  ±7.94%
timers/timers-insert-unpooled.js direction='end' n=1000000                   0.52 %       ±0.92% ±1.22%  ±1.59%
timers/timers-insert-unpooled.js direction='start' n=1000000                -0.61 %       ±1.24% ±1.65%  ±2.14%
timers/timers-timeout-nexttick.js n=50000                                   -0.61 %       ±1.60% ±2.13%  ±2.77%
timers/timers-timeout-nexttick.js n=5000000                                  0.07 %       ±2.64% ±3.52%  ±4.60%
timers/timers-timeout-pooled.js n=10000000                            *      1.07 %       ±0.89% ±1.18%  ±1.54%
timers/timers-timeout-unpooled.js n=1000000                                 -0.60 %       ±1.90% ±2.53%  ±3.30%

aduh95 · 2024-09-05T20:05:14Z

Can you please amend the commit message so it complies with our guidelines? E.g. timers: avoid generating holey internal arrays

codecov · 2024-09-05T21:29:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.60%. Comparing base (65a4d26) to head (d787801).
Report is 78 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54771      +/-   ##
==========================================
+ Coverage   87.33%   87.60%   +0.26%     
==========================================
  Files         650      650              
  Lines      182829   182998     +169     
  Branches    35060    35407     +347     
==========================================
+ Hits       159677   160314     +637     
+ Misses      16417    15934     -483     
- Partials     6735     6750      +15

Files with missing lines	Coverage Δ
lib/timers.js	`100.00% <100.00%> (ø)`

... and 69 files with indirect coverage changes

RedYetiDev · 2024-09-06T13:02:01Z

lib/timers.js

@@ -154,7 +155,7 @@ function setTimeout(callback, after, arg1, arg2, arg3) {
      args = [arg1, arg2, arg3];
      for (i = 5; i < arguments.length; i++) {
        // Extend array dynamically, makes .apply run much faster in v6.0.0
-        args[i - 2] = arguments[i];
+        ArrayPrototypePush(args, arguments[i]);


Suggested change

ArrayPrototypePush(args, arguments[i]);

args[args.length] = arguments[i];

Out of curiosity, is this faster?

I suspect they will all be similar

The goal here is to minimize HOLEY internal arrays and not press on objects too much, it might put stress on other parts of V8

gurgunday · 2024-09-06T17:24:01Z

PTAL @aduh95 :)

nodejs-github-bot · 2024-09-06T18:22:01Z

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

gurgunday · 2024-09-06T20:14:22Z

@aduh95 sorry, completely forgot about the beloved setImmediate, just added

nodejs-github-bot · 2024-09-07T09:45:58Z

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

jasnell · 2024-09-08T03:43:31Z

PR is currently blocked from landing due to unreliable CI

nodejs-github-bot · 2024-09-08T09:10:12Z

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

nodejs-github-bot · 2024-09-09T23:02:21Z

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

gurgunday · 2024-09-10T05:27:59Z

PR is currently blocked from landing due to unreliable CI

It looks good now @jasnell 🚀

nodejs-github-bot · 2024-09-10T09:26:24Z

Commit Queue failed

- Loading data for nodejs/node/pull/54771
✔  Done loading data for nodejs/node/pull/54771
----------------------------------- PR info ------------------------------------
Title      lib: (timers) make arguments array PACKED in all cases (#54771)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     gurgunday:push -> nodejs:main
Labels     timers, author ready, needs-ci, needs-benchmark-ci
Commits    1
 - timers: avoid generating holey internal arrays
Committers 1
 - Gürgün Dayıoğlu <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54771
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54771
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - timers: avoid generating holey internal arrays
   ℹ  This PR was created on Wed, 04 Sep 2024 17:18:00 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54771#pullrequestreview-2283550665
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/54771#pullrequestreview-2286915239
   ✔  Last GitHub CI successful
   ℹ  Last Benchmark CI on 2024-09-05T08:21:23Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1631/
   ℹ  Last Full PR CI on 2024-09-09T23:02:21Z: https://ci.nodejs.org/job/node-test-pull-request/62212/
- Querying data for job/node-test-pull-request/62212/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/10789630142

gurgunday · 2024-09-10T11:40:21Z

Not sure what the issue is, @aduh95 😕

⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!

My email address is in the commit

⚠ Commits were pushed since the last approving review:

Maybe it requires an up-to-date approval before running the commit-queue

RedYetiDev · 2024-09-10T11:45:31Z

⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!

My email address is in the commit

No need to worry about that, it's just a warning

⚠ Commits were pushed since the last approving review:

Maybe it requires an up-to-date approval before running the commit-queue

Exactly. Once someone reviews (and approves), they can re-run the commit-queue for this to land.

nodejs-github-bot · 2024-09-10T12:48:06Z

Landed in 26b03c1

PR-URL: #54771 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

nodejs-github-bot added needs-ci PRs that need a full CI run. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Sep 4, 2024

RedYetiDev added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 4, 2024

gurgunday force-pushed the push branch from 8c8676b to a80fabd Compare September 4, 2024 19:16

jasnell approved these changes Sep 5, 2024

View reviewed changes

RedYetiDev reviewed Sep 6, 2024

View reviewed changes

gurgunday force-pushed the push branch from a80fabd to d07b433 Compare September 6, 2024 17:21

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 6, 2024

aduh95 approved these changes Sep 6, 2024

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2024

timers: avoid generating holey internal arrays

d787801

gurgunday force-pushed the push branch from d07b433 to d787801 Compare September 6, 2024 20:12

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2024

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 10, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 10, 2024

aduh95 approved these changes Sep 10, 2024

View reviewed changes

aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Sep 10, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 10, 2024

nodejs-github-bot merged commit 26b03c1 into nodejs:main Sep 10, 2024
62 checks passed

gurgunday deleted the push branch September 10, 2024 14:01

aduh95 pushed a commit that referenced this pull request Sep 12, 2024

timers: avoid generating holey internal arrays

f535962

PR-URL: #54771 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

aduh95 pushed a commit that referenced this pull request Sep 13, 2024

timers: avoid generating holey internal arrays

b110445

PR-URL: #54771 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

aduh95 pushed a commit that referenced this pull request Sep 13, 2024

timers: avoid generating holey internal arrays

3d47872

PR-URL: #54771 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

RafaelGSS mentioned this pull request Sep 16, 2024

v22.9.0 proposal #54966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: (timers) make arguments array PACKED in all cases #54771

lib: (timers) make arguments array PACKED in all cases #54771

gurgunday commented Sep 4, 2024 •

edited

Loading

aduh95 commented Sep 5, 2024 •

edited

Loading

aduh95 commented Sep 5, 2024

codecov bot commented Sep 5, 2024 •

edited

Loading

RedYetiDev Sep 6, 2024 •

edited

Loading

gurgunday Sep 6, 2024 •

edited

Loading

gurgunday commented Sep 6, 2024

nodejs-github-bot commented Sep 6, 2024

gurgunday commented Sep 6, 2024

nodejs-github-bot commented Sep 7, 2024

jasnell commented Sep 8, 2024

nodejs-github-bot commented Sep 8, 2024

nodejs-github-bot commented Sep 9, 2024

gurgunday commented Sep 10, 2024

nodejs-github-bot commented Sep 10, 2024

gurgunday commented Sep 10, 2024

RedYetiDev commented Sep 10, 2024

nodejs-github-bot commented Sep 10, 2024

	ArrayPrototypePush(args, arguments[i]);
	args[args.length] = arguments[i];

lib: (timers) make arguments array PACKED in all cases #54771

lib: (timers) make arguments array PACKED in all cases #54771

Conversation

gurgunday commented Sep 4, 2024 • edited Loading

aduh95 commented Sep 5, 2024 • edited Loading

aduh95 commented Sep 5, 2024

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

RedYetiDev Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

gurgunday Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

gurgunday commented Sep 6, 2024

nodejs-github-bot commented Sep 6, 2024

gurgunday commented Sep 6, 2024

nodejs-github-bot commented Sep 7, 2024

jasnell commented Sep 8, 2024

nodejs-github-bot commented Sep 8, 2024

nodejs-github-bot commented Sep 9, 2024

gurgunday commented Sep 10, 2024

nodejs-github-bot commented Sep 10, 2024

gurgunday commented Sep 10, 2024

RedYetiDev commented Sep 10, 2024

nodejs-github-bot commented Sep 10, 2024

gurgunday commented Sep 4, 2024 •

edited

Loading

aduh95 commented Sep 5, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading

RedYetiDev Sep 6, 2024 •

edited

Loading

gurgunday Sep 6, 2024 •

edited

Loading