http: speed up checkIsHttpToken by JacksonTian · Pull Request #4790 · nodejs/node

JacksonTian · 2016-01-21T09:36:27Z

The Regex implementation is not faster than ascii code compare.

the field name is shorter, the speed is faster.

benchmark result here:

https://bitbucket.org/snippets/JacksonTian/Rnbad/benchmark-result

mscdex · 2016-01-21T15:50:04Z

Can you add a comment above checkIsHttpToken() that explains why the current implementation is being used over regexp, and at least include the v8 version in the explanation. For example:

// This implementation of checkIsHttpToken() loops over the string instead of// using a regular expression since the former is up to 180% faster with v8 4.7// depending on the string length (the shorter the string, the larger the// performance difference)

mscdex · 2016-01-21T15:52:56Z

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

tflanagan · 2016-01-21T15:59:43Z

constlen=val.length;for(leti=0;i<len;i++){
Not sure if it matters or improves anything.

It means the same, so shouldn't matter.

jasnell · 2016-01-21T22:41:25Z

LGTM

jasnell · 2016-01-23T04:44:25Z

@mscdex ... can I ask you to take another look?

mscdex · 2016-01-23T04:57:18Z

LGTM if CI is still ok with it: https://ci.nodejs.org/job/node-test-pull-request/1356/

ChALkeR · 2016-01-24T20:34:38Z

@JacksonTian Most probably things didn't change, but just in case — could you rebase against current master and re-run the benchmarks, please? v8 4.8 has landed to master a few days ago: #4785.

ChALkeR · 2016-01-24T20:35:58Z

This comment probably should also be updated to reflect the situation on v8 4.8 (that most likely didn't change, but still it would be better to re-check that).

Well if it performs the same, the comment is fine as-is.

ronkorving · 2016-01-25T03:04:37Z

@JacksonTian It seems that the benchmark is currently not checking strings (short and long) that are not valid tokens. Adding those would complete the picture.

JacksonTian · 2016-01-25T05:17:03Z

Thanks, @ronkorving , should check the length of string.

ChALkeR · 2016-01-25T05:24:13Z

@ronkorving Note that strings that are not valid tokens should be never hit in valid code, and definitely they wouldn't be in the hot code path.

ronkorving · 2016-01-25T05:50:41Z

@ChALkeR I'm aware that they would not be in the hot code path, but it can happen in valid code, can't it? (I can send whatever I want to your server).

jasnell · 2016-01-25T06:22:27Z

Just a quick note... This function is not called when an http message is received and parsed. This is only and called when a header is set, and yes, it's whole point is to make sure the input is valid. Benchmarking with invalid input makes sense.

JacksonTian · 2016-01-25T07:36:07Z

Updated the benchmarks:

$ ./node benchmark/compare.js ./node ~/.tnvm/versions/node/v5.5.0/bin/node -- http check_is_http_token.js
running ./node
http/check_is_http_token.js
running /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node
http/check_is_http_token.js
http/check_is_http_token.js key=TCN n=1000000: ./node: 51572000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 15310000 ................. 236.86%
http/check_is_http_token.js key=ETag n=1000000: ./node: 48290000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 17196000 ................ 180.82%
http/check_is_http_token.js key=date n=1000000: ./node: 36327000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 18460000 ................. 96.79%
http/check_is_http_token.js key=Vary n=1000000: ./node: 41768000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 15744000 ................ 165.30%
http/check_is_http_token.js key=server n=1000000: ./node: 26680000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 15540000 ............... 71.68%
http/check_is_http_token.js key=Server n=1000000: ./node: 30184000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 12586000 .............. 139.83%
http/check_is_http_token.js key=status n=1000000: ./node: 31065000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 14091000 .............. 120.46%
http/check_is_http_token.js key=version n=1000000: ./node: 27162000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 14666000 .............. 85.21%
http/check_is_http_token.js key=Expires n=1000000: ./node: 23853000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 13385000 .............. 78.21%
http/check_is_http_token.js key=alt-svc n=1000000: ./node: 27987000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 12539000 ............. 123.20%
http/check_is_http_token.js key=location n=1000000: ./node: 21943000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 13265000 ............. 65.42%
http/check_is_http_token.js key=Connection n=1000000: ./node: 19229000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 12365000 ........... 55.51%
http/check_is_http_token.js key=Keep-Alive n=1000000: ./node: 17815000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 13648000 ........... 30.54%
http/check_is_http_token.js key=content-type n=1000000: ./node: 16488000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 13965000 ......... 18.06%
http/check_is_http_token.js key=Content-Type n=1000000: ./node: 17237000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 12495000 ......... 37.95%
http/check_is_http_token.js key=Cache-Control n=1000000: ./node: 14031000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 13030000 ......... 7.69%
http/check_is_http_token.js key=Last-Modified n=1000000: ./node: 14797000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 9009100 ......... 64.25%
http/check_is_http_token.js key=Accept-Ranges n=1000000: ./node: 13960000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 11130000 ........ 25.43%
http/check_is_http_token.js key=content-length n=1000000: ./node: 13729000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 11711000 ....... 17.23%
http/check_is_http_token.js key=x-frame-options n=1000000: ./node: 11441000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 10915000 ....... 4.82%
http/check_is_http_token.js key=x-xss-protection n=1000000: ./node: 11358000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 11408000 ..... -0.44%
http/check_is_http_token.js key=Content-Encoding n=1000000: ./node: 13669000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 4164100 ..... 228.25%
http/check_is_http_token.js key=Content-Location n=1000000: ./node: 12569000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 11012000 ..... 14.14%
http/check_is_http_token.js key=Transfer-Encoding n=1000000: ./node: 12057000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 11340000 ..... 6.33%
http/check_is_http_token.js key=alternate-protocol n=1000000: ./node: 9157300 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 10027000 .... -8.67%
http/check_is_http_token.js key=: n=1000000: ./node: 58233000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 22897000 ................... 154.33%
http/check_is_http_token.js key=@@ n=1000000: ./node: 60918000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 23144000 .................. 163.21%
http/check_is_http_token.js key=中文呢 n=1000000: ./node: 56176000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 22926000 ................. 145.03%
http/check_is_http_token.js key=((((()))) n=1000000: ./node: 49334000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 20424000 ........... 141.55%
http/check_is_http_token.js key=:alternate-protocol n=1000000: ./node: 62579000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 21284000 . 194.02%
http/check_is_http_token.js key=alternate-protocol: n=1000000: ./node: 10406000 /Users/jacksontian/.tnvm/versions/node/v5.5.0/bin/node: 8494400 ... 22.51%

ronkorving · 2016-01-25T08:36:15Z

@jasnell thanks for the feedback on that
@JacksonTian awesome :) glad to see it's also much faster with bad input

jasnell · 2016-01-27T20:56:54Z

LGTM... one last CI run just to be safe: https://ci.nodejs.org/job/node-test-pull-request/1401/

jasnell · 2016-03-22T00:20:27Z

Looks like this one slipped by... old CI run is no longer available. New CI here: https://ci.nodejs.org/job/node-test-pull-request/2007/

jasnell · 2016-03-22T00:27:43Z

Looks like there are some linting issues: https://ci.nodejs.org/job/node-test-linter/1756/console

jasnell · 2016-03-22T04:54:53Z

Sigh.. scratch that on the linting issue. That was my bad. New CI: https://ci.nodejs.org/job/node-test-pull-request/2012/

jasnell · 2016-03-22T05:01:50Z

Heh... turns out there is a linting issue here afterall ;-)

jasnell · 2016-03-22T05:02:07Z

benchmark needs 'use strict'; at the top

The Regex implementation is not faster than ascii code compare. the field name is shorter, the speed is faster. benchmark result here: https://bitbucket.org/snippets/JacksonTian/Rnbad/benchmark-result

JacksonTian · 2016-03-22T14:59:58Z

Hi @jasnell , I updated the commit, and new benchmark result:

nodejs $ ./node benchmark/compare.js ./node ~/.tnvm/versions/node/v5.9.0/bin/node -- http check_is_http_token.js
running ./node
http/check_is_http_token.js
running /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node
http/check_is_http_token.js
http/check_is_http_token.js key=TCN n=1000000: ./node: 45457000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 14066000 ................. 223.17%
http/check_is_http_token.js key=ETag n=1000000: ./node: 34383000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 16297000 ................ 110.97%
http/check_is_http_token.js key=date n=1000000: ./node: 33622000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 14336000 ................ 134.53%
http/check_is_http_token.js key=Vary n=1000000: ./node: 45100000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 14597000 ................ 208.98%
http/check_is_http_token.js key=server n=1000000: ./node: 27569000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 14225000 ............... 93.80%
http/check_is_http_token.js key=Server n=1000000: ./node: 30061000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 15065000 ............... 99.54%
http/check_is_http_token.js key=status n=1000000: ./node: 26882000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 14861000 ............... 80.89%
http/check_is_http_token.js key=version n=1000000: ./node: 22659000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 14760000 .............. 53.51%
http/check_is_http_token.js key=Expires n=1000000: ./node: 24139000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 13811000 .............. 74.78%
http/check_is_http_token.js key=alt-svc n=1000000: ./node: 25975000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 13353000 .............. 94.53%
http/check_is_http_token.js key=location n=1000000: ./node: 22107000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 12315000 ............. 79.52%
http/check_is_http_token.js key=Connection n=1000000: ./node: 21405000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 11522000 ........... 85.78%
http/check_is_http_token.js key=Keep-Alive n=1000000: ./node: 22115000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 12204000 ........... 81.22%
http/check_is_http_token.js key=content-type n=1000000: ./node: 16575000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 12188000 ......... 36.00%
http/check_is_http_token.js key=Content-Type n=1000000: ./node: 16391000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 11180000 ......... 46.61%
http/check_is_http_token.js key=Cache-Control n=1000000: ./node: 13782000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 10497000 ........ 31.30%
http/check_is_http_token.js key=Last-Modified n=1000000: ./node: 13370000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 9994000 ......... 33.78%
http/check_is_http_token.js key=Accept-Ranges n=1000000: ./node: 13210000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 11462000 ........ 15.25%
http/check_is_http_token.js key=content-length n=1000000: ./node: 11774000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 10135000 ....... 16.17%
http/check_is_http_token.js key=x-frame-options n=1000000: ./node: 10369000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 10922000 ...... -5.06%
http/check_is_http_token.js key=x-xss-protection n=1000000: ./node: 10736000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 10580000 ...... 1.48%
http/check_is_http_token.js key=Content-Encoding n=1000000: ./node: 11194000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 9710300 ...... 15.28%
http/check_is_http_token.js key=Content-Location n=1000000: ./node: 13140000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 10737000 ..... 22.38%
http/check_is_http_token.js key=Transfer-Encoding n=1000000: ./node: 9408100 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 9077100 ....... 3.65%
http/check_is_http_token.js key=alternate-protocol n=1000000: ./node: 9988500 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 10603000 .... -5.80%
http/check_is_http_token.js key=: n=1000000: ./node: 53525000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 21647000 ................... 147.26%
http/check_is_http_token.js key=@@ n=1000000: ./node: 53534000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 19527000 .................. 174.16%
http/check_is_http_token.js key=中文呢 n=1000000: ./node: 57625000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 21915000 ................. 162.94%
http/check_is_http_token.js key=((((()))) n=1000000: ./node: 57758000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 21122000 ........... 173.45%
http/check_is_http_token.js key=:alternate-protocol n=1000000: ./node: 50803000 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 18648000 . 172.43%
http/check_is_http_token.js key=alternate-protocol: n=1000000: ./node: 9047800 /Users/jacksontian/.tnvm/versions/node/v5.9.0/bin/node: 6757100 .... 33.90%

jasnell · 2016-03-22T15:22:45Z

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

jasnell · 2016-03-22T16:33:45Z

CI is green. LGTM

jasnell · 2016-03-22T16:34:15Z

@indutny ... any final comments on this before I land?

The Regex implementation is not faster than ascii code compare. the field name is shorter, the speed is faster. benchmark result here: https://bitbucket.org/snippets/JacksonTian/Rnbad/benchmark-result PR-URL: #4790 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell · 2016-03-27T20:59:32Z

Landed in 089c6a4

jasnell · 2016-03-27T20:59:54Z

@thealphanerd ... let's hold off landing this in v4 until it's been in v5 for a while.

The Regex implementation is not faster than ascii code compare. the field name is shorter, the speed is faster. benchmark result here: https://bitbucket.org/snippets/JacksonTian/Rnbad/benchmark-result PR-URL: #4790 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

MylesBorins · 2016-05-17T22:33:14Z

@jasnell should we include this in v4.5.0?

jasnell · 2016-05-17T23:19:21Z

It's likely safe

The Regex implementation is not faster than ascii code compare. the field name is shorter, the speed is faster. benchmark result here: https://bitbucket.org/snippets/JacksonTian/Rnbad/benchmark-result PR-URL: #4790 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

ChALkeR added the http Issues or PRs related to the http subsystem. label Jan 21, 2016

tflanagan reviewed Jan 21, 2016
View reviewed changes

jasnell added the lts-watch-v4.x label Jan 21, 2016

JacksonTian force-pushed the checkIsHttpToken branch from 26fd567 to 6f7e790Compare January 22, 2016 02:19

ChALkeR reviewed Jan 24, 2016
View reviewed changes

JacksonTian force-pushed the checkIsHttpToken branch from 6f7e790 to 8cfc0f8Compare January 25, 2016 07:33

ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Feb 16, 2016

jasnell reviewed Mar 22, 2016
View reviewed changes

Comment threadbenchmark/http/check_is_http_token.js Outdated
jasnellMar 22, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
benchmark needs 'use strict'; at the top

JacksonTian force-pushed the checkIsHttpToken branch from 8cfc0f8 to 72e7e68Compare March 22, 2016 14:53

http: speed up checkIsHttpToken
e7246d1
The Regex implementation is not faster than ascii code compare. the field name is shorter, the speed is faster. benchmark result here: https://bitbucket.org/snippets/JacksonTian/Rnbad/benchmark-result

JacksonTian force-pushed the checkIsHttpToken branch from 72e7e68 to e7246d1Compare March 22, 2016 14:59

jasnell closed this Mar 27, 2016

evanlucas mentioned this pull request Mar 31, 2016
v5.10.0 Release Proposal #5970
Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jun 1, 2016

MylesBorins mentioned this pull request Jun 24, 2016
v4.4.7 proposal #7395
Merged

Uh oh!

Uh oh!

Conversation

JacksonTian commented Jan 21, 2016

Uh oh!

mscdex commented Jan 21, 2016

Uh oh!

mscdex commented Jan 21, 2016

Uh oh!

tflanaganJan 21, 2016

Choose a reason for hiding this comment

Uh oh!

ronkorvingJan 25, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 21, 2016

Uh oh!

jasnell commented Jan 23, 2016

Uh oh!

mscdex commented Jan 23, 2016

Uh oh!

ChALkeR commented Jan 24, 2016

Uh oh!

ChALkeRJan 24, 2016

Choose a reason for hiding this comment

Uh oh!

mscdexJan 24, 2016

Choose a reason for hiding this comment

Uh oh!

ronkorving commented Jan 25, 2016

Uh oh!

JacksonTian commented Jan 25, 2016

Uh oh!

ChALkeR commented Jan 25, 2016

Uh oh!

ronkorving commented Jan 25, 2016

Uh oh!

jasnell commented Jan 25, 2016

Uh oh!

JacksonTian commented Jan 25, 2016

Uh oh!

ronkorving commented Jan 25, 2016

Uh oh!

jasnell commented Jan 27, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

jasnellMar 22, 2016

Choose a reason for hiding this comment

Uh oh!

JacksonTian commented Mar 22, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

jasnell commented Mar 27, 2016

Uh oh!

jasnell commented Mar 27, 2016

Uh oh!

MylesBorins commented May 17, 2016

Uh oh!

jasnell commented May 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants