http: fix >90% performance regression for GET requests using response.end

[CGavrila]

Problem

This is an attempt to fix issue #8940, which describes a performance regression which started with commit 1fddc1f; more specifically, the number of requests per second dropped from ~18k to ~1.1k (on my machine) in between v0.10.31 and v0.10.32. This can be tested easily as described in issue #8940:

var http = require('http');
var body = new Buffer('Hello World');
http.createServer(function(req, res) {
  res.writeHead(200); // or: `res.statusCode = 200;`
  res.end(body); // change this to `res.end();` to see the expected speed post-1fddc1f
}).listen(3333);

wrk 'http://localhost:3333/' -d 3 -c 50 -t 4 | grep 'Requests/sec'

The problem comes from commit 1fddc1f, in particular from the following lines, which are aimed at HEAD requests:

// Transfer-encoding: chunked responses to HEAD requests
if (this._hasBody && this.chunkedEncoding)
   hot = false; // This will never execute for HEAD requests, only GET

However, the hot = false; instruction will never execute for HEAD requests, since this._hasBody is always false because of line lib/http.js:1073, but it will execute for all GET requests since the conditions will match. Therefore, hot-path optimization will never be done for GET requests when sending data via response.end().

Fix

The fix is simply to remove the lines mentioned above:

- // Transfer-encoding: chunked responses to HEAD requests
- if (this._hasBody && this.chunkedEncoding)
-   hot = false;

As previously explained, this executes only for GET requests, not for HEAD requests, as it was intended. Simply removing them will allow GET requests to have the hot-path optimization when response.end(...); is not empty, while HEAD requests will be non-optimized since there is no need (i.e no body).

Testing

As far as I can tell, there are three conditions which need to be met in order for the fix to be considered safe:

1. Performance should be at pre-1fddc1f levels for the script in Significant performance regression in http in v0.10.32+ #8940.
2. Pass all of the node tests, which also includes the one which came packaged with 1fddc1f.
3. As a bonus, make sure that Parse Error: HPE_INVALID_CONSTANT on Transfer-Encoding response to HEAD request #8361 is still fixed.

For no. 1, I have tested that the performance is at pre-1fddc1 levels after deleting those three lines.

Original
wrk 'http://localhost:3333/' -d 3 -c 50 -t 4 | grep 'Requests/sec' | awk '{ print "  " $2 }'
1184.48 

With fix 
wrk 'http://localhost:3333/' -d 3 -c 50 -t 4 | grep 'Requests/sec' | awk '{ print "  " $2 }'
18378.13

For no. 2, I have made sure that the Node tests run/pass in the same way with/without the fix.

For no. 3, I have tried those scripts, everything is fine.

status: 404
{ 'transfer-encoding': 'chunked',
  date: 'Tue, 13 Jan 2015 17:10:21 GMT',
  connection: 'close' }
end

[evantorrie]

+1. This confused me for a long time as to why our regular benchmarking in our pipeline dropped precipitously after 0.10.32.

[mscdex]

+100

[evantorrie]

I believe the correct fix is more likely

// Transfer-encoding: chunked responses to HEAD requests
if (!this._hasBody && this.chunkedEncoding)
   hot = false; 

In terms of request/second, my results are even worse (on RHEL 7.0). There seems to be a fixed 40ms delay when it goes through the flush routine.

Build 6e689ec

[01:07] evant@oxy-oxygen-0a58abcf:wrk$ ./wrk -d 60 -c 4 -t 1 --latency http://localhost:8080/
Running 1m test @ http://localhost:8080/
  1 threads and 4 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   553.71us  131.18us   6.74ms   78.04%
    Req/Sec     7.66k   614.91    10.22k    63.76%
  Latency Distribution
     50%  547.00us
     75%  597.00us
     90%  650.00us
     99%    0.92ms
  435095 requests in 1.00m, 64.73MB read
Requests/sec:   7251.64
Transfer/sec:      1.08MB

Build 1fddc1f

[01:08] evant@oxy-oxygen-0a58abcf:wrk$ ./wrk -d 60 -c 4 -t 1 --latency http://localhost:8080/
Running 1m test @ http://localhost:8080/
  1 threads and 4 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    40.00ms  196.97us  44.03ms   99.03%
    Req/Sec   100.03      7.01   135.00     83.40%
  Latency Distribution
     50%   39.99ms
     75%   40.00ms
     90%   40.03ms
     99%   40.10ms
  6000 requests in 1.00m, 0.89MB read
Requests/sec:     99.98
Transfer/sec:     15.24KB

Build 1fddc1f with following patch applied:

diff --git a/lib/http.js b/lib/http.js
index f87df17..a81ab11 100644
--- a/lib/http.js
+++ b/lib/http.js
@@ -946,7 +946,7 @@ OutgoingMessage.prototype.end = function(data, encoding) {
     hot = false;

   // Transfer-encoding: chunked responses to HEAD requests
-  if (this._hasBody && this.chunkedEncoding)
+  if (!this._hasBody && this.chunkedEncoding)
     hot = false;

   if (hot) {

[01:27] evant@oxy-oxygen-0a58abcf:wrk$ ./wrk -d 60 -c 4 -t 1 --latency http://localhost:8080/
Running 1m test @ http://localhost:8080/
  1 threads and 4 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   564.04us  147.25us   5.19ms   75.74%
    Req/Sec     7.50k   657.24     8.56k    68.40%
  Latency Distribution
     50%  553.00us
     75%  619.00us
     90%  700.00us
     99%    0.96ms
  425732 requests in 1.00m, 63.34MB read
Requests/sec:   7095.55
Transfer/sec:      1.06MB
[01:28] evant@oxy-oxygen-0a58abcf:wrk$

[mscdex]

@evantorrie Yeah, that's the change I had suggested in #8940 but I wasn't sure if it was 100% correct or not.

[evantorrie]

For completeness, here's the source of the server that the wrk benchmarks were testing:

var http = require('http');

http.createServer(function (req, res) {
  res.writeHead(200, {'Content-Type': 'text/plain'});
  res.end('Hello World\n');
}).listen(8080);

[trevnorris]

I'm fine with this.

/cc @tjfontaine @misterdjules @cjihrig

[cjihrig]

+1 as long as the tests pass

[misterdjules]

@trevnorris @cjihrig What change do you agree with, the one mentioned by @evantorrie in his comment or the one from @CGavrila's PR?

[misterdjules]

@joyent/node-coreteam Added to the 0.10.37 milestone as it seems to be a significant performance issue for which we seem to have a good candidate fix.

[cjihrig]

I was originally referring to the changes in this PR. If @evantorrie's changes also work, but are faster, then I'd prefer that.

[mhdawson]

It has been a while, but what I remember from when Cristian and I worked on this, is that we identified that this._hasBody is false for a HEAD and true for a GET. At the same time we also identified that if this._hasBody is false then hot is already false so the lines are unnecessary. So we believe that removing the lines only affects the case that needs to be fixed and felt that the fewer the lines the better.

[trevnorris]

@misterdjules I was also referring to the change in this PR.

[misterdjules]

Thank you for the clarifications! LGTM, running http benchmarks currently and tests on all platforms to make sure there's no regression. I will report results here.

[misterdjules]

Benchmarks results available and no regression found on UNICES and on Windows.

[misterdjules]

@trevnorris I'm not sure how to interpret these benchmark results, are you familiar with them?

[trevnorris]

@misterdjules yeah. those results are a pain to read though, and overall I don't find them super reliable. I've tested it myself and it seems to help.

[misterdjules]

@trevnorris Sounds good, I just wanted to make sure we didn't find anything suspicious in these benchmarks. Do you mind landing this?

Thank you @CGavrila, @evantorrie and everyone who helped!

[CGavrila]

Thank you, @misterdjules and the team, for the work done to validate this!

[misterdjules]

Thank you everyone, landed in 8bcd0a4!