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

Reexport likely/unlikely in std::hint #133695

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

x17jiri
Copy link
Contributor

@x17jiri x17jiri commented Dec 1, 2024

Since likely/unlikely should be working now, we could reexport them in std::hint. I'm not sure if this is already approved or if it requires approval

Tracking issue: #26179

@rustbot
Copy link
Collaborator

rustbot commented Dec 1, 2024

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 1, 2024
@ChrisDenton
Copy link
Member

This would need lang and libs-api to look over. I'd note that the discussion in the tracking issue seems to be saying that this is the wrong API. In particular a #[cold] attribute or maybe a cold_path() function would be a better way forward.

@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

Yes, I agree with #26179 (comment) that likely/unlikely is the wrong API (the current implementation makes it look very silly, a roundabout way to just use cold_path) and I think we should move forward with cold_path, it's extremely simple and more reliable than likely/unlikely (and maybe #[cold] for match arms).

I think the next step would be to create an ACP for cold_path: https://std-dev-guide.rust-lang.org/development/feature-lifecycle.html

@x17jiri
Copy link
Contributor Author

x17jiri commented Dec 1, 2024

cold_path() will need an update in the codegen to make it more useful. If we decide it's the way to go, I can work on ït.

Basically the current implementation gathers information about which code paths are cold, but this information is only used in select with 2 options. This was all I needed to fix likely and unlikely and I didn't want to make the PR bigger than needed.

However common patterns in Rust generate select with more than 2 options. For example this code:

if let Some(x) = t { ... }

May generate something equivalent to:

match t.discriminator {
1 => true branch,
0 => false branch, 
_ => unreachable
}

@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

"the compiler currently doesn't support the better option" sounds fixable.

@x17jiri
Copy link
Contributor Author

x17jiri commented Dec 2, 2024

@workingjubilee I'm all for improving cold_path(). I'm the guy who invented it. I even said I can work on it :-)

But likely and unlikely are more readable in some situations. For example if likely(cond) { ... } vs if cond { ... } else { cold_path() }. You can also use hem for parts of conditions. For example if a && likely(b) { ... }

And since there is RFC I thought they may already be approved

@tgross35
Copy link
Contributor

If we had hint::cold_path() then we wouldn't need #[cold] on if/match (#120193) right? But if we instead had likely/unlikely, we would still need something different for match? Keeping things consolidated to a single feature seems like a nice advantage to me (we do already have #[cold] but applying attributes to match arms is pretty novel).

In any case you should file an ACP at https://github.com/rust-lang/libs-team/issues (it's just an issue template) for adding something new in hint.

And since there is RFC I thought they may already be approved

Given it was accepted about a decade ago, there is a good chance it would have to be revisited anyway :)

@x17jiri
Copy link
Contributor Author

x17jiri commented Dec 13, 2024

@tgross35 I'd like to have both likely/unlikely and cold_path, because cold_path can sometimes be awkward.

For example, I think:

if likely(cond) { ... }

is more readable than:

if cond { ... } else { cold_path() }

I'll look into the ACP and try to create one for all of these functions.

@tgross35
Copy link
Contributor

I agree it's not the most convenient when you have one branch that you want to prioritize rather than deprioritizing all others, for either cold_path() or #[cold]. In theory could we also have hint::hot_path()? Not saying we necessarily want both, but this would also work better than likely for match arms.

@workingjubilee
Copy link
Member

it might be useful to review https://blog.aaronballman.com/2020/08/dont-use-the-likely-or-unlikely-attributes/ which raises some relevant concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants