-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: there is no corked
property of stream
#18325
http: there is no corked
property of stream
#18325
Conversation
Do not check/use unexistent property, use `OutgoingMessage` instead.
cc @nodejs/http |
@nodejs/benchmarking could you help me benchmark this vs current master? I've a feeling that we might be sitting on a performance improvement here. |
@joyeecheung I'm really newbie when it comes to benchmarks, but how long does it usually take to complete? |
http benchmarks take days to complete 😄 |
@indutny The majority of time spent on HTTP benchmarks are on |
@joyeecheung Yep, see #18003 (comment) |
@joyeecheung looks like benchmarks are completed. Is there any way to format it nicely? Thanks! |
See benchmark results
See significant benchmark results
Also, see https://gist.github.com/joyeecheung/b55c88ee465d552d7ed1dc34e67305ea on how to grab the results (hmm, looks like a good candidate of node-core-utils) |
Thanks! Looks like benchmarks are inconclusive. Some are 10% faster, some are 10% slower. Unless there're any objections to this, I'd like to land for the sake of correctness of code. |
Landed in f29c2cb, thank you! |
Do not check/use unexistent property, use `OutgoingMessage` instead. PR-URL: #18325 Reviewed-By: Mithun Sasidharan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Do not check/use unexistent property, use `OutgoingMessage` instead. PR-URL: #18325 Reviewed-By: Mithun Sasidharan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Should we land this on LTS? It lands cleanly on 8.x, but would need to be backproted for 6.x |
I wouldn't bother. |
Do not check/use unexistent property, use `OutgoingMessage` instead. PR-URL: nodejs#18325 Reviewed-By: Mithun Sasidharan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
http
Do not check/use unexistent property, use
OutgoingMessage
instead.