http: simplify checkInvalidHeaderChar #18381

sethbrenith · 2018-01-25T18:04:54Z

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

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

Benchmark results

                                                                                                improvement confidence      p.value
 http\\check_invalid_header_char.js n=1000000 input="\177"                                        -56.65 %        *** 1.320705e-40
 http\\check_invalid_header_char.js n=1000000 input=""                                            -70.08 %        *** 1.329603e-60
 http\\check_invalid_header_char.js n=1000000 input="\\t\\t\\t\\t\\t\\t\\t\\t\\t\\tFoo bar baz"   198.82 %        *** 1.052799e-76
 http\\check_invalid_header_char.js n=1000000 input="1"                                           -50.00 %        *** 7.800619e-22
 http\\check_invalid_header_char.js n=1000000 input="20091"                                        -6.44 %        *** 7.491287e-04
 http\\check_invalid_header_char.js n=1000000 input="ä,-æ-╪å`¢"                                   -33.16 %        *** 5.648190e-21
 http\\check_invalid_header_char.js n=1000000 input="close"                                        22.88 %        *** 1.497247e-25
 http\\check_invalid_header_char.js n=1000000 input="en-US"                                        24.31 %        *** 4.582121e-46
 http\\check_invalid_header_char.js n=1000000 input="foo\\nbar"                                    -5.84 %        *** 2.581802e-08
 http\\check_invalid_header_char.js n=1000000 input="group_acmeair"                                19.81 %        *** 2.764867e-37
 http\\check_invalid_header_char.js n=1000000 input="gzip"                                          4.54 %          * 1.921983e-02
 http\\check_invalid_header_char.js n=1000000 input="Here is a value that is real..."(truncated)  280.31 %        *** 8.462204e-37
 http\\check_invalid_header_char.js n=1000000 input="keep-alive"                                   80.49 %        *** 4.132616e-53
 http\\check_invalid_header_char.js n=1000000 input="private"                                      41.84 %        *** 1.856953e-15
 http\\check_invalid_header_char.js n=1000000 input="SAMEORIGIN"                                   82.00 %        *** 2.317592e-50
 http\\check_invalid_header_char.js n=1000000 input="Sat, 07 May 2016 16:54:48 GMT"               199.94 %        *** 2.698766e-29
 http\\check_invalid_header_char.js n=1000000 input="text/html; charset=utf-8"                    176.87 %        *** 1.854601e-33
 http\\check_invalid_header_char.js n=1000000 input="text/plain"                                   75.47 %        *** 1.241438e-24

TimothyGu · 2018-01-26T00:53:14Z

lib/_http_common.js

-      return true;
-  }
-  return false;
+  return !headerCharRegex.test(val);


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.

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

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.

BridgeAR · 2018-02-01T14:15:21Z

Ping @sethbrenith

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.

BridgeAR · 2018-02-10T20:09:26Z

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

mcollina

LGTM

mcollina · 2018-02-14T00:55:46Z

This should no be backported to 4 or 6, as I suspect the performance might be completely different.

bmeurer

LGTM

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Benedikt Meurer <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

BridgeAR · 2018-02-16T18:27:52Z

Landed in 862389b 🎉

sethbrenith · 2018-02-16T18:51:14Z

Thanks everyone!

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Benedikt Meurer <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Benedikt Meurer <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

jasnell · 2018-08-17T20:41:17Z

Does not land cleanly on 8.x. Will need a backport PR in order to land.

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jan 25, 2018

TimothyGu reviewed Jan 26, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:03

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

maclover7 added the wip Issues and PRs that are still a work in progress. label Feb 3, 2018

sethbrenith force-pushed the invalid-header-char-regex branch from e4b1747 to 54f84e9 Compare February 6, 2018 00:09

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

View reviewed changes

BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Feb 10, 2018

BridgeAR requested review from mcollina, jasnell, TimothyGu and joyeecheung February 12, 2018 14:46

mcollina approved these changes Feb 14, 2018

View reviewed changes

mcollina added dont-land-on-v4.x labels Feb 14, 2018

joyeecheung approved these changes Feb 14, 2018

View reviewed changes

bmeurer approved these changes Feb 14, 2018

View reviewed changes

TimothyGu approved these changes Feb 14, 2018

View reviewed changes

BridgeAR closed this Feb 16, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

jasnell added the backport-requested-v8.x label Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: simplify checkInvalidHeaderChar #18381

http: simplify checkInvalidHeaderChar #18381

sethbrenith commented Jan 25, 2018 •

edited

Loading

TimothyGu Jan 26, 2018

BridgeAR Jan 31, 2018

sethbrenith Feb 1, 2018

sethbrenith Feb 5, 2018 •

edited

Loading

BridgeAR commented Feb 1, 2018

BridgeAR commented Feb 10, 2018

mcollina left a comment

mcollina commented Feb 14, 2018

bmeurer left a comment

BridgeAR commented Feb 16, 2018

sethbrenith commented Feb 16, 2018

jasnell commented Aug 17, 2018

http: simplify checkInvalidHeaderChar #18381

http: simplify checkInvalidHeaderChar #18381

Conversation

sethbrenith commented Jan 25, 2018 • edited Loading

Checklist

Affected core subsystem(s)

Benchmark results

TimothyGu Jan 26, 2018

Choose a reason for hiding this comment

BridgeAR Jan 31, 2018

Choose a reason for hiding this comment

sethbrenith Feb 1, 2018

Choose a reason for hiding this comment

sethbrenith Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR commented Feb 1, 2018

BridgeAR commented Feb 10, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Feb 14, 2018

bmeurer left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 16, 2018

sethbrenith commented Feb 16, 2018

jasnell commented Aug 17, 2018

sethbrenith commented Jan 25, 2018 •

edited

Loading

sethbrenith Feb 5, 2018 •

edited

Loading