-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: send Content-Length when possible #1062
Conversation
Do you have performance numbers on how this affects things? I would guess it would be faster since it's not having to do extra encoding.... |
I've run the benchmark for http, but I'm not that familiar with the inner workings of those. This is run on my MBP. I do not know how sensitive those are to me surfing Github while it was running. https://gist.github.com/tellnes/05012feaede66610cf24 It looks like something is going up while other things is going down. Maybe someone with more experience in this area can shim in? cc @iojs/collaborators |
You might do something like this from the project root that contains the new changes:
(where |
this._contentLength = data.length; | ||
} else { | ||
this._contentLength = 0; | ||
} |
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.
Can you create the _contentLength field in the OutgoingMessage constructor? Adding properties post facto causes a hidden class transition.
EDIT: Sorry, scratch that. I see you're already doing that.
LGTM, no comments. |
nice work, LGTM |
Here is some numbers: https://gist.github.com/tellnes/44f09e07ac28acf8fc32 |
+1 |
LGTM |
This changes the behavior for http to send send a Content-Length header instead of using chunked encoding when we know the size of the body when sending the headers. Fixes: nodejs#1044 PR-URL: nodejs#1062 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Brian White <mscdex@mscdex.net>
1e0e702
to
4874182
Compare
This changes the behavior for http to send send a
Content-Length
header instead of using chunked encoding when we know the size of the body when sending the headers.There is tree commits here. The first one is the initial implementation. The second also sends
Content-Length: 0
when there is no body. The last reorganizes some code. I'll squash them before merge.This broke a lot less tests than I feared.
Fixes #1044