Emit less MIR for format_args! with just a str literal #110739
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
added
S-waiting-on-review
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 727864f95a5b6bc7ab5f711946819bd1bc6ad408 with merge 5d61f3d763957cca65fdc70d016c4ef3b9bc2d38... |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (5d61f3d763957cca65fdc70d016c4ef3b9bc2d38): 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.
|
rustbot
added
perf-regression
m-ou-se
reviewed
Apr 24, 2023
| @@ -20,10 +21,39 @@ pub fn expand_panic<'cx>( | |||
| sp: Span, | |||
| tts: TokenStream, | |||
| ) -> Box<dyn MacResult + 'cx> { | |||
| let mac = if use_panic_2021(sp) { sym::panic_2021 } else { sym::panic_2015 }; | |||
| let mac = if is_simple_str(&tts) { | |||
| sym::panic_2015 | |||
There was a problem hiding this comment.
core::panic_2015 accepts non-static str literals, so this will result in less efficient code. It will call panic_str("…") which will call panic_display("…"), which will call panic_fmt with a format_args!("{}", "…") argument, which pulls in the Display for &str implementation. panic_2021 calls panic_fmt with a format_args!("…") argument, which does not pull in that Display implementation.
|
The objective of this PR is to reduce the amount of MIR that we generate in the function body that expands If |
That depends on what a
That should be possible. In one of my (not very finished) experiments, |
|
Note that in std you can use the |
|
Hah! Yesterday @scottmcm was telling me to PR changing all the
That would be an improvement over what we have right now, but...
😅 I think the game I'm playing here might be more arcane than I realized, so here's a godbolt demo of what I'm talking about: https://godbolt.org/z/4hWM9cY7b Explanation: |
That should be relatively simple to test. Here's the hir expansion that needs to be wrapped in a rust/compiler/rustc_ast_lowering/src/format.rs Lines 427 to 435 in b72460f |
|
Oops, I totally forgot that |
saethlin
changed the title
|
Two incremental tests now fail, and I'm not exactly sure why. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 9f7e7f0 with merge ff9187b56dc906f349b06ad92fc36298f414dc67... |
|
☀️ Try build successful - checks-actions |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@rust-timer build ff9187b56dc906f349b06ad92fc36298f414dc67 |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ff9187b56dc906f349b06ad92fc36298f414dc67): 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.
|
rustbot
removed
the
S-waiting-on-perf
|
@m-ou-se I think the regression is connected to the failing tests. I think I goofed something in the Also I'm extra confident that I messed something up here because panic backtraces now look like this (just running a hello world): |
|
Ah 🤦 the problem with the backtrace is that I built the standard library without debuginfo. |
|
I think my idea here doesn't work because we don't optimize consts, so we actually end up with substantially more code in the end, because the subsequent benefits of shrinking the inline code size here doesn't net out. I also think that this general situation can be solved by better constant propagation. We should have that eventually. |
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.
With #110705, this is the code we now emit for
panic!("ow")in an optimized build build:I would prefer to at least see this:
Currently this PR emits:
r? @ghost