stream: skip chunk validation for Buffers by mscdex · Pull Request #10580 · nodejs/node · GitHub

These changes result in ~50% improvement in the included benchmark:

                                          improvement significant      p.value
 streams/writable-manywrites.js n=2000000     52.38 %         *** 3.778963e-47

/cc @nodejs/streams
CI: https://ci.nodejs.org/job/node-test-pull-request/5668/

Checklist

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

Affected core subsystem(s)

stream

instanceof should be very fast, is there any way to inspect why this speeds it up that much?

LGTM after CI passes.

@Fishrock123 Actually, instanceof is usually quite slow from my experience, even in V8 5.4. The InstanceOf V8 C++ function is usually one of the top few functions that consume the most ticks when profiling anything that uses (Writable) streams.

It's also avoiding a function call in case validChunk doesn't get inlined for some reason (most likely due to reaching max inline depth), so there is that too.

Added another optimization that provides an additional ~30% increase.

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

@nodejs/streams

Oh, maybe I was thinking about typeof.

@mscdex could chunk have changed before this is called?

No. writeOrBuffer() is only called by write() and validChunk() does not return a modified chunk or anything.

The code in itself is fine, and it's a very good catch. However the title in the PR and the commit message does not match the content.
The check is not skipped, it is just executed once.

You should add back some comment here, explaining this change. validChunk now only checks if a chunk is valid when it is not a Buffer.

I removed the comments here because they either duplicated what the inline comments already said or had nothing to do with validChunk() ('Otherwise stream chunks ...').

Even the inline comments are pretty redundant as they just explain what the simple code below it is doing, not worth keeping IMHO. I have added a new comment above the function definition now.

The check is not skipped, it is just executed once.

I'm not sure what you mean by this. validChunk() is skipped when chunk is a Buffer.

I'm not sure what you mean by this. validChunk() is skipped when chunk is a Buffer.

I mean that the chunk is validated, i.e. chunk instanceof Buffer is executed, just not on the validChunk method.

If this is going to be kept as new Buffer(...), can you add a comment explaining why (e.g. to allow comparison with older Node.js versions). That said, I'd prefer this to be changed to Buffer.alloc(...) given that Node.js 4 is the lowest currently supported version and the new constructors have been backported to that version already.

Changed.

Would switching to Buffer.isBuffer() have the same performance profile as instanceof? By removing the instanceof Buffer check here, this becomes a semver-major. If we can switch to Buffer.isBuffer() and still get a performance boost, then this can be semver-patch.

@jasnell this PR is not removing the check (see my comment). It is still doing it, just in a different way. I would flag it semver-minor, and it can even be backported to v4 and v6.

Buffer.isBuffer() is just an instanceof check, so that would not help. Also, @mcollina is correct, we're just avoiding duplicate instanceofs since it was already being done in write().

Ah.. lol, forgot about that :-)

@mscdex that's my point on the review. "remove duplicate instanceof checks" should be a good commit message/pr title.

@mcollina It's not just that though, for example it would have also checked for null before getting to the second instanceof check. Anyway, I've changed the commit message. Let me know if it's more suitable.

Shouldn't we use Buffer.alloc or Buffer.allocUnsafe here?

I just copied from one of the other buffer benchmarks.

Changed.

LGTM

These changes result in ~50% improvement in the included benchmark. PR-URL: nodejs#10580 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

@mscdex how long should these bake before LTS?

I'm 👍 , but I think we should assemble a release of readable-stream first that embeds those in.
(I can put some time in the next few weeks to fire a PR and post back cc @calvinmetcalf ).

I'm 👍 on backporting to v6.

These changes result in ~50% improvement in the included benchmark. PR-URL: #10580 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

These changes result in ~50% improvement in the included benchmark. PR-URL: nodejs/node#10580 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mscdex added performance Issues and PRs related to the performance of Node.js. stream Issues and PRs related to the stream subsystem. labels Jan 2, 2017

nodejs-github-bot added the lts-watch-v6.x label Jan 2, 2017

ChALkeR approved these changes Jan 3, 2017

View reviewed changes

mscdex force-pushed the stream-writable-write-perf branch from 33dbb04 to f9ca2d8 Compare January 3, 2017 01:35

nodejs-github-bot added the lts-watch-v6.x label Jan 3, 2017

mscdex force-pushed the stream-writable-write-perf branch from f9ca2d8 to 1099c80 Compare January 3, 2017 01:36

nodejs-github-bot added the lts-watch-v6.x label Jan 3, 2017

Fishrock123 reviewed Jan 3, 2017

View reviewed changes

mcollina requested changes Jan 3, 2017

View reviewed changes

mcollina reviewed Jan 3, 2017

View reviewed changes

jasnell reviewed Jan 3, 2017

View reviewed changes

thefourtheye reviewed Jan 3, 2017

View reviewed changes

mscdex force-pushed the stream-writable-write-perf branch from 1099c80 to bb29b22 Compare January 3, 2017 22:43

nodejs-github-bot added the lts-watch-v6.x label Jan 3, 2017

mcollina approved these changes Jan 3, 2017

View reviewed changes

jasnell approved these changes Jan 4, 2017

View reviewed changes

mscdex added the lts-watch-v4.x label Jan 4, 2017

mscdex force-pushed the stream-writable-write-perf branch from bb29b22 to 0a93728 Compare January 5, 2017 08:00

nodejs-github-bot added the lts-watch-v6.x label Jan 5, 2017

mscdex merged commit 0a93728 into nodejs:master Jan 5, 2017

mscdex deleted the stream-writable-write-perf branch January 5, 2017 08:03

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Mar 28, 2017

MylesBorins mentioned this pull request Apr 19, 2017

v6.10.3 proposal #12498

Merged

stream: skip chunk validation for Buffers by mscdex · Pull Request #10580 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: skip chunk validation for Buffers - #10580

stream: skip chunk validation for Buffers#10580mscdex merged 1 commit intonodejs:masternodejs/node:masterfrom mscdex:stream-writable-write-perfmscdex/io.js:stream-writable-write-perfCopy head branch name to clipboard

Conversation

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

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Fishrock123 commented Jan 2, 2017

Uh oh!

ChALkeR left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

italoacasas commented Jan 3, 2017

Uh oh!

Fishrock123 commented Jan 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

mscdex Jan 3, 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!

mscdex commented Jan 3, 2017

Uh oh!

mcollina commented Jan 3, 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!

mscdex Jan 3, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Mar 7, 2017

Uh oh!

mcollina commented Mar 7, 2017

Uh oh!

mcollina commented Mar 27, 2017

Uh oh!

Reviewers

Assignees

stream: skip chunk validation for Buffers#10580
mscdex merged 1 commit into
nodejs:masterfrom
mscdex:stream-writable-write-perf

mscdex commented Jan 2, 2017 •
edited
Loading

mscdex commented Jan 3, 2017 •
edited
Loading

mscdex commented Jan 3, 2017 •
edited
Loading

mscdex Jan 3, 2017 •
edited
Loading

mscdex Jan 3, 2017 •
edited
Loading