Skip to content

stream: some simplification #32900

New issue

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

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

Already on GitHub? # to your account

Closed
wants to merge 4 commits into from
Closed

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 17, 2020

~7% improvement

NOTE: Contains 2 commits to reduce the number of PRs.

 streams/writable-manywrites.js callback='no' writev='no' sync='no' n=2000000                    6.27 %       ±6.47%  ±8.63% ±11.26%
 streams/writable-manywrites.js callback='no' writev='no' sync='yes' n=2000000            *      8.43 %       ±6.68%  ±8.90% ±11.59%
 streams/writable-manywrites.js callback='no' writev='yes' sync='no' n=2000000                   7.77 %       ±7.88% ±10.49% ±13.69%
 streams/writable-manywrites.js callback='no' writev='yes' sync='yes' n=2000000           *      6.79 %       ±5.75%  ±7.65%  ±9.96%
 streams/writable-manywrites.js callback='yes' writev='no' sync='no' n=2000000          ***     11.31 %       ±6.08%  ±8.11% ±10.60%
 streams/writable-manywrites.js callback='yes' writev='no' sync='yes' n=2000000          **      7.76 %       ±5.46%  ±7.27%  ±9.46%
 streams/writable-manywrites.js callback='yes' writev='yes' sync='no' n=2000000                  6.12 %       ±7.40%  ±9.85% ±12.82%
 streams/writable-manywrites.js callback='yes' writev='yes' sync='yes' n=2000000         **      8.11 %       ±4.90%  ±6.52%  ±8.48%
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

writecb casted to boolean has the same
value as writing.
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Apr 17, 2020
@nodejs-github-bot
Copy link
Collaborator

@ronag ronag requested a review from lpinca April 17, 2020 07:16
Was doing some unecessary checks.
@ronag ronag force-pushed the writable-simplify branch from 959cba0 to 4ac87be Compare April 17, 2020 09:04
!state.corked &&
!state.bufferProcessing &&
state.bufferedRequest) {
if (state.bufferedRequest) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This condition is a duplicate of what's inside of clearBuffer. However, it is here on purpose since clearBuffer is not inlineable.

@mscdex
Copy link
Contributor

mscdex commented Apr 17, 2020

This seems to have a negative effect on Duplex creation:

streams/creation.js kind='duplex' n=50000000                                           ***     -6.94 %       ±3.44% ±4.62%  ±6.09%

@ronag
Copy link
Member Author

ronag commented Apr 17, 2020

@mscdex Flaky made?

Local:

 streams/creation.js kind='duplex' n=5000000                 2.88 %       ±5.44% ±7.25% ±9.46%

@ronag
Copy link
Member Author

ronag commented Apr 17, 2020

@ronag
Copy link
Member Author

ronag commented Apr 17, 2020

Nope, seems consistent:

17:55:08  streams/creation.js kind='duplex' n=50000000           ***     -7.40 %       ±2.49% ±3.31% ±4.31%

Strange... not sure how this PR could cause that

@ronag
Copy link
Member Author

ronag commented Apr 17, 2020

@ronag
Copy link
Member Author

ronag commented Apr 17, 2020

@mscdex: I'm quite baffled why this PR would affect duplex creation performance. Any ideas?

Anyway, a 7% performance increase in writing I guess is still worth it with a 7% performance degradation in creation?

This reverts commit 206f62b.
@mscdex
Copy link
Contributor

mscdex commented Apr 17, 2020

I don't know offhand, you'd probably have to compare either the generated/optimized code or optimizer/inliner trace output before and after this PR to see what is different.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
stream Issues and PRs related to the stream subsystem. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants