-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow all ArrayBufferView types as Buffers #12223
Conversation
I agree with @addaleax about the possible ambiguity with how to retrieve the byte values (by index or by byte of the underlying storage). That makes me lean towards -1 on this change. Do we know if there are other libraries (node or browser) who allow these other non-8-bit typed arrays to be used as 8-bit arrays? It might be good to see if there is a precedent for how we might handle it in node? |
Web standards usually use Web IDL as an abstraction for type conversions and other common tasks potentially used by many standards. In addition to
Additionally, Web IDL defines the abstract operations getting a reference to the bytes held by a buffer source and getting a copy of the bytes held by a buffer source, which are what the Web Standards actually call upon to perform the task of getting the bytes. They both retrieve the byte values by underlying storage, not by index. This effectively means that all Web APIs using Examples include:
At the same time, not all Web APIs accept all TypedArray types, but this always happens for a good reason. Two notable exceptions I found:
On the other hand, I'm not familiar with any examples in the Node.js ecosystem, considering /cc @domenic |
Another exception is in readable byte streams (e.g. fetch body streams). This is because we need not only bytes, but also an offset and a length (for reasons I'll leave out here but can explain if desired). And look, we already have a type for that: Uint8Array = { buffer: ArrayBuffer, byteOffset, byteLength }. So we are using Uint8Array for streams. But this is basically a supporting argument: we only restrict to Uint8Array when we also find the byteOffset and byteLength into the larger ArrayBuffer to be meaningful. Otherwise, any form of binary data is acceptable. And indeed, as @TimothyGu pointed out, when accepting data in stream's BYOB-read() method, we accept any type.
Well, jsdom's upcoming v10 will accept any of the BufferSource types, or Buffer. |
@TimothyGu Have you compared at least the Buffer benchmarks to see what (if any) performance difference there is before and after these changes? |
@mscdex there are no differences in zlib. I did not test crypto, but I don't expect there to be any performance differences since the code is practically identical (except for changing |
lib/crypto.js
Outdated
@@ -42,7 +42,7 @@ const timingSafeEqual = binding.timingSafeEqual; | |||
const Buffer = require('buffer').Buffer; | |||
const stream = require('stream'); | |||
const util = require('util'); | |||
const { isUint8Array } = process.binding('util'); | |||
const { isArrayBufferView } = process.binding('util'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this equivalent to ArrayBuffer.isView()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this equivalent to
ArrayBuffer.isView()
?
@lpinca yep, those do the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think the standard version should be better in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this WIP? There are a few more places in the API where currently only Uint8Array
s are supported (or at least it’s documented that way).
Anyway, I think I’m +1 on this change… @nodejs/collaborators Thoughts?
lib/crypto.js
Outdated
@@ -42,7 +42,7 @@ const timingSafeEqual = binding.timingSafeEqual; | |||
const Buffer = require('buffer').Buffer; | |||
const stream = require('stream'); | |||
const util = require('util'); | |||
const { isUint8Array } = process.binding('util'); | |||
const { isArrayBufferView } = process.binding('util'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this equivalent to
ArrayBuffer.isView()
?
@lpinca yep, those do the same thing.
@@ -99,7 +99,7 @@ function zlibBuffer(engine, buffer, callback) { | |||
var chunk; | |||
while (null !== (chunk = engine.read())) { | |||
buffers.push(chunk); | |||
nread += chunk.length; | |||
nread += chunk.byteLength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chunk
is engine output, so it’s always a Buffer
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, but I'd rather err on the side of replacing too many Buffer.length
s rather than missing an ArrayBufferView.byteLength
, since Buffer.length === Buffer.byteLength
.
test/common.js
Outdated
const out = []; | ||
for (const type of arrayBufferViews) { | ||
const { BYTES_PER_ELEMENT = 1 } = type; | ||
if (Number.isInteger(byteLength % BYTES_PER_ELEMENT)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean byteLength % BYTES_PER_ELEMENT === 0
, or Number.isInteger(byteLength / BYTES_PER_ELEMENT)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhh… that's embarrassing. Good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as it is. I think it make sense to support JS types for binary data wherever possible.
Somewhat. My intention was to incrementally update individual modules, instead of changing all the modules at the same time – sort of like how the support for |
It looks to me like this PR does not take into account I think you need adapter code like https://github.com/tmpvar/jsdom/blob/28d08f58b82cc2100ad36b99cf5e8b4bbb3fc291/lib/api.js#L329-L333 |
@domenic Can you point to the code that you think doesn’t account for the offset? It does look like that’s handled correctly everywhere. |
Well, for example, https://github.com/TimothyGu/node/blob/13152405dd8f248192fea558a9bc206201f884f1/lib/crypto.js does not contain the string "byteOffset" anywhere. |
@domenic For most of the APIs that use C++, including the crypto ones, the ABV is unwrapped in https://github.com/nodejs/node/pull/12223/files#diff-772f489c7d0a32de3badbfbcb5fd200dR441. That does add the |
Great, that is what I was missing. Thank you! |
Landed in ec53921...2ced07c. |
PR-URL: #12223 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #12223 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #12223 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #12223 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#18651 Refs: nodejs#12223 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Right now, many modules allow the use of plain
Uint8Array
s in addition toBuffer
s. This PR begins the work to expand such support to allArrayBufferView
types (including allTypedArray
types andDataView
).This work was inspired by #1826, which requests support for
ArrayBuffer
as well. ButArrayBuffer
is treated separately from its views in V8's API and it is easier to get started with the work here.The first two commits add the necessary infrastructure for consuming and testing all
ArrayBufferView
types. The last two demonstrate how to update the JS layer for it. (Specifically, using the newisArrayBufferView
utility function and update usage ofbuf.length
tobuf.byteLength
).I don't intend this PR to make its way into 8.0.0.
Relevant discussions on IRC with @addaleax
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)