stream: skip chunk validation for Buffers #10580

mscdex · 2017-01-02T23:56:14Z

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

Fishrock123 · 2017-01-02T23:58:50Z

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

ChALkeR

LGTM after CI passes.

mscdex · 2017-01-03T00:06:01Z

@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.

mscdex · 2017-01-03T01:36:47Z

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

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

italoacasas · 2017-01-03T01:51:04Z

@nodejs/streams

Fishrock123 · 2017-01-03T15:04:14Z

Oh, maybe I was thinking about typeof.

Fishrock123 · 2017-01-03T15:05:00Z

lib/_stream_writable.js

-  if (chunk instanceof Buffer)
-    encoding = 'buffer';
+function writeOrBuffer(stream, state, isBuf, chunk, encoding, cb) {
+  if (!isBuf) {


@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.

mcollina

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.

mcollina · 2017-01-03T16:06:36Z

lib/_stream_writable.js

-// and we're not in objectMode, then that's an error.
-// Otherwise stream chunks are all considered to be of length=1, and the
-// watermarks determine how many objects to keep in the buffer, rather than
-// how many bytes or characters.


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.

mscdex · 2017-01-03T16:10:40Z

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.

mcollina · 2017-01-03T16:17:16Z

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.

jasnell · 2017-01-03T16:19:39Z

benchmark/streams/writable-manywrites.js

+
+function main(conf) {
+  const n = +conf.n;
+  const b = new Buffer(1024);


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.

jasnell · 2017-01-03T16:22:14Z

lib/_stream_writable.js

-      typeof chunk !== 'string' &&
-      chunk !== undefined &&
-      !state.objectMode) {
+  } else if (typeof chunk !== 'string' &&


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.

thefourtheye · 2017-01-03T16:22:29Z

benchmark/streams/writable-manywrites.js

+
+function main(conf) {
+  const n = +conf.n;
+  const b = new Buffer(1024);


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

I just copied from one of the other buffer benchmarks.

mcollina

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>

MylesBorins · 2017-03-07T01:55:51Z

@mscdex how long should these bake before LTS?

mcollina · 2017-03-07T15:39:41Z

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 ).

mcollina · 2017-03-27T08:57:15Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: skip chunk validation for Buffers #10580

stream: skip chunk validation for Buffers #10580

mscdex commented Jan 2, 2017 •

edited

Loading

Fishrock123 commented Jan 2, 2017

ChALkeR left a comment

mscdex commented Jan 3, 2017 •

edited

Loading

mscdex commented Jan 3, 2017 •

edited

Loading

italoacasas commented Jan 3, 2017

Fishrock123 commented Jan 3, 2017

Fishrock123 Jan 3, 2017

mscdex Jan 3, 2017

mcollina left a comment

mcollina Jan 3, 2017

mscdex Jan 3, 2017

mscdex Jan 3, 2017 •

edited

Loading

mscdex commented Jan 3, 2017

mcollina commented Jan 3, 2017

jasnell Jan 3, 2017

mscdex Jan 3, 2017

jasnell Jan 3, 2017

mcollina Jan 3, 2017

mscdex Jan 3, 2017 •

edited

Loading

jasnell Jan 3, 2017

mcollina Jan 3, 2017

mscdex Jan 3, 2017

thefourtheye Jan 3, 2017

mscdex Jan 3, 2017

mscdex Jan 3, 2017

mcollina left a comment

MylesBorins commented Mar 7, 2017

mcollina commented Mar 7, 2017

mcollina commented Mar 27, 2017

stream: skip chunk validation for Buffers #10580

stream: skip chunk validation for Buffers #10580

Conversation

mscdex commented Jan 2, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Fishrock123 commented Jan 2, 2017

ChALkeR left a comment

Choose a reason for hiding this comment

mscdex commented Jan 3, 2017 • edited Loading

mscdex commented Jan 3, 2017 • edited Loading

italoacasas commented Jan 3, 2017

Fishrock123 commented Jan 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jan 3, 2017 • edited Loading

Choose a reason for hiding this comment

mscdex commented Jan 3, 2017

mcollina commented Jan 3, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

MylesBorins commented Mar 7, 2017

mcollina commented Mar 7, 2017

mcollina commented Mar 27, 2017

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