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: writable buffering #28978

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 5, 2019

Simplify and optimize writable stream buffering.

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 Aug 5, 2019
@ronag
Copy link
Member Author

ronag commented Aug 5, 2019

@mcollina @Trott I need a little help on how to run and compare benchmarks on this? Haven't really found the instructions in the guidelines...

@mcollina
Copy link
Member

mcollina commented Aug 5, 2019

Here they are: https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md

Be careful with this change, you are touching one of the worst hot paths in Node.js (together with emit  and nextTick). The current code is complex for reasons that made sense in old version of Node.js, so it might be a good time to check if those assumptions still hold true.

@mcollina
Copy link
Member

mcollina commented Aug 5, 2019

The semversiness of this is a bit problematic, as this is likely to break some external users.

@ronag
Copy link
Member Author

ronag commented Aug 5, 2019

@mcollina: this is definitly semver major.

Is this something that you would even consider? If not please let me know so I don't spend time in vain :).

@ronag ronag force-pushed the stream-writable-buffer branch from dc5503a to a092cb8 Compare August 5, 2019 14:05
@ronag ronag mentioned this pull request Aug 5, 2019
4 tasks
@mcollina
Copy link
Member

mcollina commented Aug 5, 2019

I think it's worthwhile having a look if it could be simplified/improved the performance of it.

@ronag ronag force-pushed the stream-writable-buffer branch from a092cb8 to 23ee586 Compare August 5, 2019 14:20
@lpinca
Copy link
Member

lpinca commented Aug 5, 2019

This should definitely be benchmarked to see if it's actually faster and how much.

lib/_stream_writable.js Outdated Show resolved Hide resolved
@ronag ronag force-pushed the stream-writable-buffer branch from 23ee586 to b6a212f Compare August 5, 2019 14:29
@ronag
Copy link
Member Author

ronag commented Aug 5, 2019

@mcollina @lpinca

Before:

writable-manywrites.js: 2,549,442

After:

writable-manywrites.js: 2,652,592

@ronag ronag force-pushed the stream-writable-buffer branch from b6a212f to 3e51f4c Compare August 5, 2019 14:50
lib/_stream_writable.js Outdated Show resolved Hide resolved
@ronag ronag force-pushed the stream-writable-buffer branch 2 times, most recently from a2ce565 to a8ba29c Compare August 5, 2019 15:00
@ronag
Copy link
Member Author

ronag commented Aug 5, 2019

Got the benchmark comparison working!

streams/writable-manywrites.js n=2000000                 0.83 %       ±2.70% ±3.60% ±4.71%

Seems like a negligible difference. Though the simplification part is still probably worth it.

@ronag ronag force-pushed the stream-writable-buffer branch 12 times, most recently from c0088f0 to a5607bd Compare August 5, 2019 17:20
@ronag ronag force-pushed the stream-writable-buffer branch 3 times, most recently from d99f8da to 85b51a2 Compare August 5, 2019 17:36
lib/_stream_writable.js Outdated Show resolved Hide resolved
@ronag ronag force-pushed the stream-writable-buffer branch from 85b51a2 to 30cb4e6 Compare August 5, 2019 18:02
@ronag
Copy link
Member Author

ronag commented Aug 5, 2019

@mcollina nothing I do here seems to make much of a difference... maybe not a hot path or maybe better benchmarks are needed? Or I'm doing something wrong...

The current benchmark mostly test sync writing... which I don't think is a good real-world example.

@ronag ronag force-pushed the stream-writable-buffer branch 7 times, most recently from bfc1416 to cd6006c Compare August 5, 2019 18:16
@ronag
Copy link
Member Author

ronag commented Aug 5, 2019

Note this also changes the behaviour so that 'finish' is never emitted after 'error'.

@ronag ronag force-pushed the stream-writable-buffer branch 2 times, most recently from 53164df to df77306 Compare August 5, 2019 18:33
@ronag
Copy link
Member Author

ronag commented Aug 5, 2019

@mcollina can you please look into this one #28979 before further reviewing this PR.

@ronag ronag force-pushed the stream-writable-buffer branch from df77306 to 69a6436 Compare August 5, 2019 18:59
@ronag ronag force-pushed the stream-writable-buffer branch from 69a6436 to 9033d42 Compare August 5, 2019 19:00
@ronag
Copy link
Member Author

ronag commented Aug 5, 2019

this is WIP

@ronag ronag closed this Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants