Skip to content

buffer: optimize buffer.concat performance #61721

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.

Sign up for GitHub

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

Jump to bottom
Open
mertcanaltin wants to merge 7 commits into
base: main
Choose a base branch
from
Open

buffer: optimize buffer.concat performance #61721

mertcanaltin wants to merge 7 commits into from

Conversation

@mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented Feb 7, 2026 •
edited
Loading

Removed the _copyActual indirection in the copy loop and called TypedArrayPrototypeSet directly.
Split auto-length and explicit-length paths so the auto-length copy loop is branch free. Replaced Buffer.allocUnsafe with allocate to skip redundant validation.

benchmark results:

➜  node git:(mert/buffer-concat-optimize-js) ✗ node-benchmark-compare ./result.csv
                                                                            confidence improvement accuracy (*)    (**)   (***)
buffers/buffer-concat-fill.js n=800000 extraSize=1                                   *     10.37 %       ±9.47% ±13.37% ±19.13%
buffers/buffer-concat-fill.js n=800000 extraSize=1024                                       2.91 %       ±6.22%  ±8.53% ±11.62%
buffers/buffer-concat-fill.js n=800000 extraSize=256                                 *      7.05 %       ±6.13%  ±8.41% ±11.49%
buffers/buffer-concat.js n=800000 withTotalLength=0 pieceSize=1 pieces=16           **      1.76 %       ±1.18%  ±1.64%  ±2.26%
buffers/buffer-concat.js n=800000 withTotalLength=0 pieceSize=1 pieces=4           ***      5.20 %       ±1.21%  ±1.65%  ±2.25%
buffers/buffer-concat.js n=800000 withTotalLength=0 pieceSize=16 pieces=16          **      1.96 %       ±1.17%  ±1.61%  ±2.21%
buffers/buffer-concat.js n=800000 withTotalLength=0 pieceSize=16 pieces=4          ***      3.06 %       ±1.50%  ±2.06%  ±2.84%
buffers/buffer-concat.js n=800000 withTotalLength=0 pieceSize=256 pieces=16                 1.93 %       ±8.35% ±11.48% ±15.73%
buffers/buffer-concat.js n=800000 withTotalLength=0 pieceSize=256 pieces=4                  2.37 %       ±6.69%  ±9.34% ±13.12%
buffers/buffer-concat.js n=800000 withTotalLength=1 pieceSize=1 pieces=16                  -0.59 %       ±0.99%  ±1.36%  ±1.86%
buffers/buffer-concat.js n=800000 withTotalLength=1 pieceSize=1 pieces=4           ***      5.17 %       ±1.25%  ±1.71%  ±2.33%
buffers/buffer-concat.js n=800000 withTotalLength=1 pieceSize=16 pieces=16                  0.98 %       ±1.45%  ±2.02%  ±2.81%
buffers/buffer-concat.js n=800000 withTotalLength=1 pieceSize=16 pieces=4          ***      4.44 %       ±1.89%  ±2.62%  ±3.62%
buffers/buffer-concat.js n=800000 withTotalLength=1 pieceSize=256 pieces=16          *     -1.97 %       ±1.89%  ±2.63%  ±3.67%
buffers/buffer-concat.js n=800000 withTotalLength=1 pieceSize=256 pieces=4         ***      3.55 %       ±1.08%  ±1.49%  ±2.04%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 15 comparisons, you can thus expect the following amount of false-positive results:
  0.75 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.15 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)
➜  node git:(mert/buffer-concat-optimize-js) ✗

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Feb 7, 2026
@codecov
Copy link

codecov bot commented Feb 7, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 90.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.74%. Comparing base (3ab9dd8) to head (4848305).
⚠️ Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
lib/buffer.js 90.00% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #61721      +/-   ##
==========================================
+ Coverage   89.73%   89.74%   +0.01%     
==========================================
  Files         675      675              
  Lines      204502   204673     +171     
  Branches    39304    39329      +25     
==========================================
+ Hits       183502   183684     +182     
+ Misses      13283    13281       -2     
+ Partials     7717     7708       -9
Files with missing lines Coverage Δ
lib/buffer.js 99.16% <90.00%> (-0.63%) ⬇️

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

ChALkeR
ChALkeR reviewed Feb 7, 2026
View reviewed changes
Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

See #60399 (comment)

Shouldn't _copyActual be just using TypedArrayPrototypeSet since #60399?
cc @gurgunday

ChALkeR
ChALkeR reviewed Feb 7, 2026
View reviewed changes
benjamingr
benjamingr reviewed Feb 7, 2026
View reviewed changes
@gurgunday gurgunday added the needs-benchmark-ci PR that need a benchmark CI run. label Feb 7, 2026
gurgunday
gurgunday reviewed Feb 7, 2026
View reviewed changes
Copy link
Member

@gurgunday gurgunday left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the ping @ChALkeR

Yeah so other than inlining what we did in _copyActual (#60399), this PR avoids going through _copy even in the non sourceStart === 0 && nb === sourceLen (partial copy) case (I remember that we'd talked about doing this in a follow-up PR anyway)

I like that change

gurgunday
gurgunday reviewed Feb 7, 2026
View reviewed changes
@mertcanaltin
Copy link
Member Author

mertcanaltin commented Feb 7, 2026

I updated bench result: #61721 (comment)

Renegade334
Renegade334 reviewed Feb 7, 2026
View reviewed changes
Renegade334
Renegade334 approved these changes Feb 10, 2026
View reviewed changes
gurgunday
gurgunday approved these changes Feb 10, 2026
View reviewed changes
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChALkeR ChALkeR ChALkeR left review comments

@jasnell jasnell jasnell left review comments

@benjamingr benjamingr benjamingr left review comments

@Renegade334 Renegade334 Renegade334 approved these changes

@gurgunday gurgunday gurgunday approved these changes

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mertcanaltin @ChALkeR @jasnell @benjamingr @Renegade334 @gurgunday @nodejs-github-bot