Allow all ArrayBufferView types as Buffers by TimothyGu · Pull Request #12223 · nodejs/node · GitHub

Right now, many modules allow the use of plain Uint8Arrays in addition to Buffers. This PR begins the work to expand such support to all ArrayBufferView types (including all TypedArray types and DataView).

This work was inspired by #1826, which requests support for ArrayBuffer as well. But ArrayBuffer 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 new isArrayBufferView utility function and update usage of buf.length to buf.byteLength).

I don't intend this PR to make its way into 8.0.0.

Relevant discussions on IRC with @addaleax

2017-03-23 20:20:54 TimothyGu  So I was looking at https://github.com/nodejs/node/issues/1826
2017-03-23 20:21:23 TimothyGu  As we are trying to support Uint8Array in C++ binding it doesn't
                               look much more work to support all the other ArrayBufferView types
2017-03-23 20:21:40 TimothyGu  including all TypedArray and DataView
2017-03-23 20:21:54 addaleax   right … that was also pointed out to me in the last streams wg
                               meeting
2017-03-23 20:22:32 addaleax   I understand where it’s coming from, but so far I have wanted to
                               keep the changes pretty minimal
2017-03-23 20:23:06 addaleax   bc going from Buffer support to Uint8Array support is, for the most
                               part, a tiny change
2017-03-23 20:23:27 addaleax   if somebody (you?) wants to work on this, yay
2017-03-23 20:23:40 TimothyGu  TBH it doesn't take much of a change on the C++ side to get it
                               working
2017-03-23 20:23:46 TimothyGu  http://sprunge.us/SgET
2017-03-23 20:23:54 TimothyGu  or http://sprunge.us/SgET?diff
2017-03-23 20:25:12 TimothyGu  so I thought maybe we could just support all of them at the same
                               time
2017-03-23 20:25:25 addaleax   that’s nice, yes
...
2017-03-23 20:28:20 addaleax   re: generic ArrayBufferView support … the one reason I’ve not been
                               doing that is that my personal goal is to give users a more
                               standard way of passing in binary data, and Uint8Arrays work just
                               fine for that purpose. the other reason is that there is at least a
                               small bit of ambiguity, because interpreting e.g. a Uint16Array as
                               a Uint8Array can be done in two ways (entry-for-entry or via the...
2017-03-23 20:28:21 addaleax   ...underlying storage)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

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?

@mscdex

Web standards usually use Web IDL as an abstraction for type conversions and other common tasks potentially used by many standards. In addition to ArrayBuffer, all TypedArray types, and DataView, Web IDL specifies two additional types for abstraction of buffer sources:

ArrayBufferView which includes all TypedArray types and DataView
BufferSource which includes ArrayBuffer and ArrayBufferView

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 ArrayBufferView or BufferSource Web IDL type, i.e. almost all Web APIs that take a generic byte buffer, support all TypedArray types.

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:

ImageData (HTML); the explicit use of Uint8ClampedArray as a hint for 8-bit packed RGBA pixel format is necessary in light of a multitude of incompatible pixel formats
AudioBuffer.getChannelData (Web Audio API); the explicit use of Float32Array as a hint for "non-interleaved 32-bit linear floating-point PCM values with a normal range of [−1,1]" is necessary in light of a multitude of incompatible audio sample formats

On the other hand, I'm not familiar with any examples in the Node.js ecosystem, considering Buffer has been the standard for byte buffers since Node.js' inception.

/cc @domenic

Two notable exceptions I found:

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.

On the other hand, I'm not familiar with any examples in the Node.js ecosystem, considering Buffer has been the standard for byte buffers since Node.js' inception.

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 Uint8Array to ArrayBufferView), both in JS and in C++.

Isn't this equivalent to ArrayBuffer.isView()?

Isn't this equivalent to ArrayBuffer.isView()?

@lpinca yep, those do the same thing.

Yes, I think the standard version should be better in this case.

Is this WIP? There are a few more places in the API where currently only Uint8Arrays are supported (or at least it’s documented that way).

Anyway, I think I’m +1 on this change… @nodejs/collaborators Thoughts?

Isn't this equivalent to ArrayBuffer.isView()?

@lpinca yep, those do the same thing.

chunk is engine output, so it’s always a Buffer, right?

I think so, but I'd rather err on the side of replacing too many Buffer.lengths rather than missing an ArrayBufferView.byteLength, since Buffer.length === Buffer.byteLength.

Did you mean byteLength % BYTES_PER_ELEMENT === 0, or Number.isInteger(byteLength / BYTES_PER_ELEMENT)?

Uhh… that's embarrassing. Good catch.

LGTM as it is. I think it make sense to support JS types for binary data wherever possible.

@addaleax

Is this WIP? There are a few more places in the API where currently only Uint8Arrays are supported (or at least it’s documented that way).

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 Uint8Arrays was incrementally added. The crypto and zlib modules in this PR serve as examples for how it's done.

@addaleax @lpinca Comments addressed.

CI: https://ci.nodejs.org/job/node-test-pull-request/7231/

It looks to me like this PR does not take into account byteOffset. Is that correct? If so that seems pretty bad.

I think you need adapter code like https://github.com/tmpvar/jsdom/blob/28d08f58b82cc2100ad36b99cf5e8b4bbb3fc291/lib/api.js#L329-L333

It looks to me like this PR does not take into account byteOffset. Is that correct? If so that seems pretty bad.

@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 byteOffset to the pointer that actually ends up used (both with and without this PR), so I see no reason for concern here.

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>

Refs: nodejs#12223

PR-URL: #18651 Refs: #12223 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

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>

TimothyGu added 4 commits April 4, 2017 16:09

src: allow ArrayBufferView as instance of Buffer

a8abe9c

test: add common.getArrayBufferViews(buf)

dae94ff

crypto: support all ArrayBufferView types

c58b945

zlib: support all ArrayBufferView types

07a399b

mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 5, 2017

lpinca reviewed Apr 5, 2017

View reviewed changes

addaleax reviewed Apr 5, 2017

View reviewed changes

mcollina approved these changes Apr 5, 2017

View reviewed changes

TimothyGu added 4 commits April 5, 2017 08:38

fixup crypto

1459f27

fixup zlib

039208e

fixup test

072e898

fixup util

1315240

jasnell approved these changes Apr 11, 2017

View reviewed changes

addaleax approved these changes Apr 11, 2017

View reviewed changes

TimothyGu closed this Apr 12, 2017

TimothyGu deleted the typedarray branch April 12, 2017 17:09

TimothyGu added a commit that referenced this pull request Apr 12, 2017

src: allow ArrayBufferView as instance of Buffer

faa447b

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>

TimothyGu added a commit that referenced this pull request Apr 12, 2017

test: add common.getArrayBufferViews(buf)

0c539fa

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>

TimothyGu added a commit that referenced this pull request Apr 12, 2017

crypto: support all ArrayBufferView types

a8f460f

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>

TimothyGu added a commit that referenced this pull request Apr 12, 2017

zlib: support all ArrayBufferView types

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>

TimothyGu mentioned this pull request Apr 14, 2017

string_decoder: support Uint8Array input to methods #11613

Closed

4 tasks

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

TimothyGu mentioned this pull request Jul 6, 2017

Accept ArrayBuffer (and typed array/data view?) anywhere Buffer is allowed in the API #1826

Closed

tniessen added a commit to tniessen/node that referenced this pull request Feb 8, 2018

doc: fix description of createDecipheriv

9ebf7bd

Refs: nodejs#12223

tniessen mentioned this pull request Feb 8, 2018

crypto: fix meta and description #18651

Closed

2 tasks

               const stream = require('stream');
               const util = require('util');
-              const { isUint8Array } = process.binding('util');
+              const { isArrayBufferView } = process.binding('util');

               const stream = require('stream');
               const util = require('util');
-              const { isUint8Array } = process.binding('util');
+              const { isArrayBufferView } = process.binding('util');

                   while (null !== (chunk = engine.read())) {
                     buffers.push(chunk);
-                    nread += chunk.length;
+                    nread += chunk.byteLength;

+                const out = [];
+                for (const type of arrayBufferViews) {
+                  const { BYTES_PER_ELEMENT = 1 } = type;
+                  if (Number.isInteger(byteLength % BYTES_PER_ELEMENT)) {

Allow all ArrayBufferView types as Buffers by TimothyGu · Pull Request #12223 · nodejs/node · GitHub

Uh oh!

Uh oh!

Allow all ArrayBufferView types as Buffers - #12223

Allow all ArrayBufferView types as Buffers#12223TimothyGu wants to merge 8 commits intonodejs:masternodejs/node:masterfrom TimothyGu:typedarrayTimothyGu/node:typedarrayCopy head branch name to clipboard

Conversation

TimothyGu commented Apr 5, 2017

Checklist

Affected core subsystem(s)

Uh oh!

mscdex commented Apr 5, 2017

Uh oh!

TimothyGu commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domenic commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Apr 5, 2017

Uh oh!

TimothyGu commented Apr 5, 2017

Uh oh!

lpinca Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Apr 5, 2017

Uh oh!

TimothyGu commented Apr 5, 2017

Uh oh!

domenic commented Apr 11, 2017

Uh oh!

addaleax commented Apr 11, 2017

Uh oh!

domenic commented Apr 11, 2017

Uh oh!

addaleax commented Apr 11, 2017

Uh oh!

domenic commented Apr 11, 2017

Uh oh!

TimothyGu commented Apr 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Allow all ArrayBufferView types as Buffers#12223
TimothyGu wants to merge 8 commits into
nodejs:masterfrom
TimothyGu:typedarray

TimothyGu commented Apr 5, 2017 •
edited
Loading

domenic commented Apr 5, 2017 •
edited
Loading

addaleax Apr 5, 2017 •
edited
Loading

addaleax Apr 5, 2017 •
edited
Loading

mcollina left a comment •
edited
Loading