-
Notifications
You must be signed in to change notification settings - Fork 30.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
http: optimize checkIsHttpToken() and checkInvalidHeaderChar() #6570
Conversation
4961e40
to
c7329a3
Compare
Is this something we should do any sort of linting for so that we maintain it? Or add a comment? |
Aside: has anyone looked into if these v8 settings would be better tweaked by node by default? |
Code comments would be good (outside of the function ;-) ...) |
I can add a comment I suppose. I'm not sure if there is an inline eslint comment we can add to enforce function code size or not. As far as changing the default |
c7329a3
to
bb208d9
Compare
Comment added. |
var ch = val.charCodeAt(i); | ||
|
||
const ch = val.charCodeAt(i); | ||
if (ch >= 94 && ch <= 122) // a-z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94-122 is not a-z, it's ^_`a-z
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
I think you can do |
bb208d9
to
1d7b821
Compare
Thanks @mscdex. Still LGTM. |
I think I may have found something that improves performance quite a bit more for every case, so do not merge yet. |
1d7b821
to
ac4c6d0
Compare
Ok, I've pushed new changes. In addition to the dramatically better performance of |
ac4c6d0
to
9044655
Compare
return false; | ||
if (ch >= 33 && ch <= 46) | ||
return true; | ||
if (ch === 124 || ch === 126) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now that this is split in two methods, are there still reasons to omit the comment here? This is // | ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now added the valid character list in the comment preceding the function instead of documenting them inline.
@mscdex Perhaps renaming |
@mscdex ... very nice. |
return false; | ||
continue; | ||
if (len > 3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the common case? Can you quantify how much the manual loop unrolling helps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't save the old results, but there was a lesser performance improvement with the pure loop implementation. I can run it again without and post the results here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok here are the results with just the loop, no unrolling:
http/check_is_http_token.js key="TCN" n="500000000": ./node: 73576000 ./node-master: 76945000 ................... -4.38%
http/check_is_http_token.js key="ETag" n="500000000": ./node: 60694000 ./node-master: 59833000 ................... 1.44%
http/check_is_http_token.js key="date" n="500000000": ./node: 71346000 ./node-master: 59617000 .................. 19.68%
http/check_is_http_token.js key="Vary" n="500000000": ./node: 64590000 ./node-master: 58538000 .................. 10.34%
http/check_is_http_token.js key="server" n="500000000": ./node: 49815000 ./node-master: 43208000 ................ 15.29%
http/check_is_http_token.js key="Server" n="500000000": ./node: 44101000 ./node-master: 42926000 ................. 2.74%
http/check_is_http_token.js key="status" n="500000000": ./node: 49805000 ./node-master: 43655000 ................ 14.09%
http/check_is_http_token.js key="version" n="500000000": ./node: 43245000 ./node-master: 37774000 ............... 14.48%
http/check_is_http_token.js key="Expires" n="500000000": ./node: 39172000 ./node-master: 37568000 ................ 4.27%
http/check_is_http_token.js key="alt-svc" n="500000000": ./node: 38508000 ./node-master: 35600000 ................ 8.17%
http/check_is_http_token.js key="location" n="500000000": ./node: 38132000 ./node-master: 34186000 .............. 11.54%
http/check_is_http_token.js key="Connection" n="500000000": ./node: 26488000 ./node-master: 24878000 ............. 6.47%
http/check_is_http_token.js key="Keep-Alive" n="500000000": ./node: 29028000 ./node-master: 27224000 ............. 6.63%
http/check_is_http_token.js key="content-type" n="500000000": ./node: 23468000 ./node-master: 21554000 ........... 8.88%
http/check_is_http_token.js key="Content-Type" n="500000000": ./node: 24507000 ./node-master: 23320000 ........... 5.09%
http/check_is_http_token.js key="Cache-Control" n="500000000": ./node: 22685000 ./node-master: 19883000 ......... 14.09%
http/check_is_http_token.js key="Last-Modified" n="500000000": ./node: 22605000 ./node-master: 19911000 ......... 13.53%
http/check_is_http_token.js key="Accept-Ranges" n="500000000": ./node: 22516000 ./node-master: 19637000 ......... 14.66%
http/check_is_http_token.js key="content-length" n="500000000": ./node: 20171000 ./node-master: 18336000 ........ 10.01%
http/check_is_http_token.js key="x-frame-options" n="500000000": ./node: 18221000 ./node-master: 16510000 ....... 10.36%
http/check_is_http_token.js key="x-xss-protection" n="500000000": ./node: 16391000 ./node-master: 14117000 ...... 16.11%
http/check_is_http_token.js key="Content-Encoding" n="500000000": ./node: 18711000 ./node-master: 16602000 ...... 12.70%
http/check_is_http_token.js key="Content-Location" n="500000000": ./node: 18612000 ./node-master: 16531000 ...... 12.59%
http/check_is_http_token.js key="Transfer-Encoding" n="500000000": ./node: 17713000 ./node-master: 15734000 ..... 12.58%
http/check_is_http_token.js key="alternate-protocol" n="500000000": ./node: 14988000 ./node-master: 14720000 ..... 1.82%
http/check_is_http_token.js key=":" n="500000000": ./node: 155200000 ./node-master: 108860000 ................... 42.57%
http/check_is_http_token.js key="@@" n="500000000": ./node: 151200000 ./node-master: 109090000 .................. 38.60%
http/check_is_http_token.js key="中文呢" n="500000000": ./node: 138090000 ./node-master: 101780000 ............... 35.67%
http/check_is_http_token.js key="((((())))" n="500000000": ./node: 184100000 ./node-master: 105320000 ........... 74.80%
http/check_is_http_token.js key=":alternate-protocol" n="500000000": ./node: 141000000 ./node-master: 103870000 . 35.75%
http/check_is_http_token.js key="alternate-protocol:" n="500000000": ./node: 14917000 ./node-master: 11983000 ... 24.49%
9044655
to
3feadd8
Compare
PR-URL: nodejs#6570 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: nodejs#6570 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
PR-URL: nodejs#6570 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
ed7d020
to
c570182
Compare
@mscdex not converting benchmark arguments to integers breaks some benchmarks. For example after applying c570182
|
@mscdex not calling
In my bechmark refactor #7094 I have added |
This commit both makes checkIsHttpToken() inlinable and extracts the character checking logic to a separate inlinable function so that the main loop can be unrolled a bit. PR-URL: #6570 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
This commit optimizes checkInvalidHeaderChar() by unrolling the character checking loop a bit. Additionally, some changes to the benchmark runner are needed in order for the included benchmark to be run correctly. Specifically, the regexp used to parse `key=value` parameters contained a greedy quantifier that was causing the `key` to match part of the `value` if `value` contained an equals sign. PR-URL: #6570 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
PR-URL: #6570 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #6570 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
PR-URL: #6570 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@mscdex lts? |
@thealphanerd Possibly, but #7311 is a related followup PR that would need to be landed and backported at the same time, since it fixes existing benchmarks due to the benchmark changes included in this PR. |
@mscdex I'll hold off then. Please tag the other as lts-watch and consider sending them together as a backport |
@mscdex thoughts on this now? Would you be up for submitting a backport |
@mscdex I'm going to opt not to land this on v4.x. Please feel free to submit a backport if you feel it should land |
Checklist
Affected core subsystem(s)
Description of change
This PR optimizes
checkIsHttpToken()
by:checkInvalidHeaderChar()
is also optimized by moving the character checking logic to a separate inlinable function to allow the main function to permit some loop unrolling.The results from the existing
check_is_http_token
benchmark are (with the iterations bumped up to5e8
):The results from the new
check_invalid_header_char
benchmark are: