stream: improve Readable.read() performance

[mscdex]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• the commit message follows commit guidelines

Affected core subsystem(s)

• stream

Description of change

read() performance is improved most by switching from an array to a linked list for storing buffered data. However, other changes that also contribute include: making some hot functions inlinable, faster
read() argument checking, and misc code rearrangement to avoid unnecessary code execution.

These improvements only exist for when n is supplied to read(), otherwise performance is the same.

Note: I did change how negative n values are interpreted when in object mode. Before this PR, negative n values were interpreted as 1 in howMuchToRead(). I changed this to instead return 0 to match how negative n values are interpreted for non-object mode streams.

Results from the included benchmarks:

streams/readable-bigread.js n=1000: ./node: 306.98 ./node-master: 171.91 ....... 78.57%
streams/readable-bigunevenread.js n=1000: ./node: 213.85 ./node-master: 125.26 . 70.72%
streams/readable-boundaryread.js n=2000: ./node: 448.09 ./node-master: 298.88 .. 49.92%
streams/readable-unevenread.js n=1000: ./node: 49.565 ./node-master: 34.442 .... 43.91%
streams/readable-readall.js n=5000: ./node: 772.56 ./node-master: 766.4 ......... 0.80%

[mscdex]

/cc @nodejs/streams

[mscdex]

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

[calvinmetcalf]

should buffer list maybe live in the internals folder ?

[mscdex]

@calvinmetcalf Debatable but I left it there since it's only used by Readable and it's better if Readable isn't referencing anything from internal/ since streams are extracted from node?

[calvinmetcalf]

I can extract that other file easily, I'm more worried about exporting that from readable which is not on the api

[mscdex]

@calvinmetcalf I don't understand what you mean by "is not on the api."

[calvinmetcalf]

sorry, misspoke, I meant that I didn't like exporting that new thing from readable

[mscdex]

@calvinmetcalf I can remove it if necessary I suppose, but then we'd end up duplicating some of it in the tests I had to modify. I thought it would be better not to have to keep two separate "implementations" in sync.

[calvinmetcalf]

I was thinking that if that file got moved to something in internal, then you could just require that for the tests and require it for from stream_readable and don't worry about the readable-stream module, it won't be much of an inconvenience.

[mscdex]

@calvinmetcalf Ok, moved to internal.

[ronkorving]

Do you think it could help to create an Entry class to increase property access performance?

[mscdex]

I just tried it and it doesn't seem to make any real difference. If anything it seems to slow things down a little bit.

[mscdex]

/cc @nodejs/collaborators Anyone have any comments/questions/LGTMs on this?

[ronkorving]

I'm not sure on the style guide on this, but the outer braces seem useless.

[mscdex]

You mean the parens? shrug I always add that when "assigning" a ternary statement to a variable as it seems more readable/clear to me.

[ronkorving]

I noticed :) I don't really mind, just curious if there's any consensus on this. No blocker, no worries.

And sorry, yeah, parens (I, non-native English speaker, always get these mixed up... sigh)

[jasnell]

const?

[mscdex]

Fixed.

[jasnell]

First read through looks good but I'll have to go through in more detail tomorrow. In general tho, if @nodejs/streams is happy, then ++

[mcollina]

Do we have a CITGM run?

If that passes.. LGTM. just with one catch.. Let's give this a long time before we backport.

[mscdex]

@mcollina citgm: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/296/

[mcollina]

the job was aborted, should we kick it off again?

[mscdex]

@mcollina It was aborted most likely because the ppcle machine is/was still down. The lodash failure on OSX1010 looks like an npm or citgm failure since it failed on installation of the package.

Otherwise citgm is green.

[mcollina]

LGTM as semver-minor.

[Fishrock123]

@mcollina Why is this semver-minor?

[mcollina]

@Fishrock123 avoiding possible breakages in a patch release. IMHO performance optimizations are semver-minor, as performance is a backward-compatible feature.

[Fishrock123]

@mcollina So you are saying this is semver-major then? This is at obvious risk of breaking something? Do I need to back it out of 6.3.0?

How this works is, in this order:

• Does it have a chance of breaking something? -> Major
• Does it add something that won't work on previous versions (feature/api)? -> Minor
• Everything else: -> Patch

[mcollina]

@Fishrock123

So you are saying this is semver-major then? This is at obvious risk of breaking something? Do I need to back it out of 6.3.0?

no and no.
We are touching streams, and based on latest history, there is a slight higher risk of breaking something compared to other part of core. Because of this, I would put them into semver-minor anything that is not strictly a bug fix.

This is a performance optimization, e.g. a new feature for me.

[Fishrock123]

So... don't land on LTS is what I'm getting out of this. :)

[mcollina]

@Fishrock123 I think this one will be fine on LTS, but I would recommend porting it after some months, not weeks. Something like "land when the new LTS is out".

[mafintosh]

@Fishrock123 @mcollina just re your above conversation, this ended up breaking duplexify (https://www.npmjs.com/package/duplexify) because it expected the _buffer to be an array

[mcollina]

I feared something like that would have happened.

I think we should make a final decision either to semver _readableState and _writableState API or not.
Formally, they are not part of the public API, and these are causing all sort of issues on streams modules.

I'm happy we got this early (before a backport to readable-stream).

[ronkorving]

They're clearly private. If people want to manipulate them, we can expose public API for that. The duplexify package authors could've pushed/asked for that, and still can imho.

[mafintosh]

@mcollina could you open a separate issue for the semver discussion so we don't take over this one? for the record i think reading some of the properties in _readableState is essential for a bunch of use-cases. like figuring out if a stream is in objectMode, figuring out if it ended but still has data in its read buffer etc.

[mcollina]

@mafintosh done ^