{"action":"created","issue":{"url":"https://api.github.com/repos/joyent/node/issues/9026","labels_url":"https://api.github.com/repos/joyent/node/issues/9026/labels{/name}","comments_url":"https://api.github.com/repos/joyent/node/issues/9026/comments","events_url":"https://api.github.com/repos/joyent/node/issues/9026/events","html_url":"https://github.com/joyent/node/pull/9026","id":54231890,"number":9026,"title":"http: fix >90% performance regression for GET requests using response.end","user":{"login":"CGavrila","id":5712534,"avatar_url":"https://avatars.githubusercontent.com/u/5712534?v=3","gravatar_id":"","url":"https://api.github.com/users/CGavrila","html_url":"https://github.com/CGavrila","followers_url":"https://api.github.com/users/CGavrila/followers","following_url":"https://api.github.com/users/CGavrila/following{/other_user}","gists_url":"https://api.github.com/users/CGavrila/gists{/gist_id}","starred_url":"https://api.github.com/users/CGavrila/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/CGavrila/subscriptions","organizations_url":"https://api.github.com/users/CGavrila/orgs","repos_url":"https://api.github.com/users/CGavrila/repos","events_url":"https://api.github.com/users/CGavrila/events{/privacy}","received_events_url":"https://api.github.com/users/CGavrila/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/joyent/node/labels/pr","name":"pr","color":"006b75"},{"url":"https://api.github.com/repos/joyent/node/labels/v0.10","name":"v0.10","color":"207de5"}],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/joyent/node/milestones/29","html_url":"https://github.com/joyent/node/milestones/0.10.37","labels_url":"https://api.github.com/repos/joyent/node/milestones/29/labels","id":947293,"number":29,"title":"0.10.37","description":"","creator":{"login":"misterdjules","id":811047,"avatar_url":"https://avatars.githubusercontent.com/u/811047?v=3","gravatar_id":"","url":"https://api.github.com/users/misterdjules","html_url":"https://github.com/misterdjules","followers_url":"https://api.github.com/users/misterdjules/followers","following_url":"https://api.github.com/users/misterdjules/following{/other_user}","gists_url":"https://api.github.com/users/misterdjules/gists{/gist_id}","starred_url":"https://api.github.com/users/misterdjules/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/misterdjules/subscriptions","organizations_url":"https://api.github.com/users/misterdjules/orgs","repos_url":"https://api.github.com/users/misterdjules/repos","events_url":"https://api.github.com/users/misterdjules/events{/privacy}","received_events_url":"https://api.github.com/users/misterdjules/received_events","type":"User","site_admin":false},"open_issues":6,"closed_issues":1,"state":"open","created_at":"2015-01-22T18:51:11Z","updated_at":"2015-03-03T00:25:38Z","due_on":null,"closed_at":null},"comments":13,"created_at":"2015-01-13T18:50:53Z","updated_at":"2015-03-04T23:32:36Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/joyent/node/pulls/9026","html_url":"https://github.com/joyent/node/pull/9026","diff_url":"https://github.com/joyent/node/pull/9026.diff","patch_url":"https://github.com/joyent/node/pull/9026.patch"},"body":"## Problem \r\n\r\nThis 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:\r\n\r\n```\r\nvar http = require('http');\r\nvar body = new Buffer('Hello World');\r\nhttp.createServer(function(req, res) {\r\n  res.writeHead(200); // or: `res.statusCode = 200;`\r\n  res.end(body); // change this to `res.end();` to see the expected speed post-1fddc1f\r\n}).listen(3333);\r\n```\r\n```\r\nwrk 'http://localhost:3333/' -d 3 -c 50 -t 4 | grep 'Requests/sec'\r\n```\r\n\r\nThe problem comes from commit 1fddc1f, in particular from the following lines, which are aimed at HEAD requests:\r\n```\r\n// Transfer-encoding: chunked responses to HEAD requests\r\nif (this._hasBody && this.chunkedEncoding)\r\n   hot = false; // This will never execute for HEAD requests, only GET\r\n```\r\n\r\nHowever, the `hot = false;` instruction will never execute for HEAD requests, since `this._hasBody` is always `false` because of line [lib/http.js:1073](https://github.com/joyent/node/blob/1fddc1fee84785d296e37337dcb0dc12c13a049f/lib/http.js#L1073), 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()`.\r\n\r\n## Fix\r\n\r\nThe fix is simply to remove the lines mentioned above:\r\n```\r\n- // Transfer-encoding: chunked responses to HEAD requests\r\n- if (this._hasBody && this.chunkedEncoding)\r\n-   hot = false;\r\n```\r\n\r\nAs 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).\r\n\r\n## Testing\r\n\r\nAs far as I can tell, there are three conditions which need to be met in order for the fix to be considered safe:\r\n1. Performance should be at pre-1fddc1f levels for the script in #8940.\r\n2. Pass all of the node tests, which also includes the one which came packaged with 1fddc1f.\r\n3. As a bonus, make sure that #8361 is still fixed.\r\n\r\nFor no. 1, I have tested that the performance is at pre-1fddc1 levels after deleting those three lines.\r\n```\r\nOriginal\r\nwrk 'http://localhost:3333/' -d 3 -c 50 -t 4 | grep 'Requests/sec' | awk '{ print \"  \" $2 }'\r\n1184.48 \r\n\r\nWith fix \r\nwrk 'http://localhost:3333/' -d 3 -c 50 -t 4 | grep 'Requests/sec' | awk '{ print \"  \" $2 }'\r\n18378.13\r\n```\r\n\r\nFor no. 2, I have made sure that the Node tests run/pass in the same way with/without the fix. \r\n\r\nFor no. 3, I have tried those scripts, everything is fine.\r\n```\r\nstatus: 404\r\n{ 'transfer-encoding': 'chunked',\r\n  date: 'Tue, 13 Jan 2015 17:10:21 GMT',\r\n  connection: 'close' }\r\nend\r\n```"},"comment":{"url":"https://api.github.com/repos/joyent/node/issues/comments/77274259","html_url":"https://github.com/joyent/node/pull/9026#issuecomment-77274259","issue_url":"https://api.github.com/repos/joyent/node/issues/9026","id":77274259,"user":{"login":"misterdjules","id":811047,"avatar_url":"https://avatars.githubusercontent.com/u/811047?v=3","gravatar_id":"","url":"https://api.github.com/users/misterdjules","html_url":"https://github.com/misterdjules","followers_url":"https://api.github.com/users/misterdjules/followers","following_url":"https://api.github.com/users/misterdjules/following{/other_user}","gists_url":"https://api.github.com/users/misterdjules/gists{/gist_id}","starred_url":"https://api.github.com/users/misterdjules/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/misterdjules/subscriptions","organizations_url":"https://api.github.com/users/misterdjules/orgs","repos_url":"https://api.github.com/users/misterdjules/repos","events_url":"https://api.github.com/users/misterdjules/events{/privacy}","received_events_url":"https://api.github.com/users/misterdjules/received_events","type":"User","site_admin":false},"created_at":"2015-03-04T23:32:36Z","updated_at":"2015-03-04T23:32:36Z","body":"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."}}