-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
stream: bitfield #29127
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
Conversation
|
I need a little help/guidance on how to do or create relevant benchmarks. |
|
Haven't quite sorted out |
ada2af9 to
5a16355
Compare
|
If this is accepted we should probably do the same for |
|
Results don't look so good: |
|
Ah! I think I can run those. I’ll try manually inlining . |
|
@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. |
69cc880 to
f08412b
Compare
|
In the best case scenario this brings down the memory overhead: from 23 (184 bytes?): to 5 (40 bytes? fits in cacheline): |
|
@Trott: WIP tag please :) |
|
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? |
aef156c to
6d21c85
Compare
39aded2 to
01b0a49
Compare
|
hah, that had quite a bit of variance |
e5d0c55 to
d7c095d
Compare
|
|
d127913 to
c9ea576
Compare
I don't think people would want the debug statements removed. |
b6d2a64 to
6151a23
Compare
|
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 |
Closing, but feel free to re-open if you change your mind! |
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:
- [ ] use localbitfieldinstance to avoid property access throughstate.Refs: #29126
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes