Skip to content

Conversation

@ronag
Copy link
Member

@ronag ronag commented Jul 23, 2021

This is an alternative variant solving some problems with the previous PR.

  • How to return Node stream?
  • Breaking change.

It's a little hacky but I think it can work out. Thoughts?

@ronag ronag requested a review from mcollina July 23, 2021 06:43
@ronag
Copy link
Member Author

ronag commented Jul 23, 2021

@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2021

Codecov Report

Merging #898 (50b7d49) into main (02a9d13) will decrease coverage by 1.91%.
The diff coverage is 20.75%.

❗ Current head 50b7d49 differs from pull request most recent head e0e0d84. Consider uploading reports for the commit e0e0d84 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #898      +/-   ##
==========================================
- Coverage   99.57%   97.66%   -1.92%     
==========================================
  Files          26       27       +1     
  Lines        2132     2184      +52     
==========================================
+ Hits         2123     2133      +10     
- Misses          9       51      +42     
Impacted Files Coverage Δ
lib/api/readable.js 19.23% <19.23%> (ø)
lib/api/api-request.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02a9d13...e0e0d84. Read the comment docs.

@ronag ronag force-pushed the node-stream2 branch 13 times, most recently from 4f1c2ad to 5370b6c Compare July 23, 2021 08:41
@ronag ronag added the enhancement New feature or request label Jul 23, 2021
@ronag ronag requested review from Ethan-Arrowood and dnlup July 23, 2021 08:43
@ronag ronag force-pushed the node-stream2 branch 4 times, most recently from c43d516 to 344bae6 Compare July 23, 2021 08:52
@ronag ronag force-pushed the node-stream2 branch 5 times, most recently from 41dd0a6 to 7068f35 Compare July 23, 2021 10:35
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

this seems less breaking. Does it wreck our perf?

@ronag
Copy link
Member Author

ronag commented Jul 25, 2021

Perf seems same:

main

[bench:run]  Tests                Samples         Result  Tolerance  Difference with slowest 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  http - no keepalive       10   6.34 req/sec   ± 0.65 %                        - 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  http - keepalive          10   7.06 req/sec   ± 1.34 %                + 11.30 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - pipeline         10  64.81 req/sec   ± 1.02 %               + 922.39 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - stream           10  68.68 req/sec   ± 1.76 %               + 983.36 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - dispatch         10  68.86 req/sec   ± 1.53 %               + 986.29 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - request          10  70.51 req/sec   ± 2.01 %              + 1012.25 % 

PR

[bench:run]  Tests                Samples         Result  Tolerance  Difference with slowest 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  http - no keepalive       10   6.34 req/sec   ± 0.72 %                        - 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  http - keepalive          10   7.00 req/sec   ± 1.41 %                + 10.48 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - pipeline         10  63.50 req/sec   ± 0.90 %               + 902.15 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - stream           10  66.57 req/sec   ± 1.58 %               + 950.58 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - request          10  70.15 req/sec   ± 2.18 %              + 1007.15 % 
[bench:run] |─────────────────────|─────────|───────────────|───────────|─────────────────────────|
[bench:run]  undici - dispatch         10  72.62 req/sec   ± 1.55 %              + 1046.09 % 

@ronag ronag requested a review from mcollina July 25, 2021 10:08
@ronag
Copy link
Member Author

ronag commented Jul 25, 2021

I've reduced the ambition here. Remove the hacky performance part and focus on API and functionality to begin with.

@ronag
Copy link
Member Author

ronag commented Jul 25, 2021

Also opened against node core. Waiting for feedback there as well.

@ronag
Copy link
Member Author

ronag commented Jul 28, 2021

I think it's likely we will land a better version in node core. nodejs/node#39520

@ronag ronag closed this Jul 28, 2021
@Uzlopak Uzlopak deleted the node-stream2 branch February 26, 2024 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants