http: improve ClientRequest constructor #10654

mscdex · 2017-01-06T08:18:10Z

These PRs improve ClientRequest creation performance and tidy up a few other things.

Results with the included benchmark:

                                                    improvement significant      p.value
 http/create-clientrequest.js n=1000000 pathlen=1       19.66 %         *** 5.177002e-26
 http/create-clientrequest.js n=1000000 pathlen=128      6.25 %         *** 2.631094e-07
 http/create-clientrequest.js n=1000000 pathlen=16      17.25 %         *** 1.114277e-27
 http/create-clientrequest.js n=1000000 pathlen=32      15.81 %         *** 1.993393e-24
 http/create-clientrequest.js n=1000000 pathlen=64       6.67 %         *** 1.037841e-11
 http/create-clientrequest.js n=1000000 pathlen=8       16.12 %         *** 4.243041e-23

/cc @nodejs/http

CI: https://ci.nodejs.org/job/node-test-pull-request/5716/

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

http

targos · 2017-01-06T08:32:17Z

lib/_http_client.js

-  if (!common._checkIsHttpToken(method)) {
-    throw new TypeError('Method must be a valid HTTP token');
+
+  if (methodIsString) {


What if method is the empty string? It defaulted to 'GET' before

Fixed + test added.

mscdex · 2017-01-06T09:48:59Z

CI after another fix: https://ci.nodejs.org/job/node-test-pull-request/5720/

indutny · 2017-01-06T12:00:25Z

lib/_http_client.js

+// checks can greatly outperform the equivalent regexp (tested in V8 5.4).
+function isInvalidPath(s) {
+  var len = s.length;
+  if (len--) { if (s.charCodeAt(0) <= 32) return true; } else return false;


I think assumption that path length is usually less than 6 chars is very far fetched. Should we really specialize to this?

It's just loop unrolling and from my testing it seems to help, no matter what the length is.

Is the path not always expected to begin with /? If so, the check for s.charCodeAt(0) can look specifically for that.

@jasnell The path does not always have to begin with a forward slash. Some notable examples include the HTTP CONNECT and OPTIONS methods which may use an absolute URI or asterisk respectively.

ah yes... good ole CONNECT and OPTIONS...

@indutny Did that answer your question? If so, does this PR LGTY?

mscdex · 2017-01-07T03:29:41Z

I've changed the path validation function implementation a bit to squeeze out a tiny bit more performance. CI again: ~~https://ci.nodejs.org/job/node-test-pull-request/5744/~~ https://ci.nodejs.org/job/node-test-pull-request/5745/

jasnell · 2017-01-10T19:34:06Z

@indutny ... does this LGTY with the recent updates?

indutny

LGTM

mscdex · 2017-01-13T19:42:16Z

CI once more before landing: https://ci.nodejs.org/job/node-test-pull-request/5853/

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

MylesBorins · 2017-03-07T01:19:24Z

should this be backported?

MylesBorins · 2017-05-08T15:09:38Z

ping

gibfahn · 2017-06-17T20:00:53Z

should this be backported?

ping @mscdex

MylesBorins · 2017-07-17T14:07:35Z

ping

MylesBorins · 2017-08-14T17:31:12Z

setting as baking for now

mscdex added the http Issues or PRs related to the http subsystem. label Jan 6, 2017

nodejs-github-bot added http Issues or PRs related to the http subsystem. dont-land-on-v7.x labels Jan 6, 2017

mscdex removed the dont-land-on-v7.x label Jan 6, 2017

targos reviewed Jan 6, 2017

View reviewed changes

mscdex force-pushed the http-client-creation-perf branch from b2f1220 to 7a8b632 Compare January 6, 2017 09:47

nodejs-github-bot added the dont-land-on-v7.x label Jan 6, 2017

mscdex removed the dont-land-on-v7.x label Jan 6, 2017

indutny suggested changes Jan 6, 2017

View reviewed changes

mscdex force-pushed the http-client-creation-perf branch from 7a8b632 to 71b0bb9 Compare January 7, 2017 03:27

nodejs-github-bot added the dont-land-on-v7.x label Jan 7, 2017

mscdex removed the dont-land-on-v7.x label Jan 7, 2017

mscdex force-pushed the http-client-creation-perf branch from 71b0bb9 to 7df1bef Compare January 7, 2017 04:14

nodejs-github-bot added the dont-land-on-v7.x label Jan 7, 2017

jasnell approved these changes Jan 7, 2017

View reviewed changes

mscdex removed the dont-land-on-v7.x label Jan 7, 2017

indutny approved these changes Jan 13, 2017

View reviewed changes

mscdex force-pushed the http-client-creation-perf branch from 7df1bef to 1291449 Compare January 13, 2017 19:38

nodejs-github-bot added the dont-land-on-v7.x label Jan 13, 2017

mscdex removed the dont-land-on-v7.x label Jan 13, 2017

mscdex added 6 commits January 13, 2017 16:11

http: optimize short path validation

5c2ef14

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

http: optimize default method case

dab22b5

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

http: avoid duplicate isArray()

15c6187

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

http: misc ClientRequest cleanup

2f77596

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

benchmark: add ClientRequest creation benchmark

c5d0fd9

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

test: add http.ClientRequest defaults test

57e2ec4

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

mscdex mentioned this pull request Jan 19, 2017

backport: v7.x: #10654 #10889

Merged

3 tasks

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017

http: optimize short path validation

7d45581

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017

http: avoid duplicate isArray()

072e53b

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017

http: misc ClientRequest cleanup

1d9c89a

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017

benchmark: add ClientRequest creation benchmark

43ffc9a

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017

test: add http.ClientRequest defaults test

5713e74

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017

http: optimize short path validation

d9684de

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017

http: avoid duplicate isArray()

6add7f7

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017

http: misc ClientRequest cleanup

64c4e1e

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017

benchmark: add ClientRequest creation benchmark

5380c48

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017

test: add http.ClientRequest defaults test

69523f6

PR-URL: nodejs#10654 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

MylesBorins added lts-watch-v4.x labels Mar 7, 2017

MylesBorins removed the lts-watch-v4.x label May 8, 2017

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 14, 2017

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: improve ClientRequest constructor #10654

http: improve ClientRequest constructor #10654

mscdex commented Jan 6, 2017 •

edited

Loading

targos Jan 6, 2017

mscdex Jan 6, 2017

mscdex commented Jan 6, 2017

indutny Jan 6, 2017

mscdex Jan 6, 2017

jasnell Jan 6, 2017

mscdex Jan 6, 2017

jasnell Jan 6, 2017

mscdex Jan 8, 2017

mscdex commented Jan 7, 2017 •

edited

Loading

jasnell commented Jan 10, 2017

indutny left a comment

mscdex commented Jan 13, 2017

MylesBorins commented Mar 7, 2017

MylesBorins commented May 8, 2017

gibfahn commented Jun 17, 2017

MylesBorins commented Jul 17, 2017

MylesBorins commented Aug 14, 2017

http: improve ClientRequest constructor #10654

http: improve ClientRequest constructor #10654

Conversation

mscdex commented Jan 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jan 6, 2017

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

mscdex commented Jan 7, 2017 • edited Loading

jasnell commented Jan 10, 2017

indutny left a comment

Choose a reason for hiding this comment

mscdex commented Jan 13, 2017

MylesBorins commented Mar 7, 2017

MylesBorins commented May 8, 2017

gibfahn commented Jun 17, 2017

MylesBorins commented Jul 17, 2017

MylesBorins commented Aug 14, 2017

mscdex commented Jan 6, 2017 •

edited

Loading

mscdex commented Jan 7, 2017 •

edited

Loading