Skip to content

Conversation

xacrimon
Copy link

@xacrimon xacrimon commented Apr 26, 2025

There is no justifiable reason to force this addition here I think? It's meant purely as something the binary crate can enable if wanted if it makes more sense for performance in in their case. #[inline] is not a universal win, very often it can worsen performance and severely balloon binary sizes for no reason.

This feature flag simply adds the #[inline] attribute to a lot more methods within hashbrown, which in this case has the primary effect of lowering the LLVM inline threshold for relatively cold codepaths. TBH there is not a lot of reason to do this, hashbrown is already quite aggressive with explicit inlining hints without this feature and it only really makes sense to look at on an application per application basis.

IME it does usually look very good on microbenchmarks and seems like "free performance", but that line of reasoning falls apart completely in any sort of real binary with more than a couple hundred lines of code being executed on repeat.

@xacrimon
Copy link
Author

@kyren Do you have any context on why this was here in the first place? Like I'm curious if it was just something copied over from some existing code or if not, what the reasoning was because I don't track and would like to know if I am missing something here.

@dtolnay
Copy link

dtolnay commented Apr 26, 2025

Context: this was a default feature in hashbrown 0.14.3 at the time of #28, so it makes sense for that PR (which was focused on something different) to have kept it.

https://docs.rs/crate/hashbrown/0.14.3/source/Cargo.toml.orig#50

@xacrimon
Copy link
Author

Context: this was a default feature in hashbrown 0.14.3 at the time of #28, so it makes sense for that PR (which was focused on something different) to have kept it.

https://docs.rs/crate/hashbrown/0.14.3/source/Cargo.toml.orig#50

Ah, that makes sense then. Thank you.

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.

2 participants