Make HeaderName::from_static const #499

jeddenlea · 2021-08-20T23:47:39Z

... plus some clean-up.

It was only after I came up with the scheme using
const fn from_bytes(&[u8]) -> Option<StandardHeader>
that I noticed the debug+wasm32-wasi version of parse_hdr, which had
something very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in #433. Then, I thought it would be worth seeing
if the use of eq! in the primary version of parse_hdr still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime. I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of HeaderName::from_static itself, which now seems to run in about 2/3
the time on average.

Unfortunately, const fn still cannot panic!, but I've followed the
lead from HeaderValue::from_static. While that version required 1.46,
this new function requires 1.49. That is almost 8 months old, so
hopefully this isn't too controversial!

seanmonstar · 2022-02-24T00:38:18Z

@carllerche you wrote the eq! stuff, any insight on instances where it's worth keeping?

carllerche · 2022-03-16T22:32:10Z

It's been a while. My recollection is this was to optimize header parsing. If there are benchmarks still checking parse speed and they aren't impacted, then it is probably fine.

jeddenlea · 2022-03-17T00:19:21Z

Using cargo +nightly bench header_name, before:

test header_name_bad     ... bench:           9 ns/iter (+/- 0)
test header_name_easy    ... bench:          10 ns/iter (+/- 0)
test header_name_various ... bench:       3,885 ns/iter (+/- 875)

and with this change,

test header_name_bad         ... bench:          11 ns/iter (+/- 1)
test header_name_custom      ... bench:          81 ns/iter (+/- 5)
test header_name_easy        ... bench:          11 ns/iter (+/- 0)
test header_name_from_static ... bench:       1,024 ns/iter (+/- 483)
test header_name_various     ... bench:       3,829 ns/iter (+/- 288)

jeddenlea · 2022-03-17T00:31:07Z

.... and just to be complete, here are the two new benchmarks cut out and run on 0.2.6,

test header_name_custom      ... bench:          79 ns/iter (+/- 7)
test header_name_from_static ... bench:       1,241 ns/iter (+/- 72)

So, maybe only a 20% improvement instead of a 33% improvement, but still!

…o constname_merge Merges hyperium#428 into hyperium#499

npmccallum · 2022-04-02T14:35:38Z

@jeddenlea @seanmonstar FYI - const_panic is now stable in 1.57.0.

jeddenlea · 2022-04-06T02:00:10Z

src/header/name.rs

-                            panic!("invalid header name")
-                        }
-                    }
+    #[allow(unconditional_panic)] // required for the panic circumventon


D'oh, I copied the same error fixed in #532

…o constname_merge Merges hyperium#428 into hyperium#499

LucioFranco

overall lgtm w/ the benches you posted.

LucioFranco · 2022-04-15T14:21:04Z

src/header/name.rs

+    ///      |             ^^^^^^^^^^^^^^^^^^
+    ///      |             |
+    ///      |             index out of bounds: the length is 0 but the index is 0
+    ///      |             inside `fastly::http::HeaderName::from_static` at http/src/header/name.rs:1241:13


did you mean to include fastly here?

Suggested change

/// | inside `fastly::http::HeaderName::from_static` at http/src/header/name.rs:1241:13

/// | inside `http::HeaderName::from_static` at http/src/header/name.rs:1241:13

Haha, no, I did not. I'm so used to seeing it I didn't even notice. I'm going to force push that change.

seanmonstar

With the one proposed change, we can merge this :)

... plus some clean-up. It was only after I came up with the scheme using `const fn from_bytes(&[u8]) -> Option<StandardHeader>` that I noticed the debug+wasm32-wasi version of `parse_hdr`, which had something very similar. While cleaning up that function, I realized it still would still panic if an attempted name was too long, which had been fixed for all other targets and profiles in hyperium#433. Then, I thought it would be worth seeing if the use of `eq!` in the primary version of `parse_hdr` still made any difference. And, it would not appear so. At least not on x86_64, nor wasm32-wasi run via wasmtime. I've run the benchmarks a number of times now, and it seems the only significant performance change anywhere is actually that of `HeaderName::from_static` itself, which now seems to run in about 2/3 the time on average. Unfortunately, `const fn` still cannot `panic!`, but I've followed the lead from `HeaderValue::from_static`. While that version required 1.46, this new function requires 1.49. That is almost 8 months old, so hopefully this isn't too controversial!

…o constname_merge Merges hyperium#428 into hyperium#499

TheBlueMatt · 2022-04-29T19:49:30Z

.github/workflows/ci.yml

@@ -21,7 +21,7 @@ jobs:
          - nightly
          # When updating this value, don't forget to also adjust the
          # `rust-version` field in the `Cargo.toml` file.
-          - 1.46.0
+          - 1.49.0


Seems kinda strange that the MSRV was bumped without any notice in the PR description or in the release notes, no? Makes it somewhat hard to handle updates in downstream projects.

This project follows the msrv of tokio https://github.com/tokio-rs/tokio/#supported-rust-versions but we should add it to the changelog. Good catch.

It doesn't seem like it? tokio supports previous versions with point releases, allowing users with an older rustc to keep using tokio with bugfixes provided even if they have to ignore new versions. It looks like this crate is updated in point releases to have a new MSRV. In this case I wasn't able to use a down-down-downstream crate because it no longer built using system rustc on popular distros.

I think this is mainly due to a few reasons, tokio is already 1.0 and thus at the time of 1.0 we try to support everyone. While this crate is below 1.0 so the guarantees are different. I assume it should be possible to pin the version of http though for your crate?

Sure, understood, and I can pin for now, I was just noting that you said "this project follow the msrv of tokio" which isn't really accurate, given tokio has great support for broadly-available/deployed versions of rustc but I don't believe such support exists here (ie you miss bugfixes if you don't use rustup).

jeddenlea force-pushed the constname branch 2 times, most recently from 787b82b to feab2c3 Compare August 24, 2021 07:10

jeddenlea force-pushed the constname branch from feab2c3 to ba2b432 Compare December 17, 2021 21:34

jeddenlea mentioned this pull request Feb 3, 2022

Enable simpler header name parser in debug builds #521

Closed

jeddenlea force-pushed the constname branch from ba2b432 to 50642bd Compare February 9, 2022 21:21

jeddenlea mentioned this pull request Mar 30, 2022

feat: add support for the digest headers #539

Closed

jeddenlea added a commit to jeddenlea/http that referenced this pull request Apr 1, 2022

Merge remote-tracking branch 'remotes/sbosnick/audit_header_name' int…

f399df0

…o constname_merge Merges hyperium#428 into hyperium#499

jeddenlea mentioned this pull request Apr 1, 2022

Merge #428 and #499 #540

Closed

jeddenlea commented Apr 6, 2022

View reviewed changes

jeddenlea force-pushed the constname branch from 50642bd to 41a3df9 Compare April 6, 2022 02:01

jeddenlea added a commit to jeddenlea/http that referenced this pull request Apr 6, 2022

Merge remote-tracking branch 'remotes/sbosnick/audit_header_name' int…

a146c3b

…o constname_merge Merges hyperium#428 into hyperium#499

LucioFranco approved these changes Apr 15, 2022

View reviewed changes

LucioFranco requested a review from seanmonstar April 15, 2022 14:21

LucioFranco mentioned this pull request Apr 15, 2022

Audit use of unsafe in header/name.rs #428

Closed

seanmonstar approved these changes Apr 15, 2022

View reviewed changes

jeddenlea force-pushed the constname branch from 41a3df9 to 84ad04f Compare April 15, 2022 23:49

jeddenlea requested a review from seanmonstar April 15, 2022 23:49

jeddenlea added a commit to jeddenlea/http that referenced this pull request Apr 15, 2022

Merge remote-tracking branch 'remotes/sbosnick/audit_header_name' int…

9bede3a

…o constname_merge Merges hyperium#428 into hyperium#499

seanmonstar approved these changes Apr 16, 2022

View reviewed changes

seanmonstar merged commit d8b35db into hyperium:master Apr 16, 2022

jeddenlea mentioned this pull request Apr 27, 2022

From impls of PathAndQuery and Authority for Uri #538

Merged

TheBlueMatt reviewed Apr 29, 2022

View reviewed changes

lperlaki mentioned this pull request May 6, 2022

declare const headers datenlord/s3-server#117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make HeaderName::from_static const #499

Make HeaderName::from_static const #499

jeddenlea commented Aug 20, 2021 •

edited

Loading

seanmonstar commented Feb 24, 2022

carllerche commented Mar 16, 2022

jeddenlea commented Mar 17, 2022

jeddenlea commented Mar 17, 2022

npmccallum commented Apr 2, 2022

jeddenlea Apr 6, 2022

LucioFranco left a comment

LucioFranco Apr 15, 2022

seanmonstar Apr 15, 2022

jeddenlea Apr 15, 2022

seanmonstar left a comment

TheBlueMatt Apr 29, 2022

LucioFranco May 2, 2022

TheBlueMatt May 2, 2022

LucioFranco May 2, 2022

TheBlueMatt May 2, 2022

	/// \| inside `fastly::http::HeaderName::from_static` at http/src/header/name.rs:1241:13
	/// \| inside `http::HeaderName::from_static` at http/src/header/name.rs:1241:13

Make HeaderName::from_static const #499

Make HeaderName::from_static const #499

Conversation

jeddenlea commented Aug 20, 2021 • edited Loading

seanmonstar commented Feb 24, 2022

carllerche commented Mar 16, 2022

jeddenlea commented Mar 17, 2022

jeddenlea commented Mar 17, 2022

npmccallum commented Apr 2, 2022

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmonstar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeddenlea commented Aug 20, 2021 •

edited

Loading