stream: use more explicit statements by BridgeAR · Pull Request #13863 · nodejs/node

BridgeAR · 2017-06-22T01:53:34Z

Writing explicit statements produces better outcomes from the
compiler.
More important though: the instanceof Buffer check is expensive
and can be avoided as nothing besides strings could be passed
to the handler. This also fixes a TODO entry.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

streams

mscdex · 2017-06-22T03:05:40Z

Writing explicit statements produces better outcomes from the
compiler.

Can you show how you determined this? Also, what is "better" ?

BridgeAR · 2017-06-22T03:39:15Z

@mscdex this comes from @bmeurer (e.g. on a meetup)

jasnell · 2017-06-22T04:28:16Z

Ping @nodejs/streams

mscdex · 2017-06-22T05:42:08Z

@BridgeAR That leaves some questions though, like:

What version(s) of V8 does that pertain to? 5.9? 6.0? 6.1? Is it something that is/can be backported to 5.9/6.0/etc.?
Is this specific to undefined? Someone specifically asked about null in that linked video, but it wasn't clear what the answer to that is/was or if this is an optimization that works for any explicit, strict equality/inequality check.

TimothyGu · 2017-06-22T05:53:34Z

nothing besides strings could be passed to the handler

How can you be sure of this? If you want to get rid of the instanceof check, Object.getPrototypeOf() === Buffer.prototype or process.binding('util').isUint8Array is generally faster.

mcollina · 2017-06-22T09:08:35Z

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

mcollina

LGTM if CI is green

BridgeAR · 2017-06-22T11:27:52Z

@TimothyGu I forgot about the object mode even though it does not really work with the object mode and the error thrown in that situation is actually wrong. I added a test for the object mode and changed to throw that error always when using objectMode. Otherwise using objectMode would result in a weird situation where you can theoretically use this by only using Buffers but nothing else.
Besides being in the objectMode the data event can only emit strings or Buffers.

@mscdex there is more to read about this in his blog post. As far as I know it's mainly about Turbofan and that being explicit is always best but I guess @bmeurer can answer these questions better than me.

BridgeAR · 2017-06-22T17:11:36Z

lib/_stream_wrap.js

The object mode check could actually be moved in the StreamWrap constructor but it is currently possible to change the objectMode variable from an existing stream and in that case the upper check would not be sufficient anymore.
It is explicitly noted in the docs that this is not safe but I'm thinking about opening a PR to make the objectMode read only.

@mcollina do you think that's reasonable?

Streams implementors often change objectMode after the fact, so we cannot assume that. It'd a semver-major change, and several of us are -1 on those.

What you did here is ok.

BridgeAR · 2017-06-23T21:57:42Z

PTAL

mcollina · 2017-06-27T08:36:33Z

lib/_stream_wrap.js

Why this error code change?

Well, the error code was very specific and somewhat misleading that objectMode would or should not trigger this even though objectMode always would have resulted in a misleading error since it's first existence.

I think now it better represents both cases. I do see that it's not ideal to rename the code though.

Given that I don't believe the other error code had actually shipped in a release yet, this is fine. Definitely makes this semver-major tho.

The new error code needs to be included in the docs tho

mcollina · 2017-06-27T08:38:17Z

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

mcollina · 2017-06-27T12:47:58Z

The linter is failing, can you fix that? make lint will show them.

mcollina · 2017-06-27T12:48:51Z

I'm flagging this as semver-major because it changes the error code.

cc @jasnell

mcollina

LGTM when the linter is ok.

jasnell · 2017-06-27T19:21:29Z

lib/_stream_transform.js

This potentially could be a breaking change given that it would miss other falsy values potentially passed for cb

I checked. We are setting this internally, it's not user-driven.

Using objectMode with stream_wrap has not worked properly before and would end in an error. Therefore prohibit the usage of objectMode alltogether. This also improves the handling performance due to the cheaper chunk check and by using explicit statements as they produce better code from the compiler.

BridgeAR · 2017-06-29T12:48:19Z

Comment addressed and rebased due to a conflict.

@mcollina I could change the error in a separate commit to allow the main change without being semver-major if you like me to. But IMHO this is not semver-major as the error code has not yet landed in any release (to my best knowledge).

mcollina · 2017-06-29T14:42:14Z

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/8874/

mcollina · 2017-06-29T16:02:44Z

Failures are unrelated, landing.

mcollina · 2017-06-29T16:05:20Z

Landed as 1b54371.

Using objectMode with stream_wrap has not worked properly before and would end in an error. Therefore prohibit the usage of objectMode alltogether. This also improves the handling performance due to the cheaper chunk check and by using explicit statements as they produce better code from the compiler. PR-URL: #13863 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jun 22, 2017

mcollina approved these changes Jun 22, 2017

View reviewed changes

BridgeAR commented Jun 22, 2017

View reviewed changes

mcollina reviewed Jun 27, 2017

View reviewed changes

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 27, 2017

mcollina approved these changes Jun 27, 2017

View reviewed changes

jasnell reviewed Jun 27, 2017

View reviewed changes

BridgeAR force-pushed the improve-streams branch from 9e2a0de to 56f21ed Compare June 29, 2017 12:46

BridgeAR force-pushed the improve-streams branch from 56f21ed to 5ff0107 Compare June 29, 2017 12:47

mcollina added dont-land-on-v4.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 29, 2017

mcollina closed this Jun 29, 2017

BridgeAR added the performance Issues and PRs related to the performance of Node.js. label Sep 14, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

BridgeAR deleted the improve-streams branch April 1, 2019 23:36

Uh oh!

Conversation

BridgeAR commented Jun 22, 2017

Checklist

Affected core subsystem(s)

Uh oh!

mscdex commented Jun 22, 2017

Uh oh!

BridgeAR commented Jun 22, 2017

Uh oh!

jasnell commented Jun 22, 2017

Uh oh!

mscdex commented Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu commented Jun 22, 2017

Uh oh!

mcollina commented Jun 22, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jun 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jun 27, 2017

Uh oh!

mcollina commented Jun 27, 2017

Uh oh!

mcollina commented Jun 27, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Jun 29, 2017

Uh oh!

mcollina commented Jun 29, 2017

Uh oh!

mcollina commented Jun 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mscdex commented Jun 22, 2017 •

edited

Loading

BridgeAR commented Jun 29, 2017 •

edited

Loading