buffer: optimize buffer.concat performance

[mertcanaltin]

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) ✗ 

[codecov]

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]

See #60399 (comment)

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

[gurgunday]

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]

Can we even hit this part? I think this is unreachable because you moved type validation to the beginning and it will always fill after going through all of them?

[mertcanaltin]

It's still reachable if a buffer gets detached between the validation and copy loops buf.length becomes 0, pos won't advance, leaving uninitialized bytes without the zero-fill.

[gurgunday]

Not really sure how it can be detached unless a length getter is compromised. It's all synchronous code here?

In any case, it would be nice to have coverage for it

Other than coverage, the PR LGTM

[Renegade334]

How about an assert here?

(Attempting to read from a detached buffer should throw an error from the engine in any case.)

[jasnell]

If the buffer gets detached between these points should actually cause the Uint8Array.prototype.set operation to fail.

Consider the example:

const u8_1 = new Uint8Array([1,2,3,4]);
const u8_2 = new Uint8Array([5,6,7,8]);

let called = false;

Object.defineProperty(u8_1, 'length', {
  get() {
    // The first time this is called, return the actual length of the array
    // The second time this is called, we'll also transfer the ArrayBuffer of the second

    if (!called) {
      called = true;
    } else {
      u8_2.buffer.transfer();
    }
    return 4;
  },
});

const buf = Buffer.concat([u8_1, u8_2]);
console.log(buf);

node:buffer:631
      TypedArrayPrototypeSet(buffer, buf, pos);
      ^

TypeError: Cannot perform %TypedArray%.prototype.set on a detached ArrayBuffer
    at Buffer.set (<anonymous>)
    at Buffer.concat (node:buffer:631:7)
    at Object.<anonymous> (/home/jsnell/tmp/fubar.js:20:20)
    at Module._compile (node:internal/modules/cjs/loader:1811:14)
    at Object..js (node:internal/modules/cjs/loader:1942:10)
    at Module.load (node:internal/modules/cjs/loader:1532:32)
    at Module._load (node:internal/modules/cjs/loader:1334:12)
    at wrapModuleLoad (node:internal/modules/cjs/loader:255:19)
    at Module.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:154:5)
    at node:internal/main/run_main_module:33:47

Node.js v26.0.0-pre

I'm all for being defensive here tho.

[Renegade334]

TypedArrayPrototypeGetLength would be the tonic there, presumably.

[jasnell]

Probably. Also just make sure that Uint8Array.prototype.set itself does not call the user overide getter (it shouldn't.. but let's confirm)

[mertcanaltin]

Thanks, switched to TypedArrayPrototypeGetByteLength to avoid the spoofed length getter. Zero-fill kept as defensive measure.

[jasnell]

I would be sure to also add a test that covers the spoofed length test.

[mertcanaltin]

I added now, thanks

[mertcanaltin]

I updated bench result: #61721 (comment)

[Renegade334]

Nit: my preference would still be for an assert here, since pos !== length is de facto an invalid state.

[mertcanaltin]

I'd prefer keeping the zero-fill here if we're ever wrong about it being unreachable, zero-filling is safer than crashing the process with uninitialized memory in the buffer.

[gurgunday]

lgtm!