Outline formatting code from the panic! macro #115562
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
Zoxc
force-pushed
the
panic-macro-size
branch
from
cbc8dd1 to
b48cbfb
Compare
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
That's a lot of complexity for an optimization. Maybe we could wait until #111693 can be landed and then implement this as an actual outlining mir-opt? @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit b48cbfbcbcbed74b13dec5b38f9517ef0194698a with merge 2e7ecf2eecd97617a1834d5e7f5d3ee4d43f4cdd... |
This comment has been minimized.
This comment has been minimized.
|
It'd be cool if we had a feature that allowed this entire change to just be the addition of a |
Zoxc
force-pushed
the
panic-macro-size
branch
from
b48cbfb to
1666baa
Compare
|
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
RalfJung
reviewed
Sep 6, 2023
| #[cold] | ||
| #[track_caller] | ||
| #[inline(never)] | ||
| #[rustc_do_not_const_check] // Allow the call to `panic_display`. |
There was a problem hiding this comment.
panic_display is a const fn, so why is this attribute needed?
There was a problem hiding this comment.
It's constrained to only work with &'static str by const checking, so we can't pass the argument of type &T to it.
There was a problem hiding this comment.
Ah... that's un-pretty.
I think a better approach would be to generalize what happens to panic_display. I'm imagining an attribute like #[const_panic_display] such that all functions with this attribute
- are allowed to be called in const context if the argument type is
&str - if evaluation reaches them, the magic hook kicks in (this branch)
That way, both the original fn panic_display and the outlined functions you are adding here can just get that attribute, and we don't need all this rustc_do_not_const_check and const_eval_select stuff here.
So there's no way to do this as a regular macro? |
Zoxc
force-pushed
the
panic-macro-size
branch
from
1666baa to
3c80038
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
There's a fair bit of verbosity, not too much complexity. An actual outlining pass would be far more complex. I'm also not sure how effective such a pass would be more generally.
We can kind of do this with closures today:
We need to do the outlining inside the expansion of |
|
Looks like I got unassigned, but I also wouldn't want a special case threaded through all the compilation for a minor optimization. |
|
sorry ^^ input is of course welcome from anyone, just didn't want to keep you on the hook if I'm assigning myself. |
flip1995
reviewed
Sep 6, 2023
| @@ -134,15 +136,56 @@ impl<'a, 'tcx> DivergenceVisitor<'a, 'tcx> { | |||
| } | |||
| } | |||
|
|
|||
| fn expr_might_diverge(e: &Expr<'_>) -> bool { | |||
There was a problem hiding this comment.
We have a really elaborate implementation fo diverging expressions in clippy_lints/src/manual_let_else.rs called expr_diverges. This can probably also be used here. For this, moving that function to clippy_utils is probably the best thing to do.
I initially had a macro which expanded differently in const fns, but I did avoid using it for this PR. I can move the constness tracking from macro expansion to AST lowering now. Being able to write macros that work both in and out of const fns seems to be quite desirable though, but I think a language feature like |
|
const closures are definitely planned (#106003), so if they would simplify this PR significantly, then maybe we should postpone this PR and link to it from the tracking issue as something that gets unblocked by it |
|
it seems the bot didn't enqueue the perf run or the force push messed up reporting the try build completion @rust-timer build 2e7ecf2eecd97617a1834d5e7f5d3ee4d43f4cdd |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (2e7ecf2eecd97617a1834d5e7f5d3ee4d43f4cdd): comparison URL. Overall result: ❌✅ regressions and improvements - 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)ResultsThis 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.
CyclesResultsThis 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 sizeResultsThis 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: 627.572s -> 632.07s (0.72%) |
rustbot
added
perf-regression
|
Closures doesn't work here, as we need I think I can do this more in the standard library if I restrict the change to format argument below a fixed number. That may have a larger compile time hit however. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 17, 2023
Partially outline code inside the panic! macro This outlines code inside the panic! macro in some cases. This is split out from rust-lang#115562 to exclude changes to rustc.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 1, 2023
…imulacrum Partially outline code inside the panic! macro This outlines code inside the panic! macro in some cases. This is split out from rust-lang#115562 to exclude changes to rustc.
|
☔ The latest upstream changes (presumably #115670) made this pull request unmergeable. Please resolve the merge conflicts. |
|
So, I had a cursory look here checking for progress. I think there a number of open comments here. I'm also looking at these two comments here and here, though not sure what's the current status and how do they compare with this patch. Anyway, switching to waiting on author for some feedbacks here and there, also for a rebase and a perf. run in need of triaging. Feel free to request a review with @rustbot author |
rustbot
added
S-waiting-on-author
flip1995
pushed a commit
to flip1995/rust-clippy
that referenced
this pull request
Oct 6, 2023
Partially outline code inside the panic! macro This outlines code inside the panic! macro in some cases. This is split out from rust-lang/rust#115562 to exclude changes to rustc.
RalfJung
pushed a commit
to RalfJung/miri
that referenced
this pull request
Oct 6, 2023
Partially outline code inside the panic! macro This outlines code inside the panic! macro in some cases. This is split out from rust-lang/rust#115562 to exclude changes to rustc.
|
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks! FYI: when a PR is ready for review, send a message containing |
lnicola
pushed a commit
to lnicola/rust-analyzer
that referenced
this pull request
Apr 7, 2024
Partially outline code inside the panic! macro This outlines code inside the panic! macro in some cases. This is split out from rust-lang/rust#115562 to exclude changes to rustc.
RalfJung
pushed a commit
to RalfJung/rust-analyzer
that referenced
this pull request
Apr 27, 2024
Partially outline code inside the panic! macro This outlines code inside the panic! macro in some cases. This is split out from rust-lang/rust#115562 to exclude changes to rustc.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes the
panic!macro generate an inner function which contains the formatting setup code. This reduces the size of the code for panics in callers. It also allows LLVM to eliminate passing of the#[track_caller]argument where it's statically known.In order to do this, a
panic_args!builtin macro is introduced. It has the same arguments asformat_args!, but will callpanic_fmtwith the generated arguments. It generates a new function in HIR for the formatting setup andpanic_fmtcall.Now this:
will expand to:
This should also reduce noise from panics in the performance measurements of #115129.
cc @m-ou-se