stream: move WHATWG byte-stream helpers to C++#63570
Open
mcollina wants to merge 1 commit into
Open
Conversation
Implement five defensive helpers from lib/internal/webstreams/util.js in a new internal binding (src/node_webstreams.cc): * arrayBufferViewGetBuffer * arrayBufferViewGetByteLength * arrayBufferViewGetByteOffset * 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. 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).
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.33%
========================================
Files 730 731 +1
Lines 234209 234514 +305
Branches 43934 43922 -12
========================================
+ Hits 211558 211850 +292
- Misses 14372 14376 +4
- Partials 8279 8288 +9
🚀 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.
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
4 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.