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.

Signed-off-by: Renegade334 <contact.9a5d6388@renegade334.me.uk>
@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.38%. Comparing base (e6b3f96) to head (5769480).
⚠️ Report is 5 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.38%   90.38%   -0.01%     
==========================================
  Files         792      792              
  Lines      275517   275553      +36     
  Branches    52813    52816       +3     
==========================================
+ Hits       249017   249049      +32     
- Misses      16912    16919       +7     
+ Partials     9588     9585       -3     
Files with missing lines Coverage Δ
src/node_buffer.cc 70.43% <56.00%> (-0.05%) ⬇️

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

2 participants