stream: move WHATWG byte-stream helpers to C++#63570
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).
|
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:
|
| 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; |
There was a problem hiding this comment.
Should this guard against overflows?
github-actions
Bot
removed
the
request-ci
| arrayBufferViewGetBuffer: ArrayBufferViewGetBuffer, | ||
| arrayBufferViewGetByteLength: ArrayBufferViewGetByteLength, | ||
| arrayBufferViewGetByteOffset: ArrayBufferViewGetByteOffset, |
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.
| inline size_t BufferByteLength(Local<Value> buffer) { | ||
| if (buffer->IsArrayBuffer()) { | ||
| return buffer.As<ArrayBuffer>()->ByteLength(); | ||
| } | ||
| return buffer.As<SharedArrayBuffer>()->ByteLength(); | ||
| } |
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).
| inline bool BufferIsDetached(Local<Value> buffer) { | ||
| if (buffer->IsArrayBuffer()) { | ||
| return buffer.As<ArrayBuffer>()->WasDetached(); | ||
| } | ||
| // SharedArrayBuffers cannot be detached. | ||
| return false; | ||
| } |
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.
| 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); |
There was a problem hiding this comment.
This should probably use MaybeNew(), otherwise it'll crash the process on OOM.
4 participants
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.