FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

http: speed up checkIsHttpToken - #4790

Closed
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:checkIsHttpToken
ConversationCommits (1)ChecksFiles changed
Closed

http: speed up checkIsHttpToken#4790
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:checkIsHttpToken

Conversation

JacksonTian commented Jan 21, 2016

Copy link
Copy Markdown
Contributor

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

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

mscdex commented Jan 21, 2016

Copy link
Copy Markdown
Contributor

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 commented Jan 21, 2016

Copy link
Copy Markdown
Contributor

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

tflanagan reviewed Jan 21, 2016
View reviewed changes
Comment thread lib/_http_common.js Outdated

tflanagan Jan 21, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality 
const len = val.length;
for (let i = 0; i < len; i++) {

Not sure if it matters or improves anything.

ronkorving Jan 25, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It means the same, so shouldn't matter.

jasnell commented Jan 21, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell commented Jan 23, 2016

Copy link
Copy Markdown
Member

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

mscdex commented Jan 23, 2016

Copy link
Copy Markdown
Contributor

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

ChALkeR commented Jan 24, 2016

Copy link
Copy Markdown
Member

@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 reviewed Jan 24, 2016
View reviewed changes
Comment thread lib/_http_common.js Outdated

ChALkeR Jan 24, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mscdex Jan 24, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ronkorving commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

@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 commented Jan 25, 2016

Copy link
Copy Markdown
Contributor Author

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

ChALkeR commented Jan 25, 2016

Copy link
Copy Markdown
Member

@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 commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

@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 commented Jan 25, 2016

Copy link
Copy Markdown
Member

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 commented Jan 25, 2016

Copy link
Copy Markdown
Contributor Author

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 commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

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

jasnell commented Jan 27, 2016

Copy link
Copy Markdown
Member

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

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

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

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 commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

jasnell reviewed Mar 22, 2016
View reviewed changes
Comment thread benchmark/http/check_is_http_token.js Outdated

jasnell Mar 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

benchmark needs 'use strict'; at the top

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 commented Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

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 commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

CI is green. LGTM

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

jasnell pushed a commit that referenced this pull request Mar 27, 2016
http: speed up checkIsHttpToken
089c6a4 
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 commented Mar 27, 2016

Copy link
Copy Markdown
Member

Landed in 089c6a4

jasnell closed this Mar 27, 2016

jasnell commented Mar 27, 2016

Copy link
Copy Markdown
Member

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

evanlucas pushed a commit that referenced this pull request Mar 30, 2016
http: speed up checkIsHttpToken
f275176 
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>
evanlucas mentioned this pull request Mar 31, 2016
Closed
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
http: speed up checkIsHttpToken
63c601b 
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 commented May 17, 2016

Copy link
Copy Markdown
Contributor

@jasnell should we include this in v4.5.0?

jasnell commented May 17, 2016

Copy link
Copy Markdown
Member

It's likely safe

MylesBorins pushed a commit that referenced this pull request Jun 1, 2016
http: speed up checkIsHttpToken
4c7272a 
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 pushed a commit that referenced this pull request Jun 23, 2016
http: speed up checkIsHttpToken
c61b0ab 
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 pushed a commit that referenced this pull request Jun 24, 2016
http: speed up checkIsHttpToken
c98aaf5 
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 mentioned this pull request Jun 24, 2016
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

http Issues or PRs related to the http subsystem. performance Issues and PRs related to the performance of Node.js.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL