-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore: http/2 benchmark #35
Conversation
* chore: add http2 alpn test using fastify * chore: update to test https 1 with http2 * chore: update alpn test to return server request alpn protocol and http version * chore: add alpn with body * fix: remove fastify from package json
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## http2 #35 +/- ##
==========================================
+ Coverage 93.19% 93.29% +0.10%
==========================================
Files 41 41
Lines 3144 3147 +3
==========================================
+ Hits 2930 2936 +6
+ Misses 214 211 -3
☔ View full report in Codecov by Sentry. |
The results indeed looks promising, though if the streams are better managed, the things can go even better as we'll be reusing them to the same origin. |
@metcoder95 Thank you! Looking forward to seeing the main PR in a mergable state. We 100% need more tests too especially around the closing of connections and sessions for the server and the client. |
Hey @mkaufmaner, can you rebase with the branch and enable the benchmarks? |
…nchmark # Conflicts: # lib/client.js # lib/core/connect.js # test/http2.js
Merged down from your http2 branch, but I am not sure what you mean by "enable the benchmarks". Could you elaborate? Secondly, I updated the benchmarks with Nonetheless,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate?
Sure, I was meaning those that were commented out
Secondly, I updated the benchmarks with allowH2: true, enabled the dispatch benchmark, and reran the http2 benchmark. I am still encountering the same issue before where sometimes the benchmark just doesn't print anything so you have to run it in succession to get a result.
Those are good numbers, thanks! 🙇
I'm not sure what the error might be, I'm guessing that is due to how the request is done, as they differ from the way undici
usually works, although not really important if we can get the numbers
Would you like to go further and do the cleanup to merge?
Cleaned up the leftover comments so this should be ready to go. |
Linter seems failing :( |
Fixed the lint errors. |
Co-authored-by: Carlos Fuentes <me@metcoder.dev>
Co-authored-by: Carlos Fuentes <me@metcoder.dev>
Start the server:
Run the benchmark:
Note: May have to run
benchmark-http2.js
multiple times, and in quick succession, in order to get a result.Result: