Skip to content
2 changes: 1 addition & 1 deletion benchmark/buffers/buffer-bytelength-string.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const common = require('../common');
const bench = common.createBenchmark(main, {
type: ['one_byte', 'two_bytes', 'three_bytes',
'four_bytes', 'latin1'],
encoding: ['utf8', 'base64'],
encoding: ['utf8', 'base64', 'latin1', 'hex'],
repeat: [1, 2, 16, 256], // x16
n: [4e6],
});
Expand Down
31 changes: 31 additions & 0 deletions benchmark/buffers/buffer-copy-bytes-from.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';

const common = require('../common.js');

const bench = common.createBenchmark(main, {
type: ['Uint8Array', 'Uint16Array', 'Uint32Array', 'Float64Array'],
len: [64, 256, 2048],
partial: ['none', 'offset', 'offset-length'],
n: [6e5],
});

function main({ n, len, type, partial }) {
const TypedArrayCtor = globalThis[type];
const src = new TypedArrayCtor(len);
for (let i = 0; i < len; i++) src[i] = i;

let offset;
let length;
if (partial === 'offset') {
offset = len >>> 2;
} else if (partial === 'offset-length') {
offset = len >>> 2;
length = len >>> 1;
}

bench.start();
for (let i = 0; i < n; i++) {
Buffer.copyBytesFrom(src, offset, length);
}
bench.end(n);
}
1 change: 1 addition & 0 deletions benchmark/buffers/buffer-fill.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const bench = common.createBenchmark(main, {
'fill("t")',
'fill("test")',
'fill("t", "utf8")',
'fill("t", "ascii")',
'fill("t", 0, "utf8")',
'fill("t", 0)',
'fill(Buffer.alloc(1), 0)',
Expand Down
2 changes: 1 addition & 1 deletion benchmark/buffers/buffer-indexof.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const searchStrings = [

const bench = common.createBenchmark(main, {
search: searchStrings,
encoding: ['undefined', 'utf8', 'ucs2', 'latin1'],
encoding: ['undefined', 'utf8', 'ascii', 'latin1', 'ucs2'],
type: ['buffer', 'string'],
n: [5e4],
}, {
Expand Down
2 changes: 1 addition & 1 deletion benchmark/buffers/buffer-tostring.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
const common = require('../common.js');

const bench = common.createBenchmark(main, {
encoding: ['', 'utf8', 'ascii', 'latin1', 'hex', 'UCS-2'],
encoding: ['', 'utf8', 'ascii', 'latin1', 'hex', 'base64', 'base64url', 'UCS-2'],
args: [0, 1, 3],
len: [1, 64, 1024],
n: [1e6],
Expand Down
109 changes: 60 additions & 49 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ const {
TypedArrayPrototypeGetByteOffset,
TypedArrayPrototypeGetLength,
TypedArrayPrototypeSet,
TypedArrayPrototypeSlice,
TypedArrayPrototypeSubarray,
Uint8Array,
} = primordials;
Expand Down Expand Up @@ -383,28 +382,33 @@ Buffer.copyBytesFrom = function copyBytesFrom(view, offset, length) {
return new FastBuffer();
}

if (offset !== undefined || length !== undefined) {
if (offset !== undefined) {
validateInteger(offset, 'offset', 0);
if (offset >= viewLength) return new FastBuffer();
} else {
offset = 0;
}
let end;
if (length !== undefined) {
validateInteger(length, 'length', 0);
end = offset + length;
} else {
end = viewLength;
}
let start = 0;
let end = viewLength;

view = TypedArrayPrototypeSlice(view, offset, end);
if (offset !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

Would switching from TypedArrayPrototypeSlice to TypedArrayPrototypeSubarray be easier here? It avoids the extraneous copy possibly eliminates the more complicated manual processing here.

Copy link
Contributor Author

@thisalihassan thisalihassan Mar 22, 2026

Choose a reason for hiding this comment

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

Ok @jasnell I tried as you suggested, here's my implementation did bench with the current implementation and there was consistant 14-20% of regresssion with TypedArrayPrototypeSubarray although I like the code, it's much cleaner

Buffer.copyBytesFrom = function copyBytesFrom(view, offset, length) {
  if (!isTypedArray(view)) {
    throw new ERR_INVALID_ARG_TYPE('view', [ 'TypedArray' ], view);
  }

  const viewLength = TypedArrayPrototypeGetLength(view);
  if (viewLength === 0) {
    return new FastBuffer();
  }

  let start = 0;
  let end = viewLength;

  if (offset !== undefined) {
    validateInteger(offset, 'offset', 0);
    if (offset >= viewLength) return new FastBuffer();
    start = offset;
  }

  if (length !== undefined) {
    validateInteger(length, 'length', 0);
    end = start + length;
  }

  view = TypedArrayPrototypeSubarray(view, start, end);

  return fromArrayLike(new Uint8Array(
    TypedArrayPrototypeGetBuffer(view),
    TypedArrayPrototypeGetByteOffset(view),
    TypedArrayPrototypeGetByteLength(view)));
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the bench between current imlementation vs your suggestion:

thisalihassan@Alis-MacBook-Pro node % cat local/benchmarks/buffers/jsnell-copybytes-subarray.csv | Rscript benchmark/compare.R
                                                                                                confidence improvement accuracy (*)   (**)  (***)
buffers/buffer-copy-bytes-from.js n=600000 partial='none' len=2048 type='Float64Array'                         -0.38 %       ±3.41% ±4.53% ±5.90%
buffers/buffer-copy-bytes-from.js n=600000 partial='none' len=2048 type='Uint16Array'                          -2.45 %       ±2.93% ±3.90% ±5.07%
buffers/buffer-copy-bytes-from.js n=600000 partial='none' len=2048 type='Uint32Array'                    *      4.21 %       ±3.69% ±4.92% ±6.41%
buffers/buffer-copy-bytes-from.js n=600000 partial='none' len=2048 type='Uint8Array'                   ***     -8.81 %       ±2.34% ±3.11% ±4.06%
buffers/buffer-copy-bytes-from.js n=600000 partial='none' len=256 type='Float64Array'                  ***     -9.65 %       ±2.32% ±3.08% ±4.02%
buffers/buffer-copy-bytes-from.js n=600000 partial='none' len=256 type='Uint16Array'                   ***    -14.88 %       ±1.09% ±1.45% ±1.89%
buffers/buffer-copy-bytes-from.js n=600000 partial='none' len=256 type='Uint32Array'                   ***    -12.90 %       ±2.06% ±2.75% ±3.62%
buffers/buffer-copy-bytes-from.js n=600000 partial='none' len=256 type='Uint8Array'                    ***    -18.38 %       ±2.80% ±3.76% ±4.96%
buffers/buffer-copy-bytes-from.js n=600000 partial='none' len=64 type='Float64Array'                   ***    -14.73 %       ±1.08% ±1.44% ±1.88%
buffers/buffer-copy-bytes-from.js n=600000 partial='none' len=64 type='Uint16Array'                    ***    -21.16 %       ±0.91% ±1.21% ±1.58%
buffers/buffer-copy-bytes-from.js n=600000 partial='none' len=64 type='Uint32Array'                    ***    -19.46 %       ±2.12% ±2.84% ±3.74%
buffers/buffer-copy-bytes-from.js n=600000 partial='none' len=64 type='Uint8Array'                     ***    -21.50 %       ±0.94% ±1.25% ±1.64%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset-length' len=2048 type='Float64Array'                 1.55 %       ±2.88% ±3.83% ±4.99%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset-length' len=2048 type='Uint16Array'         ***     -9.22 %       ±1.52% ±2.03% ±2.64%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset-length' len=2048 type='Uint32Array'                 -0.56 %       ±2.80% ±3.72% ±4.84%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset-length' len=2048 type='Uint8Array'          ***    -13.41 %       ±1.26% ±1.68% ±2.19%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset-length' len=256 type='Float64Array'         ***    -14.31 %       ±1.74% ±2.32% ±3.04%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset-length' len=256 type='Uint16Array'          ***    -18.77 %       ±0.86% ±1.15% ±1.49%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset-length' len=256 type='Uint32Array'          ***    -14.93 %       ±2.59% ±3.48% ±4.57%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset-length' len=256 type='Uint8Array'           ***    -19.45 %       ±0.81% ±1.08% ±1.41%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset-length' len=64 type='Float64Array'          ***    -16.75 %       ±2.95% ±3.94% ±5.14%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset-length' len=64 type='Uint16Array'           ***    -20.57 %       ±0.92% ±1.23% ±1.61%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset-length' len=64 type='Uint32Array'           ***    -19.43 %       ±0.74% ±0.98% ±1.28%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset-length' len=64 type='Uint8Array'            ***    -20.58 %       ±1.11% ±1.48% ±1.92%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset' len=2048 type='Float64Array'               ***     13.69 %       ±4.00% ±5.32% ±6.93%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset' len=2048 type='Uint16Array'                ***     -8.51 %       ±2.33% ±3.10% ±4.03%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset' len=2048 type='Uint32Array'                        -2.65 %       ±2.74% ±3.65% ±4.77%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset' len=2048 type='Uint8Array'                 ***    -10.05 %       ±1.26% ±1.68% ±2.19%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset' len=256 type='Float64Array'                ***    -10.48 %       ±1.17% ±1.55% ±2.02%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset' len=256 type='Uint16Array'                 ***    -15.20 %       ±1.14% ±1.52% ±1.98%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset' len=256 type='Uint32Array'                 ***    -13.07 %       ±1.19% ±1.59% ±2.07%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset' len=256 type='Uint8Array'                  ***    -19.86 %       ±1.38% ±1.84% ±2.42%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset' len=64 type='Float64Array'                 ***    -14.71 %       ±0.92% ±1.23% ±1.60%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset' len=64 type='Uint16Array'                  ***    -21.59 %       ±1.08% ±1.44% ±1.88%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset' len=64 type='Uint32Array'                  ***    -19.28 %       ±1.95% ±2.59% ±3.38%
buffers/buffer-copy-bytes-from.js n=600000 partial='offset' len=64 type='Uint8Array'                   ***    -22.33 %       ±1.03% ±1.37% ±1.79%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 36 comparisons, you can thus
expect the following amount of false-positive results:
  1.80 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.36 false positives, when considering a   1% risk acceptance (**, ***),
  0.04 false positives, when considering a 0.1% risk acceptance (***)

Copy link
Member

Choose a reason for hiding this comment

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

hmm... that would still make it a net improvement over the current implementation. I'll leave it up to you, I'm just not a big fan of the manual handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I was aiming to maxing perf, I will keep it if the regression was less I would have kept your solution

validateInteger(offset, 'offset', 0);
if (offset >= viewLength) return new FastBuffer();
start = offset;
}

if (length !== undefined) {
validateInteger(length, 'length', 0);
// The old code used TypedArrayPrototypeSlice which clamps internally.
end = MathMin(start + length, viewLength);
}

if (end <= start) return new FastBuffer();

const byteLength = TypedArrayPrototypeGetByteLength(view);
const elementSize = byteLength / viewLength;
const srcByteOffset = TypedArrayPrototypeGetByteOffset(view) +
start * elementSize;
const srcByteLength = (end - start) * elementSize;

return fromArrayLike(new Uint8Array(
TypedArrayPrototypeGetBuffer(view),
TypedArrayPrototypeGetByteOffset(view),
TypedArrayPrototypeGetByteLength(view)));
srcByteOffset,
srcByteLength));
};

// Identical to the built-in %TypedArray%.of(), but avoids using the deprecated
Expand Down Expand Up @@ -551,14 +555,15 @@ function fromArrayBuffer(obj, byteOffset, length) {
}

function fromArrayLike(obj) {
if (obj.length <= 0)
const { length } = obj;
if (length <= 0)
return new FastBuffer();
if (obj.length < (Buffer.poolSize >>> 1)) {
if (obj.length > (poolSize - poolOffset))
if (length < (Buffer.poolSize >>> 1)) {
if (length > (poolSize - poolOffset))
createPool();
const b = new FastBuffer(allocPool, poolOffset, obj.length);
const b = new FastBuffer(allocPool, poolOffset, length);
TypedArrayPrototypeSet(b, obj, 0);
poolOffset += obj.length;
poolOffset += length;
alignPool();
return b;
}
Expand Down Expand Up @@ -732,11 +737,7 @@ const encodingOps = {
write: asciiWrite,
slice: asciiSlice,
indexOf: (buf, val, byteOffset, dir) =>
indexOfBuffer(buf,
fromStringFast(val, encodingOps.ascii),
byteOffset,
encodingsMap.ascii,
dir),
indexOfString(buf, val, byteOffset, encodingsMap.ascii, dir),
},
base64: {
encoding: 'base64',
Expand Down Expand Up @@ -1118,7 +1119,9 @@ function _fill(buf, value, offset, end, encoding) {
value = 0;
} else if (value.length === 1) {
// Fast path: If `value` fits into a single byte, use that numeric value.
if (normalizedEncoding === 'utf8') {
// ASCII shares this branch with utf8 since code < 128 covers the full
// ASCII range; anything outside falls through to C++ bindingFill.
if (normalizedEncoding === 'utf8' || normalizedEncoding === 'ascii') {
Copy link
Member

Choose a reason for hiding this comment

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

Currently, ascii behaves exactly like latin1
Unsure if by design or accidentally

Copy link
Contributor Author

@thisalihassan thisalihassan Feb 18, 2026

Choose a reason for hiding this comment

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

yes, this is safe by design I am just extending the existing single byte numeric optimization to cover ASCII, since the guard already constrains it to the valid ASCII range.

const code = StringPrototypeCharCodeAt(value, 0);
if (code < 128) {
value = code;
Expand Down Expand Up @@ -1168,29 +1171,30 @@ function _fill(buf, value, offset, end, encoding) {
}

Buffer.prototype.write = function write(string, offset, length, encoding) {
const bufferLength = this.length;
// Buffer#write(string);
if (offset === undefined) {
return utf8Write(this, string, 0, this.length);
return utf8Write(this, string, 0, bufferLength);
}
// Buffer#write(string, encoding)
if (length === undefined && typeof offset === 'string') {
encoding = offset;
length = this.length;
length = bufferLength;
offset = 0;

// Buffer#write(string, offset[, length][, encoding])
} else {
validateOffset(offset, 'offset', 0, this.length);
validateOffset(offset, 'offset', 0, bufferLength);

const remaining = this.length - offset;
const remaining = bufferLength - offset;

if (length === undefined) {
length = remaining;
} else if (typeof length === 'string') {
encoding = length;
length = remaining;
} else {
validateOffset(length, 'length', 0, this.length);
validateOffset(length, 'length', 0, bufferLength);
if (length > remaining)
length = remaining;
}
Expand All @@ -1208,9 +1212,10 @@ Buffer.prototype.write = function write(string, offset, length, encoding) {
};

Buffer.prototype.toJSON = function toJSON() {
if (this.length > 0) {
const data = new Array(this.length);
for (let i = 0; i < this.length; ++i)
const len = this.length;
if (len > 0) {
const data = new Array(len);
for (let i = 0; i < len; ++i)
data[i] = this[i];
return { type: 'Buffer', data };
}
Expand Down Expand Up @@ -1253,45 +1258,50 @@ function swap(b, n, m) {
}

Buffer.prototype.swap16 = function swap16() {
// For Buffer.length < 128, it's generally faster to
// Ref: https://github.com/nodejs/node/pull/61871#discussion_r2889557696
// For Buffer.length <= 32, it's generally faster to
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a reference link to this pull-request above this line

// do the swap in javascript. For larger buffers,
// dropping down to the native code is faster.
const len = this.length;
const len = TypedArrayPrototypeGetLength(this);
Copy link
Member

Choose a reason for hiding this comment

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

While this should provide a viable defense against crashing if this isn't a typed array, the error is going to be somewhat non-obvious...

Welcome to Node.js v26.0.0-pre.
Type ".help" for more information.
> const { swap16 } = Buffer.prototype
undefined
> swap16(1)
Uncaught:
TypeError: Method get TypedArray.prototype.length called on incompatible receiver undefined
    at get length (<anonymous>)
    at swap16 (node:buffer:1265:15)
> 

Having this instead be a ERR_INVALID_THIS error with a more specific error message would be nicer.

Copy link
Member

Choose a reason for hiding this comment

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

This would have a performance impact wouldn't it?

Copy link
Contributor Author

@thisalihassan thisalihassan Mar 22, 2026

Choose a reason for hiding this comment

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

I agree about the error have discussed it with @aduh95 in this thread

Copy link
Member

Choose a reason for hiding this comment

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

These are used so infrequently that I'm not concerned about the performance impact of an additional isArrayBufferView() type check

Copy link
Contributor Author

@thisalihassan thisalihassan Mar 23, 2026

Choose a reason for hiding this comment

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

Added isArrayBufferView guard with ERR_INVALID_THIS to all three swap methods. Here's the behavior comparison:

swap16

Receiver main branch This PR
Buffer(4) OK OK
Uint8Array(4) OK OK
Float32Array(4) OK OK
Int16Array(4) OK OK
Array(4) OK ERR_INVALID_THIS
Array(200) ERR_INVALID_ARG_TYPE ERR_INVALID_THIS
number(1) ERR_INVALID_BUFFER_SIZE ERR_INVALID_THIS
string("ab") TypeError (read-only property) ERR_INVALID_THIS
plain object OK (silent garbage) ERR_INVALID_THIS
null TypeError ERR_INVALID_THIS
undefined TypeError ERR_INVALID_THIS

I don't think any of this is now considered breaking changes, some used to work but that shouldn't have worked realistically @jasnell
cc: @aduh95

if (len % 2 !== 0)
throw new ERR_INVALID_BUFFER_SIZE('16-bits');
if (len < 128) {
if (len <= 32) {
for (let i = 0; i < len; i += 2)
swap(this, i, i + 1);
return this;
}
return _swap16(this);
_swap16(this);
Copy link
Member

@ChALkeR ChALkeR Mar 5, 2026

Choose a reason for hiding this comment

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

what would happen if it's called on not a TypeArrayView now?

Copy link
Member

Choose a reason for hiding this comment

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

i.e. Buffer.prototype.swap16.apply(new Array(128))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I will add a JS side guard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would that be good approach @ChALkeR ?

Copy link
Member

Choose a reason for hiding this comment

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

It changes the method to not being callable on a typedarray that is not an uint8array, but the previous behavior was inconsistent in that case anyway as the js part swapped elements and the src part swapped bytes

So I think explicitly blocking non-uint8arr should be fine and not a semver-major?
(perhaps cc @nodejs/lts just in case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right about that, I was pointing about the inconsistency about the different paths. as JS swapped the elements but alright I will work on the feedback
Thanks

Copy link
Contributor

@aduh95 aduh95 Mar 17, 2026

Choose a reason for hiding this comment

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

I'm not against changing the current state of things, but IMO that's a separate concern and should be discussed separately. Currently Buffer.prototype.swap16.call(new Float32Array(128)) does not throw, so if we want to make it throw let's open a separate semver-major PR.
My opinion is we should rely on TypedArrayPrototypeGetByteLength doing the identity check for us in this PR, better for performance and not worse than the status quo for DX/UX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's pretty valid @aduh95, I have used TypedArrayPrototypeGetByteLength I will open a seperate issue on this.

Copy link
Member

@ChALkeR ChALkeR Mar 17, 2026

Choose a reason for hiding this comment

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

Currently Buffer.prototype.swap16.call(new Float32Array(128)) does not throw,

It does not throw but returns garbage (as in, result is not predictable / does not follow any logic)
So anything can be done with it I think

Making it swap bytes is an option. Making it throw is another option.

The current behavior is length-dependent:

  • 129 (516 bytes) throws and complains about non-even number of bytes
  • 128 (512 bytes) does not throw (as you pointed out) and swaps bytes
  • 127 (508 bytes) throws and complains about non-even number of bytes
  • 126 (504 bytes) does not throw and swaps elements (groups of 4 bytes)

That is a mere oversight and not a proper api, and, more significantly, no one should be using that realistically as it doesn't work.
I don't think that making that a hard error is a semver-major.

Copy link
Contributor

@aduh95 aduh95 Mar 17, 2026

Choose a reason for hiding this comment

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

Passing a non-Buffer this is not documented, so it implicitly is undefined behavior.
Garbage out is IMO a good tradeoff for a more performant happy path, although I didn't run any benchmark.

no one should be using that realistically as it doesn't work.

100% agreed – and I would bet that absolutely no one is passing Float32Arrays to that API. Still, I don't think it's worth changing in this PR, which is about improving performance.

return this;
};

Buffer.prototype.swap32 = function swap32() {
// For Buffer.length < 192, it's generally faster to
// Ref: https://github.com/nodejs/node/pull/61871#discussion_r2889557696
// For Buffer.length <= 32, it's generally faster to
// do the swap in javascript. For larger buffers,
// dropping down to the native code is faster.
const len = this.length;
const len = TypedArrayPrototypeGetLength(this);
if (len % 4 !== 0)
throw new ERR_INVALID_BUFFER_SIZE('32-bits');
if (len < 192) {
if (len <= 32) {
for (let i = 0; i < len; i += 4) {
swap(this, i, i + 3);
swap(this, i + 1, i + 2);
}
return this;
}
return _swap32(this);
_swap32(this);
return this;
};

Buffer.prototype.swap64 = function swap64() {
// For Buffer.length < 192, it's generally faster to
// Ref: https://github.com/nodejs/node/pull/61871#discussion_r2889557696
// For Buffer.length < 48, it's generally faster to
// do the swap in javascript. For larger buffers,
// dropping down to the native code is faster.
const len = this.length;
const len = TypedArrayPrototypeGetLength(this);
if (len % 8 !== 0)
throw new ERR_INVALID_BUFFER_SIZE('64-bits');
if (len < 192) {
if (len < 48) {
Copy link
Member

Choose a reason for hiding this comment

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

In the swap16 and swap32, the threshold check is len <=, while this is len < ... is there a particular reason for the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes intentional the crossover point for swap64 is between 40 and 48, check the bench here

Copy link
Member

Choose a reason for hiding this comment

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

A comment here would be helpful then.

for (let i = 0; i < len; i += 8) {
swap(this, i, i + 7);
swap(this, i + 1, i + 6);
Expand All @@ -1300,7 +1310,8 @@ Buffer.prototype.swap64 = function swap64() {
}
return this;
}
return _swap64(this);
_swap64(this);
return this;
};

Buffer.prototype.toLocaleString = Buffer.prototype.toString;
Expand Down
57 changes: 44 additions & 13 deletions src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1050,7 +1050,7 @@ void IndexOfString(const FunctionCallbackInfo<Value>& args) {
needle_length,
offset,
is_forward);
} else if (enc == LATIN1) {
} else if (enc == ASCII || enc == LATIN1) {
MaybeStackBuffer<uint8_t> needle_data(needle_length);
StringBytes::Write(isolate,
reinterpret_cast<char*>(needle_data.out()),
Expand Down Expand Up @@ -1196,31 +1196,59 @@ int32_t FastIndexOfNumber(Local<Value>,
static CFunction fast_index_of_number(CFunction::Make(FastIndexOfNumber));

void Swap16(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]);
DCHECK(args[0]->IsArrayBufferView());
Copy link
Member

Choose a reason for hiding this comment

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

Although I agree that DCHECK is sufficient here, we always use non-debug CHECK's. If we want to replace this, we should do it in all of the codebase.

Suggested change
DCHECK(args[0]->IsArrayBufferView());
CHECK(args[0]->IsArrayBufferView());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we already haveSPREAD_BUFFER_ARG for Check thenDCheck looks redudand want me to remove this?

SPREAD_BUFFER_ARG(args[0], ts_obj);
CHECK(nbytes::SwapBytes16(ts_obj_data, ts_obj_length));
args.GetReturnValue().Set(args[0]);
}

void FastSwap16(Local<Value> receiver,
Local<Value> buffer_obj,
// NOLINTNEXTLINE(runtime/references)
FastApiCallbackOptions& options) {
TRACK_V8_FAST_API_CALL("buffer.swap16");
HandleScope scope(options.isolate);
SPREAD_BUFFER_ARG(buffer_obj, ts_obj);
Copy link
Member

Choose a reason for hiding this comment

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

This fast api call is missing DCHECK(buffer_obj->IsArrayBufferView());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SPREAD_BUFFER_ARG already do CHECK((val)->IsArrayBufferView()); which is why I didn't add DCheck here

CHECK(nbytes::SwapBytes16(ts_obj_data, ts_obj_length));
}
Comment on lines +1204 to +1212
Copy link
Member

Choose a reason for hiding this comment

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

These fast callbacks are non-identical to the conventional callbacks they shadow.

  • The existing callbacks validate their argument and throws to JS if invalid, whereas your fast callbacks hard-crash the process. It might be better to validate in the JS layer, then use the same unwrapping logic on both sides.
  • Your fast callback cannot have a different return convention to the conventional callback. You will need to remove the return value from the conventional callback.

Copy link
Member

@ChALkeR ChALkeR Mar 5, 2026

Choose a reason for hiding this comment

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

Was removing the validation instead of moving it to js intentional?

Also fast paths can keep the validation and fall back to slow i think, so we can still validate on the src side?

Copy link
Member

Choose a reason for hiding this comment

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

Also fast paths can keep the validation and fall back to slow i think, so we can still validate on the src side?

This is outdated, the fast API doesn't use fallback any more (since Node.js v23.x).

However, any validation in a JS wrapper should be shadowed by a CHECK or DCHECK in the C++ binding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SPREAD_BUFFER_ARG already contains CHECK((val)->IsArrayBufferView()) on all Slow & Fast swap methods

Comment on lines +1204 to +1212
Copy link
Member

Choose a reason for hiding this comment

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

Fast callbacks should include debug tracking and call tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for sharing these I will update the code


static CFunction fast_swap16(CFunction::Make(FastSwap16));

void Swap32(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]);
DCHECK(args[0]->IsArrayBufferView());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DCHECK(args[0]->IsArrayBufferView());
CHECK(args[0]->IsArrayBufferView());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above comment

SPREAD_BUFFER_ARG(args[0], ts_obj);
CHECK(nbytes::SwapBytes32(ts_obj_data, ts_obj_length));
args.GetReturnValue().Set(args[0]);
}

void FastSwap32(Local<Value> receiver,
Local<Value> buffer_obj,
// NOLINTNEXTLINE(runtime/references)
FastApiCallbackOptions& options) {
TRACK_V8_FAST_API_CALL("buffer.swap32");
HandleScope scope(options.isolate);
SPREAD_BUFFER_ARG(buffer_obj, ts_obj);
Copy link
Member

Choose a reason for hiding this comment

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

check is arraybufferview is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SPREAD_BUFFER_ARG already do CHECK((val)->IsArrayBufferView()); which is why I didn't add DCheck here

CHECK(nbytes::SwapBytes32(ts_obj_data, ts_obj_length));
}

static CFunction fast_swap32(CFunction::Make(FastSwap32));

void Swap64(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]);
DCHECK(args[0]->IsArrayBufferView());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DCHECK(args[0]->IsArrayBufferView());
CHECK(args[0]->IsArrayBufferView());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as older comment

SPREAD_BUFFER_ARG(args[0], ts_obj);
CHECK(nbytes::SwapBytes64(ts_obj_data, ts_obj_length));
args.GetReturnValue().Set(args[0]);
}

void FastSwap64(Local<Value> receiver,
Local<Value> buffer_obj,
// NOLINTNEXTLINE(runtime/references)
FastApiCallbackOptions& options) {
TRACK_V8_FAST_API_CALL("buffer.swap64");
HandleScope scope(options.isolate);
SPREAD_BUFFER_ARG(buffer_obj, ts_obj);
Copy link
Member

Choose a reason for hiding this comment

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

check is arraybufferview is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

CHECK(nbytes::SwapBytes64(ts_obj_data, ts_obj_length));
}

static CFunction fast_swap64(CFunction::Make(FastSwap64));

static void IsUtf8(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK_EQ(args.Length(), 1);
Expand Down Expand Up @@ -1619,9 +1647,9 @@ void Initialize(Local<Object> target,
SetMethodNoSideEffect(
context, target, "createUnsafeArrayBuffer", CreateUnsafeArrayBuffer);

SetMethod(context, target, "swap16", Swap16);
SetMethod(context, target, "swap32", Swap32);
SetMethod(context, target, "swap64", Swap64);
SetFastMethod(context, target, "swap16", Swap16, &fast_swap16);
SetFastMethod(context, target, "swap32", Swap32, &fast_swap32);
SetFastMethod(context, target, "swap64", Swap64, &fast_swap64);

SetMethodNoSideEffect(context, target, "isUtf8", IsUtf8);
SetMethodNoSideEffect(context, target, "isAscii", IsAscii);
Expand Down Expand Up @@ -1690,8 +1718,11 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(IndexOfString);

registry->Register(Swap16);
registry->Register(fast_swap16);
registry->Register(Swap32);
registry->Register(fast_swap32);
registry->Register(Swap64);
registry->Register(fast_swap64);

registry->Register(IsUtf8);
registry->Register(IsAscii);
Expand Down
Loading
Loading