http: optimize outgoing requests #605

brendanashworth · 2015-01-26T01:56:41Z

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.

zertosh · 2015-01-26T04:59:50Z

lib/_http_outgoing.js

-      var e = this.outputEncodings.shift();
-      var cb = this.outputCallbacks.shift();
-      this.connection.write(c, e, cb);
+      this.output = [];


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

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.

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 · 2015-01-26T06:34:13Z

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

quantizor · 2015-01-26T08:29:34Z

lib/_http_outgoing.js

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


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?

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.

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.

bnoordhuis · 2015-01-26T09:41:04Z

lib/_http_outgoing.js

@@ -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))) {


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

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

brendanashworth · 2015-01-27T06:05:54Z

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 · 2015-01-27T06:14:29Z

👍

rvagg · 2015-01-27T06:14:36Z

lib/_http_outgoing.js

@@ -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)


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

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

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 · 2015-01-27T06:19:05Z

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 · 2015-01-27T06:22:43Z

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

quantizor · 2015-01-27T06:28:23Z

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

mscdex · 2015-01-27T14:58:59Z

lib/_http_outgoing.js

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


this.socket should be able to be cached too.

brendanashworth · 2015-01-28T06:52:38Z

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

bnoordhuis · 2015-01-28T16:27:17Z

lib/_http_outgoing.js

    this.socket.setTimeout(msecs);
+  }


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

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?

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 · 2015-01-28T16:38:54Z

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 · 2015-01-29T04:03:58Z

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 · 2015-01-29T06:18:59Z

@brendanashworth Nice! Across the board improvements 👏

quantizor · 2015-01-29T06:23:55Z

lib/_http_outgoing.js

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


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 :)

Changed for all three uses!

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

Reverted back to Array.isArray.

brendanashworth · 2015-01-31T23:49:08Z

Hows it looking now?

quantizor · 2015-02-04T07:09:39Z

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

brendanashworth · 2015-02-05T04:58:08Z

Rebased.

jcrugzz · 2015-02-05T23:21:17Z

LGTM. Love the perf direction here 👍

bnoordhuis · 2015-02-06T20:56:45Z

lib/_http_outgoing.js

-        this._buffer(data, encoding, callback);
-        return false;
+    var outputLength = this.output.length;
+    if (outputLength) {


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

brendanashworth · 2015-02-07T02:13:57Z

Nit picks should be fixed, @bnoordhuis .

bnoordhuis · 2015-02-07T11:47:36Z

lib/_http_outgoing.js

@@ -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)) {


Can you undo these changes as well, please?

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?

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.

Definitely harder to read.

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

Would you like me to also change this line?

Yes, please.

bnoordhuis · 2015-02-08T02:46:21Z

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/157/

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 · 2015-02-10T02:01:06Z

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

brendanashworth · 2015-02-22T02:34:01Z

Bumpity bump. Hows it looking?

bnoordhuis · 2015-02-22T05:50:42Z

LGTM

micnic · 2015-02-23T08:58:32Z

LGTM

tellnes · 2015-03-04T04:59:09Z

LGTM

mscdex · 2015-03-04T05:48:24Z

LGTM

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 · 2015-03-04T05:58:50Z

Closed in 08133f4. Thanks.

zertosh reviewed Jan 26, 2015
View reviewed changes

quantizor reviewed Jan 26, 2015
View reviewed changes

micnic mentioned this pull request Jan 26, 2015

replace util.isType() with typeof #607

Closed

bnoordhuis reviewed Jan 26, 2015
View reviewed changes

brendanashworth force-pushed the optimize-http-outgoing branch from d0b9600 to b3a7a2a Compare January 27, 2015 06:01

rvagg reviewed Jan 27, 2015
View reviewed changes

mscdex reviewed Jan 27, 2015
View reviewed changes

brendanashworth force-pushed the optimize-http-outgoing branch from b3a7a2a to ff8563a Compare January 28, 2015 06:51

bnoordhuis reviewed Jan 28, 2015
View reviewed changes

brendanashworth mentioned this pull request Jan 29, 2015

lib: reduce util.is*() usage #647

Closed

brendanashworth force-pushed the optimize-http-outgoing branch from ff8563a to 91a6e60 Compare January 29, 2015 03:25

quantizor reviewed Jan 29, 2015
View reviewed changes

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

brendanashworth force-pushed the optimize-http-outgoing branch from af5e82a to f5f6550 Compare February 5, 2015 04:56

bnoordhuis reviewed Feb 6, 2015
View reviewed changes

brendanashworth force-pushed the optimize-http-outgoing branch from f5f6550 to f763504 Compare February 7, 2015 02:10

bnoordhuis reviewed Feb 7, 2015
View reviewed changes

brendanashworth force-pushed the optimize-http-outgoing branch from f763504 to 5ff7d4f Compare February 7, 2015 19:50

brendanashworth force-pushed the optimize-http-outgoing branch from 5ff7d4f to 8742b8a Compare February 10, 2015 01:59

brendanashworth closed this Mar 4, 2015

rvagg mentioned this pull request Mar 4, 2015

Release proposal: v1.5.0 #1060

Closed

brendanashworth deleted the optimize-http-outgoing branch March 8, 2015 20:12

Fishrock123 mentioned this pull request May 5, 2015

repl require() broken in iojs 2.x #1633

Closed

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

aliscco mentioned this pull request Jun 21, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#487

Open

http: optimize outgoing requests #605

http: optimize outgoing requests #605

Conversation

brendanashworth commented Jan 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jan 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanashworth commented Jan 27, 2015

mscdex commented Jan 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quantizor commented Jan 27, 2015

mscdex commented Jan 27, 2015

quantizor commented Jan 27, 2015

Choose a reason for hiding this comment

brendanashworth commented Jan 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Jan 28, 2015

brendanashworth commented Jan 29, 2015

quantizor commented Jan 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanashworth commented Jan 31, 2015

quantizor commented Feb 4, 2015

brendanashworth commented Feb 5, 2015

jcrugzz commented Feb 5, 2015

Choose a reason for hiding this comment

brendanashworth commented Feb 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Feb 8, 2015

brendanashworth commented Feb 10, 2015

brendanashworth commented Feb 22, 2015

bnoordhuis commented Feb 22, 2015

micnic commented Feb 23, 2015

tellnes commented Mar 4, 2015

mscdex commented Mar 4, 2015

brendanashworth commented Mar 4, 2015