http: simplify checkInvalidHeaderChar - #18381
Closed
sethbrenith wants to merge 1 commit into
Closed
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
TimothyGu
reviewed
Jan 26, 2018
Comment thread
lib/_http_common.js
Outdated
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
- Rather than doing !/^[...]*$/.test(val), it might be simpler to do /[^...]/.test(val).
- It seems there are some regressions in the benchmark. If they still exist after swapping the conditions as I did above, we might want to add a fast case for things like the empty string.
Sorry, something went wrong.
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
I am relatively certain that there will still be a certain amount of regression even with that change. If not: wonderful. But in case the regressions continue to exist: I would like to keep the implementation and add the RegExp for val.length > 5.
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment.
Sorry for the delayed response, and thanks for the feedback!
- Agreed, /[^...]/.test(val) is prettier. I'll update it.
- I think regressing some particular strings is okay if the expected overall throughput on realistic data is improved. I don't have real-world data, but I do have data from an AcmeAir run (which uses popular modules in relatively straightforward ways, so might be somewhat representative of real data), and it tends to call this method with longer strings where the regex is faster. The same sort of suggestions came up in the previous change to simplify checkIsHttpToken, and people ended up deciding that the readability win was worth regressing a few benchmark cases. Furthermore, V8 folks like @bmeurer and @psmarshall are committed to ensuring that idiomatic code is performant, and there is already an open bug for improving regex performance on short inputs.
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment.
Comparison between iterations (old is !/^[...]*$/.test(val), new is /[^...]/.test(val)):
improvement confidence p.value http\\check_invalid_header_char.js n=1000000 input="\177" -10.43 % *** 1.145512e-11 http\\check_invalid_header_char.js n=1000000 input="" 20.55 % *** 5.135039e-20 http\\check_invalid_header_char.js n=1000000 input="\\t\\t\\t\\t\\t\\t\\t\\t\\t\\tFoo bar baz" -1.96 % 3.430864e-01 http\\check_invalid_header_char.js n=1000000 input="1" 18.08 % *** 1.373559e-22 http\\check_invalid_header_char.js n=1000000 input="20091" 13.54 % *** 4.766386e-09 http\\check_invalid_header_char.js n=1000000 input="ä,-æ-╪å`¢" -9.09 % *** 2.598671e-09 http\\check_invalid_header_char.js n=1000000 input="close" 14.75 % *** 5.152685e-10 http\\check_invalid_header_char.js n=1000000 input="en-US" 15.73 % *** 4.681282e-09 http\\check_invalid_header_char.js n=1000000 input="foo\\nbar" -1.83 % 1.630593e-01 http\\check_invalid_header_char.js n=1000000 input="group_acmeair" -13.64 % *** 8.914585e-15 http\\check_invalid_header_char.js n=1000000 input="gzip" 14.60 % *** 7.965079e-26 http\\check_invalid_header_char.js n=1000000 input="Here is a value that is real..."(truncated) 55.78 % *** 9.914895e-30 http\\check_invalid_header_char.js n=1000000 input="keep-alive" 10.51 % *** 5.515997e-09 http\\check_invalid_header_char.js n=1000000 input="private" 9.98 % *** 4.858798e-09 http\\check_invalid_header_char.js n=1000000 input="SAMEORIGIN" 7.52 % *** 1.855577e-13 http\\check_invalid_header_char.js n=1000000 input="Sat, 07 May 2016 16:54:48 GMT" -7.75 % *** 1.633302e-04 http\\check_invalid_header_char.js n=1000000 input="text/html; charset=utf-8" -8.10 % *** 2.957433e-07 http\\check_invalid_header_char.js n=1000000 input="text/plain" 7.21 % *** 2.673265e-11
These changes are generally pretty small compared to the change-versus-baseline numbers. I'll update the table in the description as soon as I push the updated code.
Sorry, something went wrong.
sethbrenith
force-pushed
the
invalid-header-char-regex
branch
from
e4b1747 to
54f84e9
Compare
February 6, 2018 00:09
In the spirit of [17399](nodejs#17399), we can also simplify checkInvalidHeaderChar to use regex matching instead of a loop. This makes it faster on long matches and slower on short matches or non-matches. This change also includes some sample data from an AcmeAir benchmark run, as a rough proxy for real-world data.
sethbrenith
force-pushed
the
invalid-header-char-regex
branch
from
54f84e9 to
1179bab
Compare
February 8, 2018 16:37
BridgeAR
approved these changes
Feb 10, 2018
Copy link
Copy Markdown
Member
Sorry, something went wrong.
mcollina
approved these changes
Feb 14, 2018
mcollina
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
This should no be backported to 4 or 6, as I suspect the performance might be completely different. |
Sorry, something went wrong.
joyeecheung
approved these changes
Feb 14, 2018
bmeurer
approved these changes
Feb 14, 2018
bmeurer
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
TimothyGu
approved these changes
Feb 14, 2018
BridgeAR
pushed a commit
to BridgeAR/node
that referenced
this pull request
Feb 16, 2018
In the spirit of [17399](nodejs#17399), we can also simplify checkInvalidHeaderChar to use regex matching instead of a loop. This makes it faster on long matches and slower on short matches or non-matches. This change also includes some sample data from an AcmeAir benchmark run, as a rough proxy for real-world data. PR-URL: nodejs#18381 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Copy link
Copy Markdown
Contributor
Author
|
Thanks everyone! |
Sorry, something went wrong.
MylesBorins
pushed a commit
that referenced
this pull request
Feb 21, 2018
In the spirit of [17399](#17399), we can also simplify checkInvalidHeaderChar to use regex matching instead of a loop. This makes it faster on long matches and slower on short matches or non-matches. This change also includes some sample data from an AcmeAir benchmark run, as a rough proxy for real-world data. PR-URL: #18381 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Merged
MylesBorins
pushed a commit
that referenced
this pull request
Feb 21, 2018
In the spirit of [17399](#17399), we can also simplify checkInvalidHeaderChar to use regex matching instead of a loop. This makes it faster on long matches and slower on short matches or non-matches. This change also includes some sample data from an AcmeAir benchmark run, as a rough proxy for real-world data. PR-URL: #18381 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
MayaLekova
pushed a commit
to MayaLekova/node
that referenced
this pull request
May 8, 2018
In the spirit of [17399](nodejs#17399), we can also simplify checkInvalidHeaderChar to use regex matching instead of a loop. This makes it faster on long matches and slower on short matches or non-matches. This change also includes some sample data from an AcmeAir benchmark run, as a rough proxy for real-world data. PR-URL: nodejs#18381 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Copy link
Copy Markdown
Member
|
Does not land cleanly on 8.x. Will need a backport PR in order to land. |
Sorry, something went wrong.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In the spirit of 17399, we can also simplify checkInvalidHeaderChar to use regex matching instead of a loop. This makes it faster on long matches and slower on short matches or non-matches. This change also includes some sample data from an AcmeAir benchmark run, as a rough proxy for real-world data.
Checklist
Affected core subsystem(s)
Benchmark results