GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — stream: skip chunk validation for Buffers by mscdex · Pull Request #10580 · nodejs/node

Conversation

@mscdex

@mscdex mscdex commented Jan 2, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ChALkeR and andrasq reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@mscdex mscdex added performance Issues and PRs related to the performance of Node.js. stream Issues and PRs related to Node.js streams. labels Jan 2, 2017
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Jan 2, 2017
@Fishrock123

Fishrock123 commented Jan 2, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ChALkeR
ChALkeR approved these changes Jan 3, 2017
View reviewed changes

@ChALkeR ChALkeR left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after CI passes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mscdex

mscdex commented Jan 3, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@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.
lpinca reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex
mscdex force-pushed the stream-writable-write-perf branch from 33dbb04 to f9ca2d8 Compare January 3, 2017 01:35
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Jan 3, 2017
@mscdex
mscdex force-pushed the stream-writable-write-perf branch from f9ca2d8 to 1099c80 Compare January 3, 2017 01:36
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Jan 3, 2017
@mscdex

mscdex commented Jan 3, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@italoacasas

italoacasas commented Jan 3, 2017

Copy link
Copy Markdown

@nodejs/streams
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented Jan 3, 2017

Copy link
Copy Markdown
Contributor

Oh, maybe I was thinking about typeof.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fishrock123
Fishrock123 reviewed Jan 3, 2017
View reviewed changes
Comment thread lib/_stream_writable.js Outdated

@Fishrock123 Fishrock123 Jan 3, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mscdex could chunk have changed before this is called?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mscdex mscdex Jan 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mcollina
mcollina requested changes Jan 3, 2017
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mcollina
mcollina reviewed Jan 3, 2017
View reviewed changes
Comment thread lib/_stream_writable.js Outdated

@mcollina mcollina Jan 3, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mscdex mscdex Jan 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mscdex mscdex Jan 3, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mscdex

mscdex commented Jan 3, 2017

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Jan 3, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell reviewed Jan 3, 2017
View reviewed changes
Comment thread benchmark/streams/writable-manywrites.js Outdated

@jasnell jasnell Jan 3, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mscdex mscdex Jan 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell reviewed Jan 3, 2017
View reviewed changes
Comment thread lib/_stream_writable.js Outdated

@jasnell jasnell Jan 3, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mcollina mcollina Jan 3, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mscdex mscdex Jan 3, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell Jan 3, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah.. lol, forgot about that :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mcollina mcollina Jan 3, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mscdex mscdex Jan 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
thefourtheye
thefourtheye reviewed Jan 3, 2017
View reviewed changes
Comment thread benchmark/streams/writable-manywrites.js Outdated

@thefourtheye thefourtheye Jan 3, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mscdex mscdex Jan 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied from one of the other buffer benchmarks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mscdex mscdex Jan 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mscdex
mscdex force-pushed the stream-writable-write-perf branch from 1099c80 to bb29b22 Compare January 3, 2017 22:43
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Jan 3, 2017
mcollina
mcollina approved these changes Jan 3, 2017
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell approved these changes Jan 4, 2017
View reviewed changes
@mscdex mscdex added the lts-watch-v4.x label Jan 4, 2017
@mscdex
stream: avoid additional validation for Buffers
0a93728 
These changes result in ~50% improvement in the included benchmark.

PR-URL: nodejs#10580
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mscdex
mscdex force-pushed the stream-writable-write-perf branch from bb29b22 to 0a93728 Compare January 5, 2017 08:00
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Jan 5, 2017
@mscdex
mscdex merged commit 0a93728 into nodejs:master Jan 5, 2017
@mscdex
mscdex deleted the stream-writable-write-perf branch January 5, 2017 08:03
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@mscdex Italo A. Casas
stream: avoid additional validation for Buffers
f0a10ff 
These changes result in ~50% improvement in the included benchmark.

PR-URL: nodejs#10580
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
@mscdex Italo A. Casas
stream: avoid additional validation for Buffers
a13b1bc 
These changes result in ~50% improvement in the included benchmark.

PR-URL: nodejs#10580
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
@mscdex Italo A. Casas
stream: avoid additional validation for Buffers
5c04c9d 
These changes result in ~50% improvement in the included benchmark.

PR-URL: nodejs#10580
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 25, 2017
Proposal to update v7.x-staging #10989
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@mscdex Italo A. Casas
stream: avoid additional validation for Buffers
59196af 
These changes result in ~50% improvement in the included benchmark.

PR-URL: nodejs#10580
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
v7.5.0 proposal #11062
Merged
@MylesBorins

MylesBorins commented Mar 7, 2017

Copy link
Copy Markdown
Contributor

@mscdex how long should these bake before LTS?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Mar 7, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Mar 27, 2017

Copy link
Copy Markdown
Member

I'm 👍 on backporting to v6.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
@mscdex @MylesBorins
stream: avoid additional validation for Buffers
cc2b3e6 
These changes result in ~50% improvement in the included benchmark.

PR-URL: #10580
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Mar 28, 2017
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
@mscdex @MylesBorins
stream: avoid additional validation for Buffers
0e9bd94 
These changes result in ~50% improvement in the included benchmark.

PR-URL: #10580
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
@mscdex @MylesBorins
stream: avoid additional validation for Buffers
a0e1b1e 
These changes result in ~50% improvement in the included benchmark.

PR-URL: #10580
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 4, 2017
@mscdex @MylesBorins
stream: avoid additional validation for Buffers
078188d 
These changes result in ~50% improvement in the included benchmark.

PR-URL: #10580
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@mscdex @MylesBorins
stream: avoid additional validation for Buffers
f928746 
These changes result in ~50% improvement in the included benchmark.

PR-URL: #10580
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
v6.10.3 proposal #12498
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@mscdex @andrew749
stream: avoid additional validation for Buffers
8155ad4 
These changes result in ~50% improvement in the included benchmark.

PR-URL: nodejs/node#10580
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@ChALkeR ChALkeR ChALkeR approved these changes
@jasnell jasnell jasnell approved these changes
+2 more reviewers
@thefourtheye thefourtheye thefourtheye left review comments
@Fishrock123 Fishrock123 Fishrock123 left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

performance Issues and PRs related to the performance of Node.js. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@mscdex @Fishrock123 @italoacasas @mcollina @MylesBorins @ChALkeR @jasnell @thefourtheye @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.