Skip to content

lib: avoid quadratic shift() in startup snapshot callback#62914

Closed
watilde wants to merge 1 commit intonodejs:mainfrom
watilde:startup-snapshot
Closed

lib: avoid quadratic shift() in startup snapshot callback#62914
watilde wants to merge 1 commit intonodejs:mainfrom
watilde:startup-snapshot

Conversation

@watilde
Copy link
Copy Markdown
Member

@watilde watilde commented Apr 23, 2026

Micro benchmark

$ cat bench-startup-snapshot-callbacks.js
'use strict';

function runWithShift(callbacks) {
  while (callbacks.length > 0) {
    const { 0: cb, 1: data } = callbacks.shift();
    cb(data);
  }
}

function runWithIndex(callbacks) {
  for (let i = 0; i < callbacks.length; i++) {
    const { 0: cb, 1: data } = callbacks[i];
    cb(data);
  }
  callbacks.length = 0;
}

function makeCallbacks(count) {
  return Array.from({ length: count }, (_, j) => [() => {}, j]);
}

function measure(fn, count, iterations) {
  // warmup
  for (let i = 0; i < 50; i++) fn(makeCallbacks(count));

  const start = performance.now();
  for (let i = 0; i < iterations; i++) fn(makeCallbacks(count));
  return performance.now() - start;
}

const COUNTS = [10, 100, 1_000, 10_000, 100_000];

console.log('--- shift() vs for+length=0 ---\n');

for (const count of COUNTS) {
  const iters = Math.max(10, Math.floor(1_000_000 / count));
  const shiftMs = measure(runWithShift, count, iters);
  const indexMs = measure(runWithIndex, count, iters);
  const ratio = shiftMs / indexMs;

  const countStr = `count=${String(count).padStart(6)}`;
  const shiftStr = `${shiftMs.toFixed(2).padStart(10)}ms`;
  const indexStr = `${indexMs.toFixed(2).padStart(8)}ms`;
  const ratioStr = `${ratio.toFixed(ratio >= 10 ? 0 : 1).padStart(5)}x faster`;

  console.log(`${countStr}  ${shiftStr}  vs  ${indexStr}   => ${ratioStr}`);
}

Result

$ ./node bench-startup-snapshot-callbacks.js
--- shift() vs for+length=0 ---

count=    10       58.22ms  vs     49.66ms   =>   1.2x faster
count=   100       42.62ms  vs     32.54ms   =>   1.3x faster
count=  1000       53.96ms  vs     30.92ms   =>   1.7x faster
count= 10000       60.50ms  vs     28.47ms   =>   2.1x faster
count=100000    46747.20ms  vs     50.64ms   =>   923x faster

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. v8 module Issues and PRs related to the "v8" subsystem. labels Apr 23, 2026
@watilde watilde requested a review from joyeecheung April 23, 2026 18:21
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.62%. Comparing base (d44a71a) to head (100d584).
⚠️ Report is 72 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #62914   +/-   ##
=======================================
  Coverage   89.61%   89.62%           
=======================================
  Files         706      706           
  Lines      219203   219207    +4     
  Branches    41995    41999    +4     
=======================================
+ Hits       196445   196455   +10     
+ Misses      14663    14656    -7     
- Partials     8095     8096    +1     
Files with missing lines Coverage Δ
lib/internal/v8/startup_snapshot.js 95.48% <100.00%> (+0.10%) ⬆️

... and 31 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.

@H4ad H4ad added the performance Issues and PRs related to the performance of Node.js. label Apr 27, 2026
@watilde watilde added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 27, 2026
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 27, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 27, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@nodejs-github-bot 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 Apr 27, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/62914
✔  Done loading data for nodejs/node/pull/62914
----------------------------------- PR info ------------------------------------
Title      lib: avoid quadratic shift() in startup snapshot callback (#62914)
Author     Daijiro Wachi <daijiro.wachi@gmail.com> (@watilde)
Branch     watilde:startup-snapshot -> nodejs:main
Labels     performance, needs-ci, v8 module
Commits    1
 - lib: avoid quadratic shift() in startup snapshot callback
Committers 1
 - Daijiro Wachi <daijiro.wachi@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/62914
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Ilyas Shabi <ilyasshabi94@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/62914
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Ilyas Shabi <ilyasshabi94@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 23 Apr 2026 17:49:26 GMT
   ✔  Approvals: 5
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/62914#pullrequestreview-4174486412
   ✔  - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/62914#pullrequestreview-4177919400
   ✔  - Ilyas Shabi (@IlyasShabi): https://github.com/nodejs/node/pull/62914#pullrequestreview-4178825311
   ✔  - Gürgün Dayıoğlu (@gurgunday): https://github.com/nodejs/node/pull/62914#pullrequestreview-4179212038
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/62914#pullrequestreview-4179356218
   ✘  GitHub CI failed with status: FAILURE
   ℹ  Last Full PR CI on 2026-04-27T09:04:48Z: https://ci.nodejs.org/job/node-test-pull-request/72949/
- Querying data for job/node-test-pull-request/72949/
✔  Build data downloaded
- Querying failures of job/node-test-commit/87332/
✔  Data downloaded
   ✘  1 failure(s) on the last Jenkins CI run
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/24997533196

watilde added a commit that referenced this pull request Apr 27, 2026
PR-URL: #62914
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Ilyas Shabi <ilyasshabi94@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@watilde
Copy link
Copy Markdown
Member Author

watilde commented Apr 27, 2026

Landed in b6b6e96

@watilde watilde closed this Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commit-queue-failed An error occurred while landing this pull request using GitHub Actions. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. v8 module Issues and PRs related to the "v8" subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants