FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

streams: performance improvements for clearBuffer in Writable by mcollina · Pull Request #3751 · nodejs/node · GitHub

/ node Public

streams: performance improvements for clearBuffer in Writable - #3751

Closed
mcollina wants to merge 2 commits into
nodejs:masterfrom
mcollina:perf-clearBuffer
ConversationCommits (2)ChecksFiles changed
Closed

streams: performance improvements for clearBuffer in Writable#3751
mcollina wants to merge 2 commits into
nodejs:masterfrom
mcollina:perf-clearBuffer

Conversation

mcollina commented Nov 10, 2015

Copy link
Copy Markdown
Member

This commits removes a function allocation inside the clearBuffer function. Moreover, it adds counting of buffered chunks to instantiate a fixed-sized Array. The performance improvements are in the range
5-50%, depending on the usecase.

Here is gist of the results of the http benchmarks in node on my MacBook Pro Late 2014 (i7, 16GB of RAM): https://gist.github.com/mcollina/5c59c057cb5b138fcd70.

cc @trevnorris @lucamaraschi @nodejs/benchmarking

Fishrock123 added the stream Issues and PRs related to the stream subsystem. label Nov 10, 2015

geek commented Nov 10, 2015

Copy link
Copy Markdown
Member

LGTM

Fishrock123 commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

cc @nodejs/streams

trevnorris reviewed Nov 10, 2015
View reviewed changes
Comment thread lib/_stream_writable.js Outdated

trevnorris Nov 10, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Has this property already been set on the object? If not I would suggest placing it, even just this.corkedCbs = null; in the constructor. This will help prevent the object map from changing, and aide the optimizing compiler.

mcollina Nov 11, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It is done some line before: https://github.com/nodejs/node/pull/3751/files#diff-1915a7b992a3012b177dd855fa3477e0R116

mafintosh commented Nov 11, 2015

Copy link
Copy Markdown
Member

👍 from me

lucamaraschi commented Nov 11, 2015

Copy link
Copy Markdown
Contributor

👍 LGTM

chrisdickinson reviewed Nov 11, 2015
View reviewed changes
Comment thread lib/_stream_writable.js Outdated

chrisdickinson Nov 11, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can this only ever be two corkedCbs long? If we already have corkedCbs, and it has .next assigned, will we replace it with a new set of callbacks?

thefourtheye Nov 11, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Missing semi colon

mcollina Nov 11, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@chrisdickinson good spot! I planned to write more about this! My understanding is that it is only 2 corkedCbs long. Maybe I am wrong, see these lines, the callback can be deferred on the nextTick: https://github.com/mcollina/node/blob/perf-clearBuffer/lib/_stream_writable.js#L360-L364.

There might also be a different way to implement that, maybe having an array with two objects, and a even/odd counter.

chrisdickinson commented Nov 11, 2015

Copy link
Copy Markdown
Contributor

I like the direction this is going! I might be misunderstanding this bit though — is this behavior intended?

thefourtheye reviewed Nov 11, 2015
View reviewed changes
Comment thread lib/_stream_writable.js Outdated

thefourtheye Nov 11, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Missing semi colon

mcollina commented Nov 12, 2015

Copy link
Copy Markdown
Member Author

I've renamed the field, changed the little class into CorkedCbs, and added the missing semicolons.

@chrisdickinson are you happy with my explanation on the double element? Do you see another (possibly faster) way to implement this?

jasnell commented Nov 12, 2015

Copy link
Copy Markdown
Member

LGTM

chrisdickinson commented Nov 13, 2015

Copy link
Copy Markdown
Contributor

@chrisdickinson are you happy with my explanation on the double element? Do you see another (possibly faster) way to implement this?

I think so, though I might suggest adding a comment (or an assert) stating that we only ever expect two of these per stream. Thanks for bearing with me!

mcollina commented Nov 13, 2015

Copy link
Copy Markdown
Member Author

@chrisdickinson I have added an assert and a comment. It should be ok now.

Let me know if it is ok and I'll squash my commits.

jasnell commented Nov 15, 2015

Copy link
Copy Markdown
Member

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

jasnell commented Nov 15, 2015

Copy link
Copy Markdown
Member

CI Looks good. Couple of related failures.

mcollina commented Nov 15, 2015

Copy link
Copy Markdown
Member Author

@jasnell anything I should look into/fix? It seems there are windows-specific and arm-specific failures.

On a window configuration, test-child-process-fork-regr-gh-2847.js is failing.

On ARM, I can't access https://ci.nodejs.org/job/node-test-commit-arm/1188/nodes=armv7-wheezy/tapTestReport/ (I get a Gateway Timeout) - so I can't really know.

On ARM fanned (what does that mean?), I'm getting a timeout/failure on test-crypto-dh.js.

jasnell commented Nov 15, 2015

Copy link
Copy Markdown
Member

There's nothing you need to do. The issues are with the CI environment
itself unfortunately
On Nov 15, 2015 2:22 AM, "Matteo Collina" notifications@github.com wrote:

@jasnell https://github.com/jasnell anything I should look into/fix? It
seems there are windows-specific and arm-specific failures.

On a window configuration, test-child-process-fork-regr-gh-2847
https://github.com/nodejs/node/issues/2847.js is failing.

On ARM, I can't access
https://ci.nodejs.org/job/node-test-commit-arm/1188/nodes=armv7-wheezy/tapTestReport/
(I get a Gateway Timeout) - so I can't really know.

On ARM fanned (what does that mean?), I'm getting a timeout/failure on
test-crypto-dh.js.

—
Reply to this email directly or view it on GitHub
#3751 (comment).

chrisdickinson commented Dec 2, 2015

Copy link
Copy Markdown
Contributor

LGTM — I don't have access to the CI server at the moment, perhaps @jasnell can restart CI?

MylesBorins commented Dec 2, 2015

Copy link
Copy Markdown
Contributor

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

rvagg commented Dec 2, 2015

Copy link
Copy Markdown
Member

@chrisdickinson:

I don't have access to the CI server at the moment

Do you mean technically or you're on your phone or something that makes it impractical? If you technically can't access it then we should sort that out!

chrisdickinson commented Dec 2, 2015

Copy link
Copy Markdown
Contributor

@rvagg: ah! I take it back. Last I checked I was in a weird state where I could kind of log in using the old pw auth, but it seems to have resolved. False alarm!

cjihrig mentioned this pull request Dec 4, 2015
Closed
streams: perf improvments for clearBuffer in Writable.
1819b6f 
This commits removes a function allocation inside the clearBuffer
function. Moreover, it adds counting of buffered chunks to instantiate
a fixed-sized Array. The performance improvements are in the range
5-50%, depending on the usecase.

mcollina commented Dec 14, 2015

Copy link
Copy Markdown
Member Author

I've run this on the CI, and I'm getting a possibly unrelated failure on node-test-binary-windows: https://ci.nodejs.org/job/node-test-binary-windows/301/.

(I've rebased this to master, and squashed the commits)

@chrisdickinson @jasnell if you folks are good with this, I would like to get this merged at some point this week.

cjihrig commented Dec 14, 2015

Copy link
Copy Markdown
Contributor

Those are known flakey tests, so LGTM.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL