Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: improve write performance #30736

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Nov 30, 2019

This is inspired by #30710.

Depends on #30733 (which is included).

Improves performance of write by avoiding process.nextTick and a number of conditions when possible in the hot path in sync mode.

``` confidence improvement accuracy (*) (**) (***) streams/writable-manywrites.js n=2000000 *** 1085.31 % ±19.19% ±25.86% ±34.31% ````

EDIT: A bit more modest improvement after #30710

                                                      confidence improvement accuracy (*)   (**)  (***)
 streams/writable-manywrites.js sync='no' n=2000000                  0.10 %       ±1.46% ±1.94% ±2.54%
 streams/writable-manywrites.js sync='yes' n=2000000        ***      8.43 %       ±1.73% ±2.30% ±3.00%

Note since #30733 is probably semver major, this should be the same.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Nov 30, 2019
@ronag

This comment has been minimized.

@ronag ronag force-pushed the stream-afterwrite3 branch 2 times, most recently from c1311ea to 601faeb Compare November 30, 2019 15:23
@ronag
Copy link
Member Author

ronag commented Nov 30, 2019

ping @addaleax @mcollina @mscdex

@ronag ronag force-pushed the stream-afterwrite3 branch 3 times, most recently from feab352 to 520388b Compare November 30, 2019 16:19
lib/_stream_writable.js Outdated Show resolved Hide resolved
@ronag ronag force-pushed the stream-afterwrite3 branch 4 times, most recently from 092225e to 5a99949 Compare November 30, 2019 20:29
@addaleax
Copy link
Member

addaleax commented Dec 1, 2019

@ronag Could you review this? I’ll review this as soon as I can :)

@ronag
Copy link
Member Author

ronag commented Dec 1, 2019

@addaleax rebased

@ronag
Copy link
Member Author

ronag commented Dec 1, 2019

A bit more modest improvement after #30710

                                                      confidence improvement accuracy (*)   (**)  (***)
 streams/writable-manywrites.js sync='no' n=2000000                  0.10 %       ±1.46% ±1.94% ±2.54%
 streams/writable-manywrites.js sync='yes' n=2000000        ***      8.43 %       ±1.73% ±2.30% ±3.00%

@ronag ronag force-pushed the stream-afterwrite3 branch 2 times, most recently from 6d04fa4 to 63da91b Compare December 1, 2019 09:38
@lpinca lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 1, 2019
@lpinca
Copy link
Member

lpinca commented Dec 1, 2019

Added semver-major label due to e828f510.

@mcollina
Copy link
Member

mcollina commented Dec 1, 2019

Let's park this until #30733 is landed

@ronag
Copy link
Member Author

ronag commented Dec 14, 2019

This needs a blocked label

@richardlau richardlau added the blocked PRs that are blocked by other issues or PRs. label Dec 14, 2019
@ronag
Copy link
Member Author

ronag commented Dec 14, 2019

@mcollina @Trott this is no longer blocked

@Trott Trott removed the blocked PRs that are blocked by other issues or PRs. label Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants