Skip to content
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

Merged
merged 1 commit into from
Mar 5, 2015

Conversation

tellnes
Copy link
Contributor

@tellnes tellnes commented Mar 4, 2015

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

@mscdex
Copy link
Contributor

mscdex commented Mar 5, 2015

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....

@tellnes
Copy link
Contributor Author

tellnes commented Mar 5, 2015

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

@mscdex
Copy link
Contributor

mscdex commented Mar 5, 2015

You might do something like this from the project root that contains the new changes:

./iojs benchmark/compare.js -r -g ./iojs ./iojs-prev -- http

(where ./iojs-prev is the path to a binary before the changes -- I usually use a full path including the /out/Release/iojs part to make sure a symlink doesn't mistakenly point to the new executable). This will make the output easier to compare.

this._contentLength = data.length;
} else {
this._contentLength = 0;
}
Copy link
Member

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.

@bnoordhuis
Copy link
Member

LGTM, no comments.

@rvagg
Copy link
Member

rvagg commented Mar 5, 2015

nice work, LGTM

@tellnes
Copy link
Contributor Author

tellnes commented Mar 5, 2015

Here is some numbers: https://gist.github.com/tellnes/44f09e07ac28acf8fc32

@mikeal
Copy link
Contributor

mikeal commented Mar 5, 2015

+1

@mscdex
Copy link
Contributor

mscdex commented Mar 5, 2015

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>
@tellnes tellnes merged commit 4874182 into nodejs:v1.x Mar 5, 2015
@tellnes tellnes deleted the http-content-length branch March 5, 2015 21:22
@rvagg rvagg mentioned this pull request Mar 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants