stream: support Uint8Array input to methods #11608

addaleax · 2017-02-28T15:34:52Z

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)

streams

$ ./node benchmark/compare.js --new ./node --old ./node-d08836003c57 --runs 5 streams | Rscript benchmark/compare.R
[00:10:00|% 100| 6/6 files | 10/10 runs | 1/1 configs]: Done
                                          improvement confidence      p.value
 streams/readable-bigread.js n=1000           20.95 %        *** 8.399686e-07
 streams/readable-bigunevenread.js n=1000     18.08 %        *** 1.672017e-06
 streams/readable-boundaryread.js n=2000      15.66 %         ** 1.908462e-03
 streams/readable-readall.js n=5000           42.03 %        *** 1.686883e-08
 streams/readable-unevenread.js n=1000         2.66 %            3.173964e-01
 streams/writable-manywrites.js n=2000000     13.60 %        *** 4.770480e-05

(Yes, those are improvements, not losses… I find it odd that instanceof is so heavy compared to isUint8Array, but this is what the benchmarks say, and I won’t complain. d088360 is the master head at the time of writing.)

@nodejs/streams

mscdex · 2017-02-28T15:41:02Z

lib/stream.js

+}
+
+const version = process.version.substr(1).split('.');
+if (version[0] <= 0 && version[1] < 12) {


When would the major version be less than zero?

Also, why are we adding an explicit check for such old versions of node (v0.10 and older) if they're no longer supported?

Because readable-stream still supports them and removing that support would at least be a breaking change…

When would the major version be less than zero?

Never, I can update this to ===

Ugh... I can't help but think readable-stream should polyfill this instead of node core...

If @nodejs/streams is willing to do that I’m all for it.

mscdex · 2017-02-28T15:42:35Z

doc/api/stream.md

  - version: v6.0.0
    pr-url: https://github.com/nodejs/node/pull/6170
    description: Passing `null` as the `chunk` parameter will always be
                 considered invalid now, even in object mode.
 -->

-* `chunk` {String|Buffer} The data to write
+* `chunk` {String|Buffer|Uint8Array} The data to write


A bit unrelated, but this doesn't match the chunk description of .end() which is confusing, specifically the 'any' part.

mscdex · 2017-02-28T15:43:23Z

doc/api/stream.md

 -->

-* `chunk` {Buffer|String} Chunk of data to unshift onto the read queue
+* `chunk` {Buffer|Uint8Array|String} Chunk of data to unshift onto the


Ditto here about the chunk description and missing 'any' I think.

mscdex · 2017-02-28T15:44:08Z

doc/api/stream.md


-* `chunk` {Buffer|Null|String} Chunk of data to push into the read queue
+* `chunk` {Buffer|Uint8Array|Null|String} Chunk of data to push into


Another unrelated nit: it seems like 'Null' should be all lowercase since there is no such constructor.

By that logic String should also be lowercase since it doesn't mean the String constructor.

@seishun I agree, but I recall others possibly arguing for the uppercase version in the past? There might need to be changes to the doc tool...

addaleax · 2017-02-28T16:02:19Z

@mscdex addressed your comments, PTAL

addaleax · 2017-02-28T16:55:08Z

@seishun right… should’ve seen that. done!

addaleax · 2017-02-28T17:02:08Z

CI: https://ci.nodejs.org/job/node-test-commit/8172/

jasnell · 2017-02-28T20:21:31Z

Overall this LGTM but I'd definitely prefer more attention and signoff from @nodejs/streams folks

joyeecheung · 2017-03-01T03:13:21Z

lib/stream.js

+
+// Internal utilities
+try {
+  Stream._isUint8Array = process.binding('util').isUint8Array;


Maybe move this into internal/util?

@joyeecheung The thing is that the streams modules can’t really unconditionally rely on internal modules because they are also exported as https://github.com/nodejs/readable-stream. @mscdex expressed a preference for having this code polyfilled on the readable-stream side and I agree, but ultimately that’s @nodejs/streams’ call …

(Please let me know if I’m misunderstanding you!)

Oh yes, forgot about the vendoring out part :) Nevermind then!

mcollina · 2017-03-01T19:21:59Z

Before LGTMing this I would like to consider if we have alternative fast approaches here to avoid instanceof, maybe @fhinkel can check things out.

I'm not 100% on board with accepting UInt8Array as input, as it changes the behavior of when decoding is not enabled. Also, there are plenty of objectMode streams in the community leveraging Buffer objects. If we go ahead with this, it is semver-major for me (also in readable-stream).

Regarding readable-stream compatibility, I am afraid that Object.getPrototypeOf(chunk) !== Buffer.prototype might cause issues and we would likely have to rewrite this line. Plus, Buffer.prototype.slice.call(chunk) is likely to cause perf regressions on old node versions.
A safer approach might be to completely swap out UInt8Array support in readable-stream. @calvinmetcalf what do you think?

addaleax · 2017-03-01T20:31:16Z

I would like to consider if we have alternative fast approaches here to avoid instanceof

What are the semantics that you would like to see in such an alternative?

I'm not 100% on board with accepting UInt8Array as input, as it changes the behavior of when decoding is not enabled.

Could you give an example for that? I tried to specifically avoid that.

Also, there are plenty of objectMode streams in the community leveraging Buffer objects.

Why would they be affected by this change?

I am afraid that Object.getPrototypeOf(chunk) !== Buffer.prototype might cause issues and we would likely have to rewrite this line.

What does readable-stream use as Buffers in non-Node environments?

Plus, Buffer.prototype.slice.call(chunk) is likely to cause perf regressions on old node versions.

This only hits when the input is not already a Buffer, so I’m not sure why this would be relevant.

A safer approach might be to completely swap out UInt8Array support in readable-stream

Does that mean skipping this PR for readable-stream in some way?

mcollina · 2017-03-03T02:55:46Z

I would like to consider if we have alternative fast approaches here to avoid instanceof

What are the semantics that you would like to see in such an alternative?

To some extent, I do not care. I care about the speed improvement this brings without breaking any existing code, plus readable-stream.
Also, it should be something we can easily polyfill for older nodes, i.e. switch back to instanceof.

I'm not 100% on board with accepting UInt8Array as input, as it changes the behavior of when decoding is not enabled.

Could you give an example for that? I tried to specifically avoid that.

Here is the difference:

class MyTransform {

  constructor(opts) {
    super(opts)
  }

  transform(chunk, enc, cb) {
    // chunk can be both a UInt8Array or a Buffer
    // because this stream can be set in objectMode or not
    cb()
  })
}

I know it might be a very narrow case, but there is a lot of code where objectMode is a passed-through option.

Plus, Buffer.prototype.slice.call(chunk) is likely to cause perf regressions on old node versions.

This only hits when the input is not already a Buffer, so I’m not sure why this would be relevant.

What about

  return Object.prototype.toString.call(obj) === '[object Uint8Array]';

What does readable-stream use as Buffers in non-Node environments?

https://github.com/feross/buffer

A safer approach might be to completely swap out UInt8Array support in readable-stream

Does that mean skipping this PR for readable-stream in some way?

Yes

addaleax · 2017-03-03T08:20:16Z

Here is the difference:

Thanks for pointing that out, that was a bug in this PR and not intentional. Fixed!

Plus, Buffer.prototype.slice.call(chunk) is likely to cause perf regressions on old node versions.

This only hits when the input is not already a Buffer, so I’m not sure why this would be relevant.

What about

 return Object.prototype.toString.call(obj) === '[object Uint8Array]';

The _uint8ArrayToBuffer call is still guarded by Object.getPrototypeOf(chunk) !== Buffer.prototype

What does readable-stream use as Buffers in non-Node environments?

https://github.com/feross/buffer

Right, this might indeed get tricky for platforms that don’t have Uint8Array… I’ll think about that.

fhinkel · 2017-03-03T10:56:14Z

Looking into the instanceof vs Stream._isUint8Array performance difference.

addaleax · 2017-03-05T12:57:25Z

Just read @joyeecheung’s comment in #11690 (review) and checked the benchmarks on canary instead of master:

$ ./node benchmark/compare.js --new ./node --old ./node-ab7a25b93edb --runs 5 streams | Rscript benchmark/compare.R
[00:07:42|% 100| 6/6 files | 10/10 runs | 1/1 configs]: Done
                                          improvement confidence      p.value
 streams/readable-bigread.js n=1000            5.69 %        *** 2.415267e-04
 streams/readable-bigunevenread.js n=1000      6.76 %        *** 5.907977e-05
 streams/readable-boundaryread.js n=2000       4.13 %            7.522906e-02
 streams/readable-readall.js n=5000            5.27 %        *** 1.201201e-04
 streams/readable-unevenread.js n=1000         0.95 %            3.617545e-01
 streams/writable-manywrites.js n=2000000     -0.39 %            6.255211e-01

So this would still seem okay from a performance viewpoint on V8 5.8.

But anyway, I’m not proposing this change because of performance but because I think Uint8Array support is actually valuable for Node’s API.

mcollina · 2017-03-05T17:45:04Z

@addaleax So, I'd like to have the speed boost in core, but on the other side I am not 100% in with the support of UInt8Array.

What's the use case to support UInt8Array? Most users leverage .pipe(), and after the first pass in write() the chunk is converted into a Buffer anyway.

IMHO, we should expose _uint8ArrayToBuffer as it seems to be the fastest way to do this.

addaleax · 2017-03-06T00:33:14Z

@mcollina So … for the other modules where I’m doing this, it makes sense because:

There’s not really any reason not to support generic Uint8Arrays
The isUint8Array check is the accurate check when it comes to what the C++ bindings actually need
This kind of API change has been suggested in buffer: discuss future direction of Buffer constructor API #9531 and I don’t think that’s a silly approach; if Node didn’t already have Buffer as part of its API it wouldn’t introduce it now and just use typed arrays instead
And finally the small performance gain by using the C++ check instead of letting instanceof iterate over the prototype chain.

I realize that these things don’t apply to streams the same way they do for other parts of the API, but I’d still say that there’s no real reason not to support Uint8Arrays.

Maybe other @nodejs/collaborators have some thoughts to offer on whether we should do this, or a general 👍/👎 feeling?

IMHO, we should expose _uint8ArrayToBuffer as it seems to be the fastest way to do this.

We could expose FastBuffer directly if we really want to; otherwise that’s basically already exposed as Buffer.prototype.slice().

fhinkel · 2017-03-06T10:17:07Z

Not really part of the discussion anymore, just fyi, I looked the into performance of instance of vs Stream._isUint8Array. instance of is doing an (EcmaScript spec compliant) instance of check, which is quite involved, even if optimized. _isUint8Array calls a V8 API function v8::IsUint8Array, which is a quick cast and simple check internally. So the benchmark results look reasonable.

mcollina · 2017-03-06T16:08:32Z

@addaleax all the streams API speaks buffers. Specially the _read, _write and _transform method, will keep supporting only Buffer for the time being. I think adding support for UInt8Array for some of the API will just make things more confusing for the end user.

I think we should talk about this with the rest of @nodejs/streams.

addaleax · 2017-03-06T16:56:52Z

I think adding support for UInt8Array for some of the API will just make things more confusing for the end user.

Right – that’s kind of my point. 😄 It makes sense for the rest of Node core’s API, therefore it would be consistent and less confusing if that was the same for streams.

Specially the _read, _write and _transform method, will keep supporting only Buffer for the time being.

Yeah, of course, all the APIs that give back Buffers to user code will need to keep doing so. To me this means that they already support Uint8Arrays, though? Just not very explicitly…

I think we should talk about this with the rest of @nodejs/streams.

Yes! :)

mcollina · 2017-03-06T18:32:13Z

I think adding support for UInt8Array for some of the API will just make things more confusing for the end user.

Right – that’s kind of my point. 😄 It makes sense for the rest of Node core’s API, therefore it would be consistent and less confusing if that was the same for streams.

So it might make sense from core's point of view, but not on require('streams') alone. At least we framed the problem.

Specially the _read, _write and _transform method, will keep supporting only Buffer for the time being.

Yeah, of course, all the APIs that give back Buffers to user code will need to keep doing so. To me this means that they already support Uint8Arrays, though? Just not very explicitly…

In Node.js, yes. However the whole streams ecosystem does not rely on Buffer inheriting from UInt8Array, see https://github.com/feross/buffer/blob/master/index.js#L21-L35.

mcollina

LGTM

mcollina · 2017-05-19T08:39:24Z

@addaleax this was approved during the WG meeting in Berlin! Would you mind updating the conflicting doc and land this? :)

mcollina · 2017-05-26T18:02:08Z

@addaleax can you rebase? I think we can release this.

addaleax · 2017-05-26T21:28:47Z

Rebased.

CI: https://ci.nodejs.org/job/node-test-commit/10177/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/838/

addaleax · 2017-05-27T09:20:31Z

Landed in 9f610b5

PR-URL: #11608 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

mcollina · 2017-05-27T09:21:49Z

Fantastic!

PR-URL: #11608 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax added buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem. labels Feb 28, 2017

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. stream Issues and PRs related to the stream subsystem. labels Feb 28, 2017

addaleax force-pushed the stream-uint8array branch from eb79285 to bbdafd7 Compare February 28, 2017 15:40

mscdex reviewed Feb 28, 2017

View reviewed changes

addaleax force-pushed the stream-uint8array branch from fe74aee to 8c68763 Compare February 28, 2017 17:00

jasnell requested a review from mcollina February 28, 2017 20:20

joyeecheung reviewed Mar 1, 2017

View reviewed changes

mcollina mentioned this pull request Mar 3, 2017

March 2017 WG meeting nodejs/readable-stream#261

Closed

6 tasks

addaleax force-pushed the stream-uint8array branch 2 times, most recently from b282929 to 5105ec0 Compare March 3, 2017 08:19

addaleax force-pushed the stream-uint8array branch from 5105ec0 to 0e7fb4b Compare March 5, 2017 12:51

mcollina mentioned this pull request Mar 17, 2017

Fixed 'Invalid non-string/buffer chunk' error when using Uint8Array nodejs/readable-stream#266

Closed

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

mcollina mentioned this pull request May 3, 2017

May 2017 wg meeting: in-person @ Collaborator Summit nodejs/readable-stream#275

Closed

mcollina approved these changes May 19, 2017

View reviewed changes

mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label May 22, 2017

addaleax force-pushed the stream-uint8array branch from 0e7fb4b to ece3234 Compare May 26, 2017 21:14

addaleax added 2 commits May 26, 2017 23:20

stream: support Uint8Array input to methods

5692736

[squash] apply fix to BufferList

6573670

addaleax force-pushed the stream-uint8array branch from ece3234 to 6573670 Compare May 26, 2017 21:21

addaleax closed this May 27, 2017

addaleax deleted the stream-uint8array branch May 27, 2017 09:20

addaleax added a commit that referenced this pull request May 27, 2017

stream: support Uint8Array input to methods

9f610b5

PR-URL: #11608 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell pushed a commit that referenced this pull request May 28, 2017

stream: support Uint8Array input to methods

220186c

PR-URL: #11608 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

MylesBorins mentioned this pull request Jan 15, 2018

Semver Minor Tracking nodejs/Release#298

Closed

addaleax mentioned this pull request Apr 8, 2018

stream: use instanceof when checking chunk type #19872

Closed

2 tasks

SirR4T mentioned this pull request Sep 4, 2018

stream: allow typed arrays to be written and read #22427

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: support Uint8Array input to methods #11608

stream: support Uint8Array input to methods #11608

addaleax commented Feb 28, 2017

mscdex Feb 28, 2017

addaleax Feb 28, 2017

mscdex Feb 28, 2017

addaleax Feb 28, 2017

mscdex Feb 28, 2017 •

edited

Loading

mscdex Feb 28, 2017

mscdex Feb 28, 2017

seishun Feb 28, 2017

mscdex Feb 28, 2017

addaleax commented Feb 28, 2017

addaleax commented Feb 28, 2017

addaleax commented Feb 28, 2017

jasnell commented Feb 28, 2017

joyeecheung Mar 1, 2017

addaleax Mar 1, 2017

joyeecheung Mar 5, 2017

mcollina commented Mar 1, 2017

addaleax commented Mar 1, 2017

mcollina commented Mar 3, 2017

addaleax commented Mar 3, 2017

fhinkel commented Mar 3, 2017

addaleax commented Mar 5, 2017

mcollina commented Mar 5, 2017

addaleax commented Mar 6, 2017 •

edited by fhinkel

Loading

fhinkel commented Mar 6, 2017

mcollina commented Mar 6, 2017 •

edited

Loading

addaleax commented Mar 6, 2017

mcollina commented Mar 6, 2017

mcollina left a comment

mcollina commented May 19, 2017

mcollina commented May 26, 2017

addaleax commented May 26, 2017

addaleax commented May 27, 2017

mcollina commented May 27, 2017


		* `chunk` {Buffer\|Null\|String} Chunk of data to push into the read queue
		* `chunk` {Buffer\|Uint8Array\|Null\|String} Chunk of data to push into

stream: support Uint8Array input to methods #11608

stream: support Uint8Array input to methods #11608

Conversation

addaleax commented Feb 28, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Feb 28, 2017

addaleax commented Feb 28, 2017

addaleax commented Feb 28, 2017

jasnell commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Mar 1, 2017

addaleax commented Mar 1, 2017

mcollina commented Mar 3, 2017

addaleax commented Mar 3, 2017

fhinkel commented Mar 3, 2017

addaleax commented Mar 5, 2017

mcollina commented Mar 5, 2017

addaleax commented Mar 6, 2017 • edited by fhinkel Loading

fhinkel commented Mar 6, 2017

mcollina commented Mar 6, 2017 • edited Loading

addaleax commented Mar 6, 2017

mcollina commented Mar 6, 2017

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented May 19, 2017

mcollina commented May 26, 2017

addaleax commented May 26, 2017

addaleax commented May 27, 2017

mcollina commented May 27, 2017

mscdex Feb 28, 2017 •

edited

Loading

addaleax commented Mar 6, 2017 •

edited by fhinkel

Loading

mcollina commented Mar 6, 2017 •

edited

Loading