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

Ensure unreachable branch is eliminated #2708

Merged
merged 1 commit into from
Feb 18, 2023

Conversation

EFanZh
Copy link
Contributor

@EFanZh EFanZh commented Feb 13, 2023

With #2704, I introduced two unwrap() calls in Select::poll, but I found that with opt-level=z, the compiler won’t be able to optimize these unwrap() calls, so maybe changing them to unwrap_unchecked() is a better idea? See https://godbolt.org/z/cef118zKe for comparison with opt-level=z.

Also, since unwrap_unchecked() is introduced in Rust 1.58.0, I have to implement this function manually.

@EFanZh EFanZh requested a review from taiki-e as a code owner February 13, 2023 04:26
@EFanZh EFanZh force-pushed the unwrap-unchecked branch 2 times, most recently from 4a89f42 to 678b2dd Compare February 13, 2023 04:52
@EFanZh EFanZh changed the title Use unwrap_unchecked to ensure unreachable branch is eliminated Ensure unreachable branch is eliminated Feb 13, 2023
@taiki-e
Copy link
Member

taiki-e commented Feb 16, 2023

Hmm, given that opt-level=s works, I feel this is like a compiler bug. https://godbolt.org/z/sPTdMo1Kd

Copy link
Contributor

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless the performance gains are critical, I'd tend to err on keeping the safer version, and hoping that the compiler can be made smart enough eventually.

@EFanZh
Copy link
Contributor Author

EFanZh commented Feb 16, 2023

It seems that the #[inline] attribute on Option::unwrap is not enough for the compiler to apply inlining, which causes the compiler not being able to do the optimization. And if I implement unwrap manually (panic instead of UB) with #[inline(always)], the compiler is able to do the optimization: https://godbolt.org/z/55savPbaE. Is this change acceptable?

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The new approach looks good to me. (It is enough simple and has no unsafe code)

@taiki-e taiki-e merged commit b41ede5 into rust-lang:master Feb 18, 2023
@taiki-e taiki-e added the 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. label Mar 6, 2023
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants