-
Notifications
You must be signed in to change notification settings - Fork 30k
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
http: refactor to use more primordials #36194
Conversation
Review requested:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/701/ (queued, will 404 until it starts) |
The benchmark CI didn't kick off. I'd say we should wait for #36221 to land, and then run the benchmark to see how it will actually perform. |
Be very selective (explicitly set benchmark parameters) when running http benchmarks, otherwise they will take way too long to finish because of the fixed duration of a single run and because many of the http benchmarks contain a fair number of parameters and parameter values (all multiplied by x number of runs). |
Whoops, I goofed and typed "http" in the wrong place in the parameters so the benchmark didn't run.... As @mscdex notes, we're not going to want to run all the benchmarks anyway. (Or maybe we are, but then be prepared to be patient, and maybe get the Build WG to let two benchmarks run at once so that it doesn't block all other benchmarks while it's running for days.) |
a9eb62b
to
f7e6581
Compare
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/805/ I'm splitting it into several 5-run benchmarks, then we can concat the results. |
f7e6581
to
943cd31
Compare
The benchmark CIs didn't show any significant regression, I'm marking this as author ready, and I plan to land this tomorrow.
|
PR-URL: nodejs#36194 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
943cd31
to
8cf5ae0
Compare
Landed in 8cf5ae0 |
hey @aduh95, this PR does not land cleanly on v15.x. Would you be able to backport? |
PR-URL: nodejs#36194 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: #36194 Backport-PR-URL: #36803 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes