buffer: optimize Buffer.prototype.copy#63828
Conversation
|
Review requested:
|
0e2e646 to
1eb628a
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates Node’s Buffer.prototype.copy() implementation to use V8’s new ArrayBuffer::CopyArrayBufferBytes / SharedArrayBuffer::CopyArrayBufferBytes APIs, improving performance for partial copies and ensuring detached/immutable targets become no-op copies that report 0 bytes copied.
Changes:
- Route the native
_copybinding through V8’s newCopyArrayBufferBytesAPI (including relaxed-atomic handling when both sides areSharedArrayBuffer-backed). - Update JS-side
copy()to return the actual number of bytes copied (as reported by the native binding), including0for immutable/detached targets. - Add test coverage for
SharedArrayBuffercopies and immutableArrayBuffertargets, plus V8 cctests and API header/docs updates.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/parallel/test-buffer-copy.js | Adds SharedArrayBuffer copy coverage (including overlap + byteOffset cases). |
| test/parallel/test-buffer-copy-immutable.js | Adds coverage for copying into immutable ArrayBuffer-backed targets (expects 0 bytes copied). |
| src/node_buffer.cc | Reworks native copy to call V8 CopyArrayBufferBytes and return actual copied byte count. |
| lib/buffer.js | Simplifies JS copy path to rely on native _copy return value for bytes-copied semantics. |
| deps/v8/test/cctest/test-api-array-buffer.cc | Adds V8 cctests for the new CopyArrayBufferBytes APIs (detached + immutable cases). |
| deps/v8/src/api/api.cc | Implements ArrayBuffer::CopyArrayBufferBytes and SharedArrayBuffer::CopyArrayBufferBytes. |
| deps/v8/include/v8-array-buffer.h | Exposes the new public V8 API methods and documents their behavior. |
| common.gypi | Bumps v8_embedder_string to reflect the added floating V8 patch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1eb628a to
8bd6850
Compare
|
can you split the v8 patch commit and the implementation commit? |
Add v8::ArrayBuffer::CopyArrayBufferBytes and
v8::SharedArrayBuffer::CopyArrayBufferBytes, which copy a byte range
from one backing store to another. V8 clamps the byte range and handles
detached and immutable buffers: nothing is copied when the source is
detached or the target is detached or immutable, and the number of bytes
actually copied is returned. The SharedArrayBuffer overload uses a
relaxed-atomic memmove that honors the SharedArrayBuffer memory model.
Carried as a floating patch cherry-picked from:
https://chromium-review.googlesource.com/c/v8/v8/+/7735151
That CL is authored by a Node.js collaborator (Ben Noordhuis) and is
expected to land upstream, so the floating patch should be easy to
maintain: it touches nothing but the public ArrayBuffer API and can be
dropped wholesale on the next V8 upgrade that includes it.
v8_embedder_string is bumped to -node.21 accordingly.
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7735151
Signed-off-by: Robert Nagy <ronagy@icloud.com>
8bd6850 to
5cb3967
Compare
Route the native backing of Buffer.prototype.copy (CopyImpl, the `_copy` binding) through V8's new v8::ArrayBuffer::CopyArrayBufferBytes API (added in the preceding commit) instead of materializing an ArrayBufferViewContents and doing a manual memmove. This speeds up partial copies (sourceStart > 0). All copies now go through this binding: the previous %TypedArray%.prototype.set fast-path for whole-buffer copies is dropped, since it would throw on a detached or immutable target rather than report a 0-byte no-op, and the benchmarks below show the native path is comparable for that case. When both sides are backed by a SharedArrayBuffer the relaxed-atomic overload is used, which honors the SharedArrayBuffer memory model. Mixed copies (one SharedArrayBuffer, one ArrayBuffer) use the regular overload. copy() now reports the number of bytes actually copied, as returned by V8: 0 when the target is backed by a detached or immutable ArrayBuffer. The copy is then a no-op rather than a write to read-only memory. The native binding now plumbs byte offsets and the copied-byte count through as size_t, passed across the fast/slow API boundary as doubles (exact for integer values below 2^53), so copies that cross the 4 GiB boundary are no longer truncated to 32 bits. buffer-copy.js vs node v26.3.0 (x64, 30 runs): partial=false bytes=1024: +7.91% (***) partial=false bytes=128: +0.17% partial=false bytes=8: -0.89% partial=true bytes=1024: +22.33% (***) partial=true bytes=128: +19.58% (***) partial=true bytes=8: +18.70% (***) This supersedes the prototype in nodejs#62491, which added a bespoke ArrayBufferView::FastCopy instead of using the upstream-friendly CopyArrayBufferBytes API. Adds SharedArrayBuffer and immutable-ArrayBuffer coverage to the buffer copy tests, plus a pummel regression test for copies larger than 2^32 bytes. Refs: nodejs#55422 Signed-off-by: Robert Nagy <ronagy@icloud.com> Assisted-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5cb3967 to
da189fb
Compare
For posterity, said CL was merged upstream earlier today. |
| @@ -279,13 +279,10 @@ function _copyActual(source, target, targetStart, sourceStart, sourceEnd, isUint | |||
| if (nb <= 0) | |||
| return 0; | |||
There was a problem hiding this comment.
Nit: Am I right in saying that CopyArrayBufferBytesImpl does its own clamping? If so, could we get rid of this wrapper entirely?
Commit Queue failed- Loading data for nodejs/node/pull/63828 ✔ Done loading data for nodejs/node/pull/63828 ----------------------------------- PR info ------------------------------------ Title buffer: optimize Buffer.prototype.copy (#63828) Author Robert Nagy <ronagy@icloud.com> (@ronag) Branch ronag:buffer-faster-copy -> nodejs:main Labels lib / src, author ready, needs-ci Commits 2 - deps: V8: add CopyArrayBufferBytes API - buffer: optimize Buffer.prototype.copy Committers 1 - Robert Nagy <ronagy@icloud.com> PR-URL: https://github.com/nodejs/node/pull/63828 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: René <contact.9a5d6388@renegade334.me.uk> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/63828 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: René <contact.9a5d6388@renegade334.me.uk> -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 10 Jun 2026 06:51:37 GMT ✔ Approvals: 2 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/63828#pullrequestreview-4467706416 ✔ - René (@Renegade334): https://github.com/nodejs/node/pull/63828#pullrequestreview-4479569028 ✘ GitHub CI failed with status: FAILURE ℹ Last Benchmark CI on 2026-06-10T07:02:49Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1868/ ℹ Last Full PR CI on 2026-06-11T18:53:12Z: https://ci.nodejs.org/job/node-test-pull-request/74062/ - Querying data for job/node-test-pull-request/74062/ ✔ Build data downloaded - Querying failures of job/node-test-commit/88614/ ✔ Data downloaded ✘ 1 failure(s) on the last Jenkins CI run -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/27400061810 |
Commit Queue failed- Loading data for nodejs/node/pull/63828 ✔ Done loading data for nodejs/node/pull/63828 ----------------------------------- PR info ------------------------------------ Title buffer: optimize Buffer.prototype.copy (#63828) Author Robert Nagy <ronagy@icloud.com> (@ronag) Branch ronag:buffer-faster-copy -> nodejs:main Labels lib / src, author ready, needs-ci, commit-queue-failed Commits 2 - deps: V8: add CopyArrayBufferBytes API - buffer: optimize Buffer.prototype.copy Committers 1 - Robert Nagy <ronagy@icloud.com> PR-URL: https://github.com/nodejs/node/pull/63828 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: René <contact.9a5d6388@renegade334.me.uk> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/63828 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: René <contact.9a5d6388@renegade334.me.uk> -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 10 Jun 2026 06:51:37 GMT ✔ Approvals: 2 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/63828#pullrequestreview-4467706416 ✔ - René (@Renegade334): https://github.com/nodejs/node/pull/63828#pullrequestreview-4479569028 ✔ Last GitHub CI successful ℹ Last Benchmark CI on 2026-06-12T06:59:29Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1868/ ℹ Last Full PR CI on 2026-06-12T07:04:01Z: https://ci.nodejs.org/job/node-test-pull-request/74069/ - Querying data for job/node-test-pull-request/74069/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 63828 From https://github.com/nodejs/node * branch refs/pull/63828/merge -> FETCH_HEAD ✔ Fetched commits as 144f79595670..da189fb122ca -------------------------------------------------------------------------------- [main b17b65061a] deps: V8: add CopyArrayBufferBytes API Author: Robert Nagy <ronagy@icloud.com> Date: Wed Jun 10 11:09:44 2026 +0200 4 files changed, 125 insertions(+), 1 deletion(-) [main 00629a26db] buffer: optimize Buffer.prototype.copy Author: Robert Nagy <ronagy@icloud.com> Date: Wed Jun 10 11:09:48 2026 +0200 5 files changed, 205 insertions(+), 35 deletions(-) create mode 100644 test/parallel/test-buffer-copy-immutable.js create mode 100644 test/pummel/test-buffer-large-size-copy.js ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. (node:358) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated. (Use `node --trace-deprecation ...` to show where the warning was created) Rebasing (2/4) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- deps: V8: add CopyArrayBufferBytes APIhttps://github.com/nodejs/node/actions/runs/27416075610 |
Route the native backing of Buffer.prototype.copy (CopyImpl, the
_copybinding) through V8's new v8::ArrayBuffer::CopyArrayBufferBytes API instead of materializing an ArrayBufferViewContents and doing a manual memmove. This speeds up partial copies (sourceStart > 0). All copies now go through this binding: the previous %TypedArray%.prototype.set fast-path for whole-buffer copies is dropped, since it would throw on a detached or immutable target rather than report a 0-byte no-op, and the benchmarks below show the native path is comparable for that case.V8 clamps the byte range and handles detached and immutable buffers. When both sides are backed by a SharedArrayBuffer the relaxed-atomic overload is used, which honors the SharedArrayBuffer memory model. Mixed copies (one SharedArrayBuffer, one ArrayBuffer) use the regular overload.
copy() now reports the number of bytes actually copied, as returned by V8: 0 when the target is backed by a detached or immutable ArrayBuffer. The copy is then a no-op rather than a write to read-only memory.
buffer-copy.js vs node v26.3.0 (x64, 30 runs):
partial=false bytes=1024: +7.91% ()
partial=false bytes=128: +0.17%
partial=false bytes=8: -0.89%
partial=true bytes=1024: +22.33% ()
partial=true bytes=128: +19.58% ()
partial=true bytes=8: +18.70% ()
The V8 API is carried as a floating patch cherry-picked from:
That CL is authored by a Node.js collaborator (Ben Noordhuis) and is expected to land upstream, so the floating patch should be easy to maintain: it touches nothing but the public ArrayBuffer API and can be dropped wholesale on the next V8 upgrade that includes it. v8_embedder_string is bumped to -node.21 accordingly.
This supersedes the prototype in
#62491, which added a bespoke ArrayBufferView::FastCopy instead of using the upstream-friendly CopyArrayBufferBytes API.
Adds SharedArrayBuffer and immutable-ArrayBuffer coverage to the buffer copy tests.
Assisted-by: Claude Opus 4.8 (1M context) noreply@anthropic.com