Skip to content

Make sure std::panic::Location::caller() gets optimized away. #93031

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

Closed

Conversation

michaelwoerister
Copy link
Member

I noticed that using #[track_caller] and std::panic::Location::caller() result in function calls being generated even though all the information is available at compile time and std::panic::Location::caller() is basically a no-op.

Marking std::panic::Location::caller() with #[inline] allows the compiler to optimize these calls out.

Without #[inline] a call to std::panic::Location::caller() will result in LLVM IR like:

; The caller locations are available in constants
@alloc1 = private unnamed_addr constant <{ [56 x i8] }> <{ [56 x i8] c"src/test/codegen/track_caller_inlined.rs" }>, align 1
@alloc2 = private unnamed_addr constant <{ i8*, [16 x i8] }> <{ i8* getelementptr inbounds (<{ [56 x i8] }>, <{ [56 x i8] }>* @alloc1, i32 0, i32 0, i32 0), [16 x i8] c"8\00\00\00\00\00\00\00\11\00\00\00\05\00\00\00" }>, align 8

define void @foo() unnamed_addr #0 {
start:
  %0 = alloca %"core::panic::location::Location"*, align 8
  ; call core::panic::location::Location::caller
  %_2.i = tail call align 8 dereferenceable(24) %"core::panic::location::Location"* @_ZN4core5panic8location8Location6caller17hec6a7053b6d2c145E(%"core::panic::location::Location"* noalias readonly align 8 dereferenceable(24) bitcast (<{ i8*, [16 x i8] }>* @alloc2 to %"core::panic::location::Location"*))
  store %"core::panic::location::Location"* %_2.i, %"core::panic::location::Location"** %0, align 8, !noalias !2

Notice that the constant @alloc2 is passed into a call to Location::caller() just to be returned from it unmodified.

With the #[inline] annotation that call just goes away:

  %0 = alloca %"core::panic::location::Location"*, align 8
  store %"core::panic::location::Location"* bitcast (<{ i8*, [16 x i8] }>* @alloc2 to %"core::panic::location::Location"*), %"core::panic::location::Location"** %0, align 8

@rust-highfive
Copy link
Contributor

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 18, 2022
@michaelwoerister
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 18, 2022
@bors
Copy link
Collaborator

bors commented Jan 18, 2022

⌛ Trying commit 543e77f with merge 6b9358f887a37cd96b09b534cb87993c48f0201e...

@bors
Copy link
Collaborator

bors commented Jan 18, 2022

☀️ Try build successful - checks-actions
Build commit: 6b9358f887a37cd96b09b534cb87993c48f0201e (6b9358f887a37cd96b09b534cb87993c48f0201e)

@rust-timer
Copy link
Collaborator

Queued 6b9358f887a37cd96b09b534cb87993c48f0201e with parent 7531d2f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6b9358f887a37cd96b09b534cb87993c48f0201e): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking 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 led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 18, 2022
@bjorn3
Copy link
Member

bjorn3 commented Jan 18, 2022

Overall it seems to be a slight (<0.3%) improvement.

@michaelwoerister
Copy link
Member Author

I was under the impression that some logging frameworks might generate many calls to Location::caller(). Not inlining those calls might have a noticeable effect on code size and performance (although it's unlikely to be the dominant factor).

In any case, this seems to be a small win overall.

@michaelwoerister michaelwoerister added the A-codegen Area: Code generation label Jan 31, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 6, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2022
@michaelwoerister
Copy link
Member Author

#96348 does the same thing. Closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants