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: optimize outgoing requests #605

Closed

Conversation

brendanashworth
Copy link
Contributor

This commit does some small optimization changes on
lib/_http_outgoing.js. These include switching from while loops to
for loops, moving away from util to typeof checks, and removing
dead code.

Here is a graph of the benchmark
change (duration = 20, ran twice each). Changes aren't very visible in
smaller packets - at 32 bytes, only about 1% more requests can be sent,
but it increases as the packets increase in size; 1024 has 4.7% more
throughput than before.

Here is a graph of the same
benchmark but sorted by encoding type. As it is shown, buffers get the
biggest speed increase, then utf-8, then ASCII.

var e = this.outputEncodings.shift();
var cb = this.outputCallbacks.shift();
this.connection.write(c, e, cb);
this.output = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the previous version didn't create new arrays. you can just do this.output.length = 0;. the same for the rest of these.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see it going either way. .length is a (very magic!) setter in V8; the array.length = 0 trick might well be slower than creating a new empty array, even if the latter produces a little more garbage.

Using array.pop() here could help as well, it's normally faster than array.shift() because it needs to do less work.

EDIT: To be clear, switching to array.pop() requires that the arrays are built up in reverse order as well, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous version didn't create new arrays, instead of shift()ed off all the values - basically emptying the array, which for the new version, I simply emptied the array by making a new one.

JSPerf says that using = []; instead of .length = 0; is faster, but if you know differently I'd be open to hearing about it.

Building the arrays in reverse order by using array.unshift() would, I think, impact other benchmarks in a bad way to the point of making that optimization a de-optimization though.

@mscdex
Copy link
Contributor

mscdex commented Jan 26, 2015

FWIW some further optimizations could be done, such as caching this.outputCallbacks, this.outputEncodings, this.output, this.connection, etc. and using the cached versions inside the loop (and elsewhere in the function where it's safe).

var outputLength = this.output.length;
if (outputLength) {
for (var i = 0; i < outputLength; i++) {
var cb = this.outputCallbacks[i];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialize this variable outside the loop so it's only setting the value each time and not creating garbage, and does this cb variable need to exist at all?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean hoist the var cb outside the loop? It shouldn't matter, V8's flow analysis doesn't care. The variable as such probably doesn't even exist in the generated machine code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially just inlined cb in the function call to deal with the 80 line limit, but I just realized I could just wrap the function.
Whoops, I'll fix that.

@@ -408,7 +407,7 @@ OutgoingMessage.prototype.write = function(chunk, encoding, callback) {
return true;
}

if (!util.isString(chunk) && !util.isBuffer(chunk)) {
if (!(typeof chunk === 'string' || util.isBuffer(chunk))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might as well change util.isBuffer(chunk) to chunk instanceof Buffer while you're here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't realize that. I think there's a few other places I could pop that in, thanks.

@brendanashworth
Copy link
Contributor Author

Pushed up an --amend commit that addresses some of the nitpickings and suggestions of @bnoordhuis, @yaycmyk, and @mscdex - thanks guys - along with a few other things I found along the way.

MAJOR improvement! Now I'm getting into the ~5800 req/sec range for some, which dips down to only ~4400 req/sec on the worst ones - a major improvement on before. Graph.

@mscdex
Copy link
Contributor

mscdex commented Jan 27, 2015

👍

@@ -78,11 +78,12 @@ exports.OutgoingMessage = OutgoingMessage;
OutgoingMessage.prototype.setTimeout = function(msecs, callback) {
if (callback)
this.on('timeout', callback);
if (!this.socket) {

if (!this.socket)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is hazardous styling IMO, personally I'd prefer braces where the body of a conditional spans more than one line, even if it's effectively a one-liner, simply for clarity

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Most major style guides advocate for always having curlies to accompany conditional logic, length irrelevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually do agree with you but I changed it to match the function declaration just a few lines below. I'll update with a commit that fixes this one and the one below, too.

@quantizor
Copy link

My only other general comment is there are a lot of variables being initialized inside block scopes. I've found that moving these declarations to the topmost level (as they would be hoisted by the interpreter) makes code much more readable and you don't run into issues like accidentally re-declaring variables.

@mscdex
Copy link
Contributor

mscdex commented Jan 27, 2015

@yaycmyk True, but a linter like jshint can catch things like that no?

@quantizor
Copy link

@mscdex Sure, but it's more being courteous to the next developer than a general coding problem. Legibility > brevity IMO -- declaring everything at the onset gives you a quick birds' eye view of your function and makes it easier to grok what the important bits of data are.

var outputEncodings = this.outputEncodings;
var outputCallbacks = this.outputCallbacks;
for (var i = 0; i < outputLength; i++) {
ret = this.socket.write(output[i], outputEncodings[i],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.socket should be able to be cached too.

@brendanashworth
Copy link
Contributor Author

Pushed up another update with some hoisted variables per @yaycmyk and @mscdex, plus a few other extraneous changes.

this.socket.setTimeout(msecs);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No unrelated style changes, please. It makes the diff noisier than it should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little bit confused here, why not just keep it in this pull request? It also goes along with the change here, as spurred by this conversation. Could I keep it in?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please undo them, it complicates code archeology. The guideline is to refrain from stylistic changes unless you're updating the code that you're touching anyway.

@bnoordhuis
Copy link
Member

Left some style-related comments but the general thrust seems alright to me.

Can you post benchmark numbers? The http tests are I/O-sensitive, you may have to run them a few times to check if the numbers are stable.

@brendanashworth
Copy link
Contributor Author

Pushed up 91a6e60 to fix some of your concerns.

I actually upped the duration to twenty seconds for each test, that way I shouldn't have to redo the tests over and over again; here are the results:

HEAD (514b1d9):

http/client-request-body.js dur=20 type=asc bytes=32 method=write: 4631.16983
http/client-request-body.js dur=20 type=asc bytes=32 method=end  : 4603.73615
http/client-request-body.js dur=20 type=asc bytes=256 method=write: 4517.03899
http/client-request-body.js dur=20 type=asc bytes=256 method=end  : 4721.23036
http/client-request-body.js dur=20 type=asc bytes=1024 method=write: 4409.02940
http/client-request-body.js dur=20 type=asc bytes=1024 method=end  : 4510.25363
http/client-request-body.js dur=20 type=utf bytes=32 method=write: 4845.59965
http/client-request-body.js dur=20 type=utf bytes=32 method=end  : 5486.24442
http/client-request-body.js dur=20 type=utf bytes=256 method=write: 5454.11828
http/client-request-body.js dur=20 type=utf bytes=256 method=end  : 5426.24377
http/client-request-body.js dur=20 type=utf bytes=1024 method=write: 4979.11541
http/client-request-body.js dur=20 type=utf bytes=1024 method=end  : 4875.72046
http/client-request-body.js dur=20 type=buf bytes=32 method=write: 4989.57029
http/client-request-body.js dur=20 type=buf bytes=32 method=end  : 5021.14313
http/client-request-body.js dur=20 type=buf bytes=256 method=write: 5035.30080
http/client-request-body.js dur=20 type=buf bytes=256 method=end  : 4987.51212
http/client-request-body.js dur=20 type=buf bytes=1024 method=write: 4445.43889
http/client-request-body.js dur=20 type=buf bytes=1024 method=end  : 4451.50138

PR (91a6e60):

http/client-request-body.js dur=20 type=asc bytes=32 method=write: 5648.85466
http/client-request-body.js dur=20 type=asc bytes=32 method=end  : 5692.56884
http/client-request-body.js dur=20 type=asc bytes=256 method=write: 5824.67313
http/client-request-body.js dur=20 type=asc bytes=256 method=end  : 5842.87834
http/client-request-body.js dur=20 type=asc bytes=1024 method=write: 5088.18078
http/client-request-body.js dur=20 type=asc bytes=1024 method=end  : 4997.80228
http/client-request-body.js dur=20 type=utf bytes=32 method=write: 5767.48703
http/client-request-body.js dur=20 type=utf bytes=32 method=end  : 5878.60542
http/client-request-body.js dur=20 type=utf bytes=256 method=write: 5716.26737
http/client-request-body.js dur=20 type=utf bytes=256 method=end  : 5743.40856
http/client-request-body.js dur=20 type=utf bytes=1024 method=write: 5223.89715
http/client-request-body.js dur=20 type=utf bytes=1024 method=end  : 5051.03064
http/client-request-body.js dur=20 type=buf bytes=32 method=write: 5221.08511
http/client-request-body.js dur=20 type=buf bytes=32 method=end  : 5221.20909
http/client-request-body.js dur=20 type=buf bytes=256 method=write: 5206.54862
http/client-request-body.js dur=20 type=buf bytes=256 method=end  : 5267.09782
http/client-request-body.js dur=20 type=buf bytes=1024 method=write: 4464.81740
http/client-request-body.js dur=20 type=buf bytes=1024 method=end  : 4594.75648

@quantizor
Copy link

@brendanashworth Nice! Across the board improvements 👏

if (headers) {
var keys = Object.keys(headers);
var isArray = util.isArray(headers);
var isArray = Array.isArray(headers);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably get a further speedup from switching these to {var} instanceof Array - http://jsperf.com/array-isarray-vs-instanceof-array/5

benchmark puts it at 2x faster, since we're making micro-optimizations :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed for all three uses!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instanceof doesn't work well:

var obj = { __proto__ : [] };

console.log(obj instanceof Array); // true
console.log(Array.isArray(obj)); // false

var arr = require('vm').runInNewContext('[]');

console.log(arr instanceof Array); // false
console.log(Array.isArray(arr)); // true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted back to Array.isArray.

@brendanashworth brendanashworth force-pushed the optimize-http-outgoing branch 2 times, most recently from e571203 to af5e82a Compare January 29, 2015 23:42
@brendanashworth
Copy link
Contributor Author

Hows it looking now?

@quantizor
Copy link

Looks like it needs a rebase, code's good though!

@brendanashworth
Copy link
Contributor Author

Rebased.

@jcrugzz
Copy link

jcrugzz commented Feb 5, 2015

LGTM. Love the perf direction here 👍

this._buffer(data, encoding, callback);
return false;
var outputLength = this.output.length;
if (outputLength) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I speculate that using outputLength > 0 here will be infinitesimally faster.

@brendanashworth
Copy link
Contributor Author

Nit picks should be fixed, @bnoordhuis .

@@ -487,7 +492,7 @@ OutgoingMessage.prototype.end = function(data, encoding, callback) {
encoding = null;
}

if (data && typeof data !== 'string' && !(data instanceof Buffer)) {
if (data && !(typeof data === 'string' || data instanceof Buffer)) {
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 undo these changes as well, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was less of a style change and more of an optimization, leaving the data instanceof Buffer to not be called if it was a string. Would you still like them removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's worth optimizing for the uncommon case ('bug case' really) but I'll leave that at your discretion. I do think it's a little harder to read now, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely harder to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll revert this and the other like line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like me to also change this line?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please.

@bnoordhuis
Copy link
Member

This commit does some small optimization changes on
`lib/_http_outgoing.js`. These include switching from `while` loops to
`for` loops, moving away from `util` to `typeof` checks, and removing
dead code. It also includes variable caches to avoid lookups and
generic style changes. All in all, much faster execution.

It gets an across the board increase in req/sec on the benchmarks,
from my experience about a 10% increase.
@brendanashworth
Copy link
Contributor Author

!(a || b) changed back to (!a && !b), @bnoordhuis.

@brendanashworth
Copy link
Contributor Author

Bumpity bump. Hows it looking?

@bnoordhuis
Copy link
Member

LGTM

3 similar comments
@micnic
Copy link
Contributor

micnic commented Feb 23, 2015

LGTM

@tellnes
Copy link
Contributor

tellnes commented Mar 4, 2015

LGTM

@mscdex
Copy link
Contributor

mscdex commented Mar 4, 2015

LGTM

brendanashworth added a commit that referenced this pull request Mar 4, 2015
This commit does some small optimization changes on
`lib/_http_outgoing.js`. These include switching from `while` loops to
`for` loops, moving away from `util` to `typeof` checks, and removing
dead code. It also includes variable caches to avoid lookups and
generic style changes. All in all, much faster execution.

It gets an across the board increase in req/sec on the benchmarks,
from my experience about a 10% increase.

PR-URL: #605
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Christian Vaagland Tellnes <christian@tellnes.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@brendanashworth
Copy link
Contributor Author

Closed in 08133f4. Thanks.

@rvagg rvagg mentioned this pull request Mar 4, 2015
@brendanashworth brendanashworth deleted the optimize-http-outgoing branch March 8, 2015 20:12
@ChALkeR ChALkeR added http Issues or PRs related to the http subsystem. performance Issues and PRs related to the performance of Node.js. labels Feb 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.