Skip to content
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

Don't allow one invalid char after header name when allowing spaces #115

Merged
merged 1 commit into from
Apr 26, 2022

Conversation

nox
Copy link
Contributor

@nox nox commented Apr 25, 2022

No description provided.

Copy link
Contributor

@acfoltzer acfoltzer left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

Even though the sequence of operations doesn't change much with this patch, the revised code structure must really agree with some compiler optimizations:

❯ cargo criterion
   Compiling httparse v1.7.0 (/home/acfoltzer/src/httparse)
    Finished bench [optimized] target(s) in 22.59s
Gnuplot not found, using plotters backend
req/req                 time:   [249.34 ns 250.45 ns 251.62 ns]
                        thrpt:  [2.6649 GiB/s 2.6774 GiB/s 2.6893 GiB/s]
                 change:
                        time:   [-22.215% -21.847% -21.523%] (p = 0.00 < 0.05)
                        thrpt:  [+27.426% +27.954% +28.559%]
                        Performance has improved.

req_short/req_short     time:   [60.949 ns 61.140 ns 61.380 ns]
                        thrpt:  [1.0318 GiB/s 1.0358 GiB/s 1.0391 GiB/s]
                 change:
                        time:   [-4.9845% -4.5957% -4.2197%] (p = 0.00 < 0.05)
                        thrpt:  [+4.4056% +4.8171% +5.2460%]
                        Performance has improved.

resp/resp               time:   [262.45 ns 263.11 ns 263.86 ns]
                        thrpt:  [2.4672 GiB/s 2.4742 GiB/s 2.4805 GiB/s]
                 change:
                        time:   [-26.910% -26.560% -26.208%] (p = 0.00 < 0.05)
                        thrpt:  [+35.516% +36.166% +36.818%]
                        Performance has improved.

resp_short/resp_short   time:   [57.907 ns 58.072 ns 58.247 ns]
                        thrpt:  [1.4550 GiB/s 1.4594 GiB/s 1.4636 GiB/s]
                 change:
                        time:   [-20.539% -19.870% -19.219%] (p = 0.00 < 0.05)
                        thrpt:  [+23.792% +24.797% +25.848%]
                        Performance has improved.

A nice bonus in addition to the correctness fix.

@acfoltzer
Copy link
Contributor

@seanmonstar I don't have write access to the repo so this will still need your +1, but lgtm.

@seanmonstar seanmonstar merged commit 7ab0192 into seanmonstar:master Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants