stream: move WHATWG byte-stream helpers to C++#63570
Open
mcollina wants to merge 1 commit into
Open
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #63570 +/- ##
==========================================
+ Coverage 90.32% 90.34% +0.01%
==========================================
Files 730 733 +3
Lines 234209 236495 +2286
Branches 43934 44542 +608
==========================================
+ Hits 211558 213652 +2094
- Misses 14372 14549 +177
- Partials 8279 8294 +15
🚀 New features to boost your workflow:
|
addaleax
approved these changes
May 26, 2026
| size_t from_byte_length = BufferByteLength(args[2]); | ||
|
|
||
| bool ok = static_cast<uint64_t>(to_index) + count <= to_byte_length && | ||
| static_cast<uint64_t>(from_index) + count <= from_byte_length; |
Member
There was a problem hiding this comment.
Should this guard against overflows?
github-actions
Bot
removed
the
request-ci
Collaborator
Renegade334
reviewed
May 26, 2026
Comment on lines
+25
to
+27
| arrayBufferViewGetBuffer: ArrayBufferViewGetBuffer, | ||
| arrayBufferViewGetByteLength: ArrayBufferViewGetByteLength, | ||
| arrayBufferViewGetByteOffset: ArrayBufferViewGetByteOffset, |
Member
There was a problem hiding this comment.
What would happen to performance if we made this a single getArrayBufferView that returned a { buffer, byteLength, byteOffset }? I'm just wondering if cutting the number of binding traversals would be cheaper, even if it does occasionally fetch unneeded properties.
Renegade334
reviewed
May 26, 2026
Comment on lines
+31
to
+36
| inline size_t BufferByteLength(Local<Value> buffer) { | ||
| if (buffer->IsArrayBuffer()) { | ||
| return buffer.As<ArrayBuffer>()->ByteLength(); | ||
| } | ||
| return buffer.As<SharedArrayBuffer>()->ByteLength(); | ||
| } |
Member
There was a problem hiding this comment.
FWIW, this isn't necessary: SAB handles are ArrayBuffer handles in V8, and calling sab.As<ArrayBuffer>()->ByteLength() is legal. The V8 API has this behaviour baked in (calling abv->Buffer() on a SAB-backed view returns the SAB as an ArrayBuffer handle), and we already rely on this behaviour ourselves (when passing a SAB-backed view to ArrayBufferViewContents).
Comment on lines
+23
to
+29
| inline bool BufferIsDetached(Local<Value> buffer) { | ||
| if (buffer->IsArrayBuffer()) { | ||
| return buffer.As<ArrayBuffer>()->WasDetached(); | ||
| } | ||
| // SharedArrayBuffers cannot be detached. | ||
| return false; | ||
| } |
Member
There was a problem hiding this comment.
This also doesn't need an AB/SAB typeguard, sab.As<ArrayBuffer>().WasDetached() is legal, and will always return false.
Renegade334
reviewed
May 26, 2026
| CHECK(args[0]->IsArrayBufferView()); | ||
| Local<ArrayBufferView> view = args[0].As<ArrayBufferView>(); | ||
| size_t byte_length = view->ByteLength(); | ||
| Local<ArrayBuffer> new_buffer = ArrayBuffer::New(isolate, byte_length); |
Member
There was a problem hiding this comment.
This should probably use MaybeNew(), otherwise it'll crash the process on OOM.
anonrig
approved these changes
May 31, 2026
Implement three defensive helpers from lib/internal/webstreams/util.js
in a new internal binding (src/node_webstreams.cc):
* getArrayBufferView
* canCopyArrayBuffer
* cloneAsUint8Array
The previous JavaScript versions used Reflect.get on
view.constructor.prototype and called ArrayBuffer.prototype methods
through primordials so they would survive prototype tampering. The C++
versions use the V8 ArrayBufferView and ArrayBuffer APIs directly,
preserving the same robustness without the JS-side overhead.
getArrayBufferView returns { buffer, byteOffset, byteLength } in a
single binding crossing, replacing three separate accessors.
cloneAsUint8Array uses ArrayBuffer::MaybeNew so the process is not
killed on allocation failure.
These functions sit on the byte-stream hot paths
(ReadableByteStreamController enqueue/read, pull-into descriptor copy,
tee clones). ReadableStream type='bytes' throughput on
benchmark/webstreams/readable-read.js improves by ~15% on the BYOB
read path on my workstation.
WPT streams parity is preserved (1403 subtests passing, 0 unexpected
failures, identical to baseline).
Signed-off-by: Matteo Collina <hello@matteocollina.com>
mcollina
force-pushed
the
stream/webstreams-cpp-helpers
branch
from
a71a3f7 to
e2c7b65
Compare
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
5 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.
Implement five defensive helpers from
lib/internal/webstreams/util.jsin a new internal binding (src/node_webstreams.cc) usingv8::ArrayBufferView/v8::ArrayBufferAPIs directly:arrayBufferViewGet{Buffer,ByteLength,ByteOffset}canCopyArrayBuffercloneAsUint8ArrayThe previous JS versions used
Reflect.getagainstview.constructor.prototypeandArrayBuffer.prototype.{slice,getDetached,getByteLength}via primordials to survive prototype tampering. The C++ versions preserve the same defensive semantics without the JS-side overhead.benchmark/webstreams/readable-read.js type=bytes(BYOB read path, which exercises these helpers on every chunk) improves by ~15% on my workstation. WPT streams parity preserved: 1403 subtests passing, 0 unexpected failures.