Skip to content

src: use native CopyArrayBufferBytes for webstream buffer copy - #66400

Open
Renegade334 wants to merge 1 commit into
nodejs:mainfrom
Renegade334:copyarraybuffer-native
Open

Renegade334 wants to merge 1 commit into
nodejs:mainfrom
Renegade334:copyarraybuffer-native

Conversation

@Renegade334

Copy link
Copy Markdown
Member

Use the V8 API path for these, which performs its own bounds handling.

I'm not seeing any performance delta, but we may as well not reinvent the wheel.

@Renegade334 Renegade334 added dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. labels Sep 29, 2026
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Sep 29, 2026
Comment thread src/node_buffer.cc
Comment on lines +1675 to +1677
CHECK(args[0]->IsArrayBuffer());
CHECK(args[1]->IsUint32());
CHECK(args[2]->IsArrayBuffer() || args[2]->IsSharedArrayBuffer());
CHECK(args[2]->IsArrayBuffer());

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

SharedArrayBuffers could never make it down this path, as canCopyArrayBuffer() uses ArrayBuffer primordials which throw on SharedArrayBuffers.

@codecov

codecov Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 56.00000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.37%. Comparing base (f71d644) to head (6b117f7).
⚠️ Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
src/node_buffer.cc 56.00% 8 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66400      +/-   ##
==========================================
+ Coverage   90.36%   90.37%   +0.01%     
==========================================
  Files         792      792              
  Lines      275564   275562       -2     
  Branches    52832    52829       -3     
==========================================
+ Hits       249016   249048      +32     
+ Misses      16965    16920      -45     
- Partials     9583     9594      +11     
Files with missing lines Coverage Δ
src/node_buffer.cc 70.43% <56.00%> (+0.03%) ⬆️

... and 30 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.

Signed-off-by: Renegade334 <contact.9a5d6388@renegade334.me.uk>
@Renegade334
Renegade334 force-pushed the copyarraybuffer-native branch from 5769480 to 6b117f7 Compare September 30, 2026 04:44
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 30, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 30, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Renegade334 Renegade334 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants