Closed
Conversation
Member
|
If the figures from #60399 still apply for ArrayBuffer-backed buffers, is there scope to add |
Member
Author
doing all of these type checks also adds overhead. Furthermore the benchmark from that PR apply to concat so it's unclear what it's actual effect on |
RafaelGSS
approved these changes
Feb 27, 2026
Collaborator
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62032 +/- ##
==========================================
+ Coverage 89.65% 89.69% +0.03%
==========================================
Files 676 676
Lines 206231 206236 +5
Branches 39505 39759 +254
==========================================
+ Hits 184898 184981 +83
+ Misses 13463 13404 -59
+ Partials 7870 7851 -19
π New features to boost your workflow:
|
anonrig
approved these changes
Feb 28, 2026
Renegade334
reviewed
Feb 28, 2026
Member
Renegade334
left a comment
There was a problem hiding this comment.
Think it's worth mentioning in the commit message that this is essentially a reversion of 24bebd0, sans tests.
This fixes a performance regression for Buffer.copy(target, 0) and brings it back inline with Buffer.write.
V8 has a massive TypedArray.prototype.set penalty on SharedArrayBuffer
Buffer.set and Buffer.copy are up to 8.4x slower when writing to a SharedArrayBuffer vs a regular ArrayBuffer, while Buffer.write (string encoding) is completely unaffected.
256 bytes, varying offset (Apple M3 Pro, Node 25.6.1):
ArrayBuffer SharedArrayBuffer Slowdown
Buffer.set 13.6 ns 56.1 ns 4.1x
Buffer.copy 17.0 ns 65.1 ns 3.8x
Buffer.write 75.8 ns 74.1 ns 1.0x (unaffected)
4096 bytes, varying offset:
ArrayBuffer SharedArrayBuffer Slowdown
Buffer.set 80.3 ns 674.2 ns 8.4x
Buffer.copy 78.4 ns 677.7 ns 8.6x
Buffer.write 190.6 ns 186.1 ns 1.0x (unaffected)
Contributor
Failed to start CIβ Commits were pushed since the last approving review: β - buffer: always use _copy for copy β Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/22516891493 |
richardlau
approved these changes
Feb 28, 2026
gurgunday
requested changes
Feb 28, 2026
Member
There was a problem hiding this comment.
I am seeing a significant regression in non-partial copy performance:
node % ./node benchmark/buffers/buffer-copy.js
buffers/buffer-copy.js n=6000000 partial="true" bytes=8: 37,745,651.53108819
buffers/buffer-copy.js n=6000000 partial="false" bytes=8: 72,230,886.31703596
buffers/buffer-copy.js n=6000000 partial="true" bytes=128: 37,269,250.926878504
buffers/buffer-copy.js n=6000000 partial="false" bytes=128: 70,233,110.24800341
buffers/buffer-copy.js n=6000000 partial="true" bytes=1024: 26,739,253.757964805
buffers/buffer-copy.js n=6000000 partial="false" bytes=1024: 37,699,305.6259145
node % ./node-ronag benchmark/buffers/buffer-copy.js
buffers/buffer-copy.js n=6000000 partial="true" bytes=8: 36,820,630.5993248
buffers/buffer-copy.js n=6000000 partial="false" bytes=8: 37,401,995.084754474
buffers/buffer-copy.js n=6000000 partial="true" bytes=128: 37,360,956.762793645
buffers/buffer-copy.js n=6000000 partial="false" bytes=128: 35,075,651.428067446
buffers/buffer-copy.js n=6000000 partial="true" bytes=1024: 26,155,344.217952482
buffers/buffer-copy.js n=6000000 partial="false" bytes=1024: 22,480,048.984626204Those checks are necessary to hit the V8 .set optimization, it is faster than our native copy, I think we should instead make those checks more strict if you found a case that's slower, as @Renegade334 suggested
You are right that .concat no longer hits that path, but copy still does
Cc @ChALkeR
Contributor
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1805/ Results |
Member
|
Let's hold off until we figure out what's going on @ronag Which benchmark is that in the PR description? If it's not in tree, could you add it (or modify buffer-copy.js) to test behavior on:
All of those are important / could give different results This should be tested with benchmarks code Also the bech should have 64/128 bytes in sizes (along others lower/higher) as there is a sharp difference there in how the data is stored in some cases. |
Member
|
Also i'm curious why is v8 slower on SAB there |
Member
Author
Because the javascript memory model is super strict. |
Member
Author
|
ChatGPT:
|
Member
Author
|
So from what I understand on ARM the implementation is not allowed to use SIMD since it invalidated the "no-tear" requirement of the memory model, while on x86 this is not a problem. I think we maybe should document that Buffer.copy (and in general all buffer operations that we implement) do not strictly follow the ecmascript memory model guarantees. @mcollina |
Member
AFAIA it's that V8 uses a libatomic-based memmove implementation if the source or target is SAB-backed. |
Member
|
DetailsDoc: https://nodejs.org/api/buffer.html#bufcopytarget-targetstart-sourcestart-sourceend > Copies data from a region of buf to a region in target, even if the target memory region overlaps with buf.It should be using memove, not memcpy then Those are different under some libs/optimizations E.g.: I have not verified that the issue exists yet, but given the overall lack of bound checks in _copy it needs a recheck Upd: No,
This is hallucinated, |
Member
How often are Buffer.from(SAB) used and where? |
Member
Author
When it's used it's super important. It's the only way to efficiently use workers in node. Also implementing that guarantee needs us to update most buffer operations which will make things harder to maintain, e.g. rewriting and TBH it doesn't make any difference for this PR since sometimes the implementation uses set and sometimes _copy which make the whole situation even more confusing. |
Member
Author
It's talking about typedarray.set, not _copy. |
Member
Author
|
@ChALkeR I do see you have good intentions here and you are probably right. But this is becoming to much work for what I feel is a rather straightforward change. We run x86 in production which is not affected by this so I can't motivate spending much more time on this. Please feel free to take over this PR or open a different one. I'm happy to continue discussing but getting this over the finish line seems like a too big task for me at the moment. IMHO I would recommend to land this is as revert of a change causing a perf regression and then start over with 2 different PR's in regards to the memory model and the performance (set is faster in certain scenarios) topics. |
Member
Author
|
I think @jasnell might have some insightful opinions in regards to the difference between memory models of |
Member
Yes, it's talking about typedarray.set vs memcpy, and the bottom part of that is not relevant as this is PR is really typedarray.set vs _copy which is memmove (not memcpy), so memcpy overlapping concerns (the cited part) do not apply to this PR. So that's not the answer.
Perf regression under which usecase?
Also it's not demonstrated. |
Member
If this is about There might be more non-literal matches, but this hints at low usage anyway. What exactly is affected that justifies regressing note: this perf question is orthogonal to the concern that using |
ChALkeR
requested changes
Mar 1, 2026
Member
There was a problem hiding this comment.
I don't think we should land perf-only PRs without benchmarks that demonstrate the improvement
So far, existing benchmarks demonstrate only slowdown
Even if this is landed (in the current form), it is likely to be flipped back in another attempt to optimize under the guidance of maintained benchmarks
Separately, when benchmarks are introduced, we should consider if that's a usecase that is likely to happen / under what conditions (and if that is worth a slowdown on other common usecases). I don't say that this doesn't have usage, but so far it's unclear from the current PR.
Member
Author
import { bench, run, group, summary } from 'mitata'
const SIZES = [64, 256, 1024, 4096, 16_384, 65_536, 256_000, 1024_000]
for (const size of SIZES) {
const src = Buffer.alloc(size, 0x42)
const abDst = Buffer.from(new ArrayBuffer(size))
const sabDst = Buffer.from(new SharedArrayBuffer(size))
summary(() => {
group(`Buffer.copy ${size} bytes`, () => {
bench(`ArrayBuffer (${size})`, () => {
src.copy(abDst)
})
bench(`SharedArrayBuffer (${size})`, () => {
src.copy(sabDst)
})
})
})
}
await run() |
Member
Author
|
@RafaelGSS maybe you can assists with measuring + benchmarks on this to possibly unblock the PR? |
Member
|
Thanks for the benchmark! I think it was a mistake to compare only relative SAB/AB perf, not absolute with/without patch. According to that bench (modified to not use a dep), comparing 24.11.0 to 24.12.0 (which introduced that change in the first place), I expect that this PR is a slowdown for both SAB and ArrayBuffer on sizes <= 256 (or slightly larger) It makes AB and SAB values approximately equal there, so the coef is brought to ~1x, but it just makes them equally slow. For sizes 1k-16k, according to that provided benchmark, this significantly slows down regular Buffer but significantly improves SAB. For sizes >=65k this has almost no effect on regular Buffer but significantly improves SAB. This hints at a better fix - not plain revert (that makes AB worse and even SAB worse on small buffers), not SAB detection, but a mix of SAB + size detection. 24.12.0 (with #60399): 24.11.0 (likely where with this revert would bring us): That said, I have not tested applying this to main yet. |
ChALkeR
requested changes
Mar 1, 2026
Member
There was a problem hiding this comment.
I re-tested this PR and compared it to base branch without it
In short: this PR description mentions sizes 256 and 4096 with relative "slowdown" on SAB compared to AB, and claims to improve that relative slowdown.
What is does instead is makes both AB and SAB slower on size=256. Down to an identical value though.
It has benefits for on higher lengths though, for Buffer.from(new SharedArrayBuffer( only.
But it also still hurts regular Buffer instances up to ~16k, and Buffer.from(new SharedArrayBuffer( is very rare to justify noticeable slowdown on regular Buffer.
This needs not just benchmarks, but the implementation fixed to not cause a perf regression.
Base
ArrayBuffer (64) x 54,529,149 ops/sec @ 18ns/op (0ns..305ΞΌs)
SharedArrayBuffer (64) x 53,716,573 ops/sec @ 18ns/op (0ns..38ΞΌs)
ArrayBuffer (256) x 52,328,394 ops/sec @ 19ns/op (0ns..41ΞΌs)
SharedArrayBuffer (256) x 41,411,669 ops/sec @ 24ns/op (0ns..42ΞΌs)
ArrayBuffer (1024) x 38,529,150 ops/sec @ 25ns/op (0ns..36ΞΌs)
SharedArrayBuffer (1024) x 18,232,579 ops/sec @ 54ns/op (0ns..41ΞΌs)
ArrayBuffer (4096) x 16,600,224 ops/sec @ 60ns/op (0ns..40ΞΌs)
SharedArrayBuffer (4096) x 6,146,755 ops/sec @ 162ns/op (42ns..59ΞΌs)
ArrayBuffer (16384) x 3,042,573 ops/sec @ 328ns/op (83ns..72ΞΌs)
SharedArrayBuffer (16384) x 1,741,612 ops/sec @ 574ns/op (458ns..45ΞΌs)
ArrayBuffer (65536) x 1,053,112 ops/sec @ 949ns/op (666ns..70ΞΌs)
SharedArrayBuffer (65536) x 451,151 ops/sec @ 2ΞΌs/op (2ΞΌs..60ΞΌs)
ArrayBuffer (256000) x 251,257 ops/sec @ 3ΞΌs/op (3ΞΌs..10ms)
SharedArrayBuffer (256000) x 116,322 ops/sec @ 8ΞΌs/op (8ΞΌs..80ΞΌs)
ArrayBuffer (1024000) x 66,171 ops/sec @ 15ΞΌs/op (14ΞΌs..206ΞΌs)
SharedArrayBuffer (1024000) x 29,225 ops/sec @ 34ΞΌs/op (33ΞΌs..135ΞΌs)
This PR
ArrayBuffer (64) x 40,335,525 ops/sec @ 24ns/op (0ns..46ΞΌs)
SharedArrayBuffer (64) x 39,725,116 ops/sec @ 25ns/op (0ns..80ΞΌs)
ArrayBuffer (256) x 35,785,083 ops/sec @ 27ns/op (0ns..45ΞΌs)
SharedArrayBuffer (256) x 35,819,652 ops/sec @ 27ns/op (0ns..39ΞΌs)
ArrayBuffer (1024) x 27,800,955 ops/sec @ 35ns/op (0ns..41ΞΌs)
SharedArrayBuffer (1024) x 27,316,682 ops/sec @ 36ns/op (0ns..57ΞΌs)
ArrayBuffer (4096) x 14,335,737 ops/sec @ 69ns/op (0ns..35ΞΌs)
SharedArrayBuffer (4096) x 14,335,732 ops/sec @ 69ns/op (0ns..84ΞΌs)
ArrayBuffer (16384) x 3,025,528 ops/sec @ 330ns/op (84ns..43ΞΌs)
SharedArrayBuffer (16384) x 2,995,333 ops/sec @ 333ns/op (125ns..36ΞΌs)
ArrayBuffer (65536) x 1,053,536 ops/sec @ 949ns/op (708ns..23ΞΌs)
SharedArrayBuffer (65536) x 1,052,752 ops/sec @ 949ns/op (708ns..67ΞΌs)
ArrayBuffer (256000) x 265,626 ops/sec @ 3ΞΌs/op (3ΞΌs..62ΞΌs)
SharedArrayBuffer (256000) x 266,723 ops/sec @ 3ΞΌs/op (3ΞΌs..55ΞΌs)
ArrayBuffer (1024000) x 66,146 ops/sec @ 15ΞΌs/op (14ΞΌs..170ΞΌs)
SharedArrayBuffer (1024000) x 66,000 ops/sec @ 15ΞΌs/op (14ΞΌs..102ΞΌs)
Member
Author
If someone wants to do that go ahead. I still think this PR is better than the current state of things, i.e. both have acceptable performance while the current state of things is AB with great performance and SAB with horrible performance. If you look at the numbers the AB regression you mention is 6ns (?) while the SAB regression I want to fix is 1000+ ns (?). Also we should document that difference in the memory model for Buffer vs TypedArray, if no-one else feels comfortable doing that I will try to make time for that at some point. |
Member
Author
|
Digging into |
Member
|
cc @nodejs/v8 |
Member
First, not only AB regression but both AB and SAB regression here up for length at least up to 256 Second, you seem to be comparing Third, see the usecase concern. This trades 1.0x-1.4x (depending on size) perf on copying This PR does demonstrate that there is smth with SAB going on on higher lengths, but it needs someone to fix this and should not land as-is. I.e. agree on "If someone wants to do that go ahead" and this is likely fixable into an actual improvement, disagree on "I still think this PR is better than the current state of things", for making vastly most common case significantly worse and the usecase concern. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes a performance regression for
Buffer.copy(target, 0)and brings it back inline with Buffer.write.V8 has a massive
TypedArray.prototype.setpenalty on SharedArrayBufferBuffer.set and Buffer.copy are up to 8.4x slower when writing to a SharedArrayBuffer vs a regular ArrayBuffer, while Buffer.write (string encoding) is completely unaffected.