-
Notifications
You must be signed in to change notification settings - Fork 20
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
Export cold_path() in std::hint #510
Comments
I have a minor compiler concern for this proposal: The proposed library implementation of #[inline(always)]
pub fn cold_path() {
std::intrinsics::cold_path()
} given the current compiler implementation, relies on While technically this is a hint so it's fine to do nothing, and branch hints are unlikely to be useful in compilations that disable the MIR inliner, it would be a bummer if this got stabilized, then later we figured out a better way to do branch hints and had to deprecate this one. Perhaps the reliance on the MIR inliner will be alleviated by rust-lang/rust#134082 but at the time of writing that PR isn't even merged. It might be by the time this ACP gets discussed, who knows. |
if we run into compiler issues, the #[cold]
pub fn cold_path() {} this should work since it is entirely stable code now, even if less optimal. |
Hashbrown without nightly feature had such a formulation for a while but removed it because it didn’t work. I guess it has the opposite problem: rather than relying on MIR inlining, it’s too easily optimized out (by the inliner or other passes) before it can influence branch weights. |
@hanna-kruppe Before rust-lang/rust#120370 was merged, such a formulation would not work. rustc doesn't inline a cold function regardless how small it is. But LLVM does and doesn't set the branch weights. Now, there is code that detects calls to cold functions and sets the weights. |
I have not reviewed that PR but I wonder: if the formulation with a |
@hanna-kruppe Intrinsic gives us more control. Without it, the functionality depends on two assumptions:
At the moment both assumptions hold. With intrinsic, however, we can make sure that future updates will not break one or both of them.
|
Thanks for explaining. Sounds like the slightly generalized version of @saethlin's concern, that |
The intrinsic-less version depends on whether a tiny function marked as cold gets inlined. I would call this implementation detail. The export of intrinsic in But yes, it's a valid concern and hopefully rust-lang/rust#134082 will fix it. |
Proposal
Problem statement
It is sometimes helpful to let the compiler know what code path is the fast path, so it can be optimized at the expense of the slow path. This proposal suggests that the
cold_path()
intrinsic is simple and reliable way to provide this information and it could be reexported instd::hint
.grep-ing the LLVM source code for
BlockFrequencyInfo
andBranchProbabilityInfo
shows that this information is used at many places in the optimizer. Such as:Motivating examples or use cases
The
cold_path()
call can be simply placed on some code path marking it as cold.Solution sketch
This is already implemented in intrinsics. All we have to do is create a wrapper in
std::hint
:Alternatives
likely
/unlikely
These are harder to use for idiomatic Rust. For example they don't work well with match arms. On the other hand, sometimes they can be more readable and may also be worth reexporting in
std::hint
.For example, this would be harder to express using
cold_path()
:And this looks better without the extra branch:
extending the functionality of
#[cold]
attributeThis attribute could be allowed on match arms or on closures. I'm not sure if it's worth it adding extra syntax if the functionality can be implemented by a library call.
Links and related work
rust-lang/rust#120370 - added
cold_path()
as part of fixinglikely
andunlikely
rust-lang/rust#133852 - improvements of the
cold_path()
implementationrust-lang/rust#120193 - proposal for
#[cold]
on match armsThe text was updated successfully, but these errors were encountered: