panic: Use local functions in panic! whenever possible
#128068
Conversation
This is basically extending the idea implemented in `panic_2021!`. By creating a cold/noinline function that wraps the setup for the call to the internal panic function moves more of the code-size cost of `panic!` to the cold path. For example: https://godbolt.org/z/T1ndrcq4d
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @scottmcm (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
library/core/src/panic.rs
Outdated
| #[rustc_const_panic_str] // enforce a &&str argument in const-check and hook this by const-eval | ||
| #[rustc_do_not_const_check] // hooked by const-eval | ||
| const fn panic_cold_display<T: $crate::fmt::Display>(arg: &T) -> ! { | ||
| $crate::panicking::panic($crate::format_args!("internal error: entered unreachable code: {}", *arg)); |
There was a problem hiding this comment.
Can this use panic_fmt and const_format_args?
unreachable_2015 and unreachable_2021 format string arms are also suspect, and the ($($t:tt)+) for the one in 2021 is inconsistent. Not sure if that's a macro thing though.
There was a problem hiding this comment.
re:
unreachable_2015 and unreachable_2021 format string arms are also suspect, and the ($($t:tt)+) for the one in 2021 is inconsistent. Not sure if that's a macro thing though.
I don't understand, what do you mean by this?
There was a problem hiding this comment.
Some of the cases for general format_args! usage use ($($t:tt)+) instead of an $fmt:expr first. There's also some inconsistency with panic! being used within the macro itself. I think it can all be done with panic_fmt, concat!, and const_format_args!.
Edit: I guess the ($($t:tt)+) is for the 2021 relegate-to-format_args! behavior and is intentional?
There was a problem hiding this comment.
Also, unreachable_2015! doesn't have a special case for "{}" for const_panic. These macros are very inconsistent.
There was a problem hiding this comment.
These macros are very inconsistent
Agreed!
|
One question I had when working on this patch, is: Is there a way to handle string literals in the 2021 implementations? AFAICT the most common panic usage is something like I tried something like: Which works for something like |
Done |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Okay, I realized I had completely messed up my local testing setup, might take a bit for me to fixup all the tests (which I'm not able to reproduce locally). I'm going to mark this as a draft until I've sorted it all out. |
workingjubilee
dismissed
their stale review
Yesterday's Jubilee was slightly confused.
There was a problem hiding this comment.
@saethlin I know you have done some work to specifically alter how panic codegen operates, because we do some pretty unusual things. Do you have any thoughts on whether this PR is a feasible approach?
|
It looks like this approach is trying to fold the I feel like this approach will look good when browsing the assembly for functions, but it's unclear to me if this will make code size on net worse. I think the better win is figuring out how to mash all 3 instructions that are currently required for setting up the arguments into 1. The fact that we have some instructions in a Anyway, we have a bot that can give us some idea of what the code size impact is. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
library/core/src/panic.rs
Outdated
| #[cold] | ||
| #[track_caller] | ||
| #[inline(never)] | ||
| const fn panic_cold_explicit() -> ! { | ||
| $crate::panicking::panic("explicit panic"); | ||
| } |
There was a problem hiding this comment.
As it's the same everywhere, this could be moved to panicking.
library/core/src/panic.rs
Outdated
| #[cold] | ||
| #[track_caller] | ||
| #[inline(never)] | ||
| const fn unreachable_cold_explicit() -> ! { | ||
| $crate::panicking::panic("internal error: entered unreachable code"); | ||
| } |
library/core/src/panic.rs
Outdated
| #[cold] | ||
| #[track_caller] | ||
| #[inline(never)] | ||
| const fn unreachable_cold_explicit() -> ! { | ||
| $crate::panicking::panic("internal error: entered unreachable code"); | ||
| } |
| #[cold] | ||
| #[track_caller] | ||
| #[inline(never)] | ||
| const fn panic_cold_explicit() -> ! { | ||
| $crate::rt::begin_panic("explicit panic"); | ||
| } |
This comment has been minimized.
This comment has been minimized.
Sorry for the consistent issue w/ this test. When I run |
|
No need to apologize, you're doing great :)
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Ayy the tests all pass :) think can try re-running bors perf test. |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…ppers, r=<try> panic: Use local functions in `panic!` whenever possible This is basically extending the idea implemented in `panic_2021!`. By creating a cold/noinline function that wraps the setup for the call to the internal panic function moves more of the code-size cost of `panic!` to the cold path. For example: https://godbolt.org/z/T1ndrcq4d
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (216574a): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary -5.2%, secondary 4.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary -7.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary 0.1%, secondary -0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 771.005s -> 770.634s (-0.05%) |
rustbot
added
perf-regression
|
Mmmh, that's not that great. Perhaps you could try using |
IIUC this is basically saying the result has a [-.3%, 1.8%] compile time regression (as in time in Edit: |
|
|
|
|
Just removing the |
Not yet unfortunately, I have a patch to llvm to try to fix that though: llvm/llvm-project#101298 I might revisit this patch later, but at the moment I'm looking into trying to enable hot-cold splitting on the default optimized llvm pipeline which will solve the issue with hopefully less compile time impact (and for other languages as well). |
@goldsteinn I'm closing this due to inactivity. @rustbot label: +S-inactive |
rustbot
added
the
S-inactive
This is basically extending the idea implemented in
panic_2021!.By creating a cold/noinline function that wraps the setup for the call to the internal panic function moves more of the code-size cost of
panic!to the cold path.For example: https://godbolt.org/z/T1ndrcq4d