Skip to content

Conversation

@mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented Feb 7, 2026

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

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.

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

@gurgunday gurgunday added the needs-benchmark-ci PR that need a benchmark CI run. label Feb 7, 2026
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.

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

@mertcanaltin
Copy link
Member Author

I updated bench result: #61721 (comment)

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

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

Development

Successfully merging this pull request may close these issues.

7 participants