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

Feature request: compile-time hashes for HeaderName::from_static #653

Open
itrofimow opened this issue Nov 30, 2023 · 9 comments · May be fixed by #659
Open

Feature request: compile-time hashes for HeaderName::from_static #653

itrofimow opened this issue Nov 30, 2023 · 9 comments · May be fixed by #659
Labels
A-headers Area: HTTP headers B-rfc Blocked: request for comments. More discussion would help move this along. S-performance Severity: performance. Make existing functionality go faster.

Comments

@itrofimow
Copy link

Hi!
An almost complete rust newbie here (coming from C++, mostly), so please forgive my ignorance.


I believe it's a pretty common practice to have some company-wide standard headers for, say, tracing/authorization/etc., which are de-facto StandardHeaders in that sense.
However, since they aren't actually values of that enum, hashing them calls into FNV (i assume hashing a StandardHeader only hashes its discriminant, am i right?), and that FNV usage is measurable.

For example, i took the axum implementation from TechEmpower benchmarks, and did this: https://github.com/itrofimow/FrameworkBenchmarks/pull/7/files
Looking at flamegraphs, the contains_key amounts to ~3% of total CPU usage, most of it being spent in FNV, when replacing the CUSTOM_HEADERS with [ACCESS_CONTROL_ALLOW_CREDENTIALS, ACCESS_CONTROL_ALLOW_HEADERS, ACCESS_CONTROL_EXPOSE_HEADERS] (something of comparable length, basically) leads to the contains_key only taking ~0.3%.


Does it make sense to implement the optimization proposed, or do its drawbacks outweigh its performance benefits?
I have a clear understanding of how that could be done in C++ (we actually do so in the web-framework of ours, with its HeaderMap implementation being heavily inspired by what hyper offers), and would be glad to give it a try in Rust.

@seanmonstar
Copy link
Member

Hey there!

It could make sense, I think it would depend on what the implementation looks like. Would you be willing to discuss it here first? That could help save time before code is written :)

@seanmonstar seanmonstar added B-rfc Blocked: request for comments. More discussion would help move this along. A-headers Area: HTTP headers S-performance Severity: performance. Make existing functionality go faster. labels Nov 30, 2023
@itrofimow
Copy link
Author

Thank you for the, dare i say, blazingly fast response :)

Sure. In my mind it could be implemented either via adding an Option<u64> compile_time_hash into Custom, with the hash implementation hijacked for the fast-hash case, or adding a new CustomStatic representation, which basically does the same, but is a separate type.
I don't really know yet how (and where) to hijack the hash, but i assume it should be doable in somewhat straightforward way; i'd need to dive a bit deeper into the project to figure it out, which i'm ready to do.

In both cases something like https://crates.io/crates/const-fnv1a-hash would be needed, too.

@seanmonstar
Copy link
Member

It's a fine thing to explore, but we'll want to weigh how much adding that affects performance of other parts. If it's much slower in other paths, we might not end up accepting the change.

I imagine we don't really need another dependency, the FNV algorithm is tiny and having the const fns in this repo would be fine (probably even removing the need for the fnv dependency in the first place).

@itrofimow
Copy link
Author

I've got a draft for this, which right now apparently breaks dependent crates because of

error: to use a constant of type http::header::name::Repr<http::header::name::Custom> in a pattern, http::header::name::Repr<http::header::name::Custom> must be annotated with #[derive(PartialEq, Eq)]

Not really sure what this is about, but i guess there should be a workaround.


Benchmarks show a slowdown for header_name_from_static, which is expected, however other benchmarks are a surprise to me: https://pastebin.com/raw/N1Mqd7EE.
Everything header_map-related became faster, some benchmarks for other map implementations became substantially slower, and i have no clue why.

These results confuse me, and i would assume I've done something completely nonsensical if not for the tests passing; could you @seanmonstar please have a glance on this? - I don't request a review yet, more of a sanity check, and whether this direction has potential.

@dmitrmax
Copy link

dmitrmax commented Aug 5, 2024

How do you define your company-wide standard headers in the code?

I'm asking because I think your issue is well connected with the issue #174 which was closed without any actions long time ago. But I think it's time for it.

@itrofimow
Copy link
Author

My case is a C++-based one, and for me it goes like this, with the hash being calculated at compile time.
The most common headers we define within the framework itself, but nothing stops our users from declaring their own constexpr headers (and we do so internally)
@dmitrmax

@dmitrmax
Copy link

dmitrmax commented Aug 5, 2024

Oh, I thought you are using this framework in Rust to define such headers, that is why I've asked. Because I use lazy_static to define them and this looks ugly since I have to dereference them when putting these headers into a request. This breaks common style when standard headers are mixed with the custom ones.

@itrofimow
Copy link
Author

I think one should be able to declare const headers via HeaderName::from_static, so the functionality is there, and my question is rather about potential performance gains in that case

@dmitrmax
Copy link

dmitrmax commented Aug 5, 2024

We are talking about different matters. Of course HeaderName::from_static should be used. But I'm rather talking about declaring a global constant. I don't see currenly any good way to declare and use it in the same manner as the standart headers can be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-headers Area: HTTP headers B-rfc Blocked: request for comments. More discussion would help move this along. S-performance Severity: performance. Make existing functionality go faster.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants