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: bitfield #29127

Closed
wants to merge 3 commits into from
Closed

stream: bitfield #29127

wants to merge 3 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 14, 2019

Tries to significantly reduce readable state size for lower memory overhead and possibly better performance.

I'm making the assumption here that V8 is able to inline getters and setters.

TODO:

  • put flowing into bitfield
  • ensure all properties are accessed inline
    - [ ] use local bitfield instance to avoid property access through state.
  • improve readability

Refs: #29126

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

ronag commented Aug 14, 2019

I need a little help/guidance on how to do or create relevant benchmarks.

@ronag
Copy link
Member Author

ronag commented Aug 14, 2019

Haven't quite sorted out flowing. It seems to be a tribool.

@ronag ronag force-pushed the stream-bitfield branch 2 times, most recently from ada2af9 to 5a16355 Compare August 14, 2019 20:41
@ronag
Copy link
Member Author

ronag commented Aug 14, 2019

If this is accepted we should probably do the same for Writable and OutgoingMessage.

@addaleax
Copy link
Member

@mscdex
Copy link
Contributor

mscdex commented Aug 14, 2019

Results don't look so good:

                                                      confidence improvement accuracy (*)    (**)   (***)
streams/creation.js kind='duplex' n=50000000                 ***    -11.91 %       ±2.37%  ±3.16%  ±4.11%
streams/creation.js kind='readable' n=50000000               ***    -80.02 %       ±2.12%  ±2.86%  ±3.79%
streams/creation.js kind='transform' n=50000000                *     -1.97 %       ±1.78%  ±2.37%  ±3.08%
streams/creation.js kind='writable' n=50000000                        1.92 %       ±2.67%  ±3.56%  ±4.63%
streams/pipe.js n=5000000                                     **     -4.69 %       ±3.06%  ±4.09%  ±5.34%
streams/pipe-object-mode.js n=5000000                         **     -4.48 %       ±2.77%  ±3.69%  ±4.81%
streams/readable-bigread.js n=1000                           ***     -5.48 %       ±2.72%  ±3.62%  ±4.72%
streams/readable-bigunevenread.js n=1000                             -1.30 %      ±16.12% ±21.45% ±27.92%
streams/readable-boundaryread.js type='buffer' n=2000        ***     -7.05 %       ±2.26%  ±3.00%  ±3.91%
streams/readable-boundaryread.js type='string' n=2000        ***     -4.70 %       ±1.53%  ±2.04%  ±2.66%
streams/readable-readall.js n=5000                             *      2.44 %       ±2.19%  ±2.92%  ±3.81%
streams/readable-unevenread.js n=1000                         **     -2.82 %       ±1.93%  ±2.58%  ±3.36%
streams/writable-manywrites.js n=2000000                              1.39 %       ±4.94%  ±6.57%  ±8.55%

@ronag
Copy link
Member Author

ronag commented Aug 15, 2019

Ah! I think I can run those.

I’ll try manually inlining .

@ronag
Copy link
Member Author

ronag commented Aug 15, 2019

@mscdex: After inlining, the worst case actually becomes an improvement:

full benchmarks take way to long to run...

 streams/creation.js kind='duplex' n=1000000            **      7.02 %       ±4.09% ±5.46% ±7.13%
 streams/creation.js kind='readable' n=1000000         ***     12.69 %       ±3.33% ±4.44% ±5.81%
 streams/creation.js kind='transform' n=1000000         **      4.58 %       ±3.21% ±4.28% ±5.57%
 streams/readable-bigread.js n=1000                             0.26 %       ±1.52% ±2.03%  ±2.64%
 streams/readable-bigunevenread.js n=1000              ***     13.10 %       ±6.64% ±8.83% ±11.49%

I'm a little worried whether the cognitive overhead is worth the improved performance. Should I continue work on this? I think there is more I can get out of this in terms of pref & mem.

@ronag ronag force-pushed the stream-bitfield branch 12 times, most recently from 69cc880 to f08412b Compare August 15, 2019 08:38
@ronag
Copy link
Member Author

ronag commented Aug 15, 2019

In the best case scenario this brings down the memory overhead:

from 23 (184 bytes?):

objectMode
highWaterMark
buffer
length
pipes
flowing
ended
endEmitted
reading
sync
needReadable
emittedReadable
readableListening
resumeScheduled
paused
emitClose
autoDestroy
destroyed
defaultEncoding
awaitDrain
decoder
encoding
endEmitted

to 5 (40 bytes? fits in cacheline):

bitfield
highWaterMark
buffer
length
pipes

@ronag
Copy link
Member Author

ronag commented Aug 15, 2019

@Trott: WIP tag please :)

@mcollina
Copy link
Member

I like the perf benefit, however I'm not really liking the added cognitive overhead of a bitfield. Can you try the http and fs benchmarks as well?

@ronag ronag force-pushed the stream-bitfield branch 3 times, most recently from aef156c to 6d21c85 Compare August 15, 2019 09:38
@ronag ronag force-pushed the stream-bitfield branch 5 times, most recently from 39aded2 to 01b0a49 Compare August 15, 2019 22:21
@mscdex
Copy link
Contributor

mscdex commented Aug 16, 2019

@ronag
Copy link
Member Author

ronag commented Aug 16, 2019

hah, that had quite a bit of variance

@ronag ronag closed this Aug 16, 2019
@ronag ronag reopened this Aug 16, 2019
lib/_stream_readable.js Outdated Show resolved Hide resolved
@ronag ronag force-pushed the stream-bitfield branch 3 times, most recently from e5d0c55 to d7c095d Compare August 16, 2019 17:37
@ronag
Copy link
Member Author

ronag commented Aug 16, 2019

Starting to look better:

@ronag ronag force-pushed the stream-bitfield branch 4 times, most recently from d127913 to c9ea576 Compare August 16, 2019 17:49
@mscdex
Copy link
Contributor

mscdex commented Aug 16, 2019

Starting to look better:

I don't think people would want the debug statements removed.

@ronag ronag force-pushed the stream-bitfield branch 4 times, most recently from b6d2a64 to 6151a23 Compare August 16, 2019 18:28
@ronag
Copy link
Member Author

ronag commented Aug 16, 2019

I thought it was the slow accessors that was the limiting factor. Re-added debug without slow accessors. Difference is small again:

 streams/readable-bigunevenread.js n=1000         **     13.63 %       ±8.98% ±11.95% ±15.55%

I don't think I can get much more out of this...

Although a fun exercise I'm not sure about the value of this... feel free to close

@Trott
Copy link
Member

Trott commented Aug 16, 2019

Although a fun exercise I'm not sure about the value of this... feel free to close

Closing, but feel free to re-open if you change your mind!

@Trott Trott closed this Aug 16, 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. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants