-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Optimize panic!(<str lit>) into method call
#148375
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
bf67ced to
23ff45b
Compare
This comment has been minimized.
This comment has been minimized.
23ff45b to
d2b1a5f
Compare
This comment has been minimized.
This comment has been minimized.
8af6b0d to
16cafa7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Optimize `panic!(<str lit>)` into method call
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Finished benchmarking commit (d1479e2): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.8%, secondary -3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 3.0%, secondary 1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.2%, secondary -0.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 474.434s -> 474.79s (0.08%) |
|
Awesome, it actually worked! Marking as ready for review, though I'm going to add some doc changes and misc. cleanups in nearby code. The existing changes won't be altered. |
|
The Miri subtree was changed cc @rust-lang/miri These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
re. the pings: miri is simply blessing a test since the backtrace changed (expected), and clippy is updating the MSRV lint to handle |
This ignores `core`, `alloc`, and `std` rather than just `core`. The same reasoning applies.
16cafa7 to
e890c3b
Compare
|
@m-ou-se as the formatting guru…Would it be feasible to leverage the internals of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am not too familiar with this code, but it looks good to me. cc @rust-lang/libs on whether this is generally something you want
| && let TokenKind::Literal(lit) = &token.kind | ||
| && let Lit { kind: LitKind::Str | LitKind::StrRaw(_), symbol, .. } = lit | ||
| && let msg = symbol.as_str() | ||
| && !msg.contains(|c| c == '{' || c == '}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very much a hack. It would be much better to use rustc_parse_format here.
In particular since I think your version would not optimize panic!("foo {{ bar }}"); since it doesn't know about the escaping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aware. I tried to use that crate but couldn't figure out how to get it to work despite multiple approaches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should work:
if tts.len() == 1
&& let Some(TokenTree::Token(token, _)) = tts.get(0)
&& let TokenKind::Literal(lit) = &token.kind
&& let Lit { kind: litkind, symbol, .. } = lit
&& let Some(style) = match litkind {
LitKind::Str => Some(None),
LitKind::StrRaw(n) => Some(Some(*n as usize)),
_ => None,
}
&& let mut parser = rustc_parse_format::Parser::new(
symbol.as_str(),
style,
None,
true,
rustc_parse_format::ParseMode::Format,
)
&& let Some(rustc_parse_format::Piece::Lit(msg)) = parser.next()
&& let None = parser.next()
{
let msg = match mac {
InnerCall::Panic2015 | InnerCall::Panic2021 => cx.expr(sp, ExprKind::Lit(*lit)),
InnerCall::Unreachable2015 | InnerCall::Unreachable2021 => cx.expr_str(
sp,
Symbol::intern(&format!("internal error: entered unreachable code: {msg}")),
),
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that works for the most part! Just one test for a lint failing that I'll look into. That gets part of the way towards what I'd love to have, which is even leveraging the optimizations of inlining literals and similar.
My first impression of this PR is that it seems like a lot of complexity on the macro expansion side, and that it feels like this should be doable by using |
|
A more extreme approach would be to optimise |
In some places, the standard library calls out to a standalone method (
core::panicking::panic) when panicking to avoid unnecessary use of the formatting machinery. This PR attempts to optimizepanic!calls directly and for everyone, rather than requiring manual effort to call the method.The current approach is to intercept
panic!andunreachable!when it's going thru edition resolution. If any only if the macro call consists of exactly one string literal and there are no formatting placeholders (determined laxly by searching for braces in the string), then we pass the message into the method call. In the case ofunreachable!, we need to prepend the message prefix, necessitating unescaping as that info isn't saved at this point of the compilation process.As with any optimization, I think this will work, but a perf run will be necessary.