Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
http: there is no corked property of stream#18325
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Do not check/use unexistent property, use `OutgoingMessage` instead.
indutny commented Jan 23, 2018
cc @nodejs/http |
indutny commented Jan 23, 2018
@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 commented Jan 23, 2018
indutny commented Jan 24, 2018
@joyeecheung I'm really newbie when it comes to benchmarks, but how long does it usually take to complete? |
lpinca commented Jan 24, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
http benchmarks take days to complete 😄 |
joyeecheung commented Jan 24, 2018
@indutny The majority of time spent on HTTP benchmarks are on |
AndreasMadsen commented Jan 25, 2018
@joyeecheung Yep, see #18003 (comment) |
indutny commented Jan 25, 2018
@joyeecheung looks like benchmarks are completed. Is there any way to format it nicely? Thanks! |
joyeecheung commented Jan 25, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
See benchmark resultsSee significant benchmark resultsAlso, see https://gist.github.com/joyeecheung/b55c88ee465d552d7ed1dc34e67305ea on how to grab the results (hmm, looks like a good candidate of node-core-utils) |
indutny commented Jan 25, 2018
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. |
indutny commented Jan 27, 2018
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]>
MylesBorins commented Feb 27, 2018
Should we land this on LTS? It lands cleanly on 8.x, but would need to be backproted for 6.x |
indutny commented Feb 27, 2018
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
OutgoingMessageinstead.