Skip to content
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.

Sign up for GitHub

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

Help LLVM understand that some spans are never going to do anything #1600

Merged
merged 4 commits into from
Sep 30, 2021
Merged

Help LLVM understand that some spans are never going to do anything #1600

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
feda6ee
Help LLVM understand that some spans are never going to do anything
oli-obk Sep 29, 2021
94842ed
Pacify clippy
oli-obk Sep 29, 2021
27bd20e
Explain the lazy variable initialization
oli-obk Sep 29, 2021
b1b0de2
Apply the static level optimization to async fns and error printing
oli-obk Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 51 additions & 8 deletions tracing-attributes/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,8 @@ fn gen_block(
.map(|name| quote!(#name))
.unwrap_or_else(|| quote!(#instrumented_function_name));

let level = args.level();

// generate this inside a closure, so we can return early on errors.
let span = (|| {
// Pull out the arguments-to-be-skipped first, so we can filter results
Expand Down Expand Up @@ -448,7 +450,6 @@ fn gen_block(
}
}

let level = args.level();
let target = args.target();

// filter out skipped fields
Expand Down Expand Up @@ -515,7 +516,9 @@ fn gen_block(
if err {
quote_spanned!(block.span()=>
let __tracing_attr_span = #span;
tracing::Instrument::instrument(async move {
// See comment on the default case at the end of this function
// for why we do this a bit roundabout.
let fut = async move {
match async move { #block }.await {
#[allow(clippy::unit_arg)]
Ok(x) => Ok(x),
Expand All @@ -524,22 +527,46 @@ fn gen_block(
Err(e)
}
}
}, __tracing_attr_span).await
};
if tracing::level_enabled!(#level) {
tracing::Instrument::instrument(
fut,
__tracing_attr_span
)
.await
Comment on lines +531 to +536
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case, since we are always creating the span, we could actually do this:

Suggested change
if tracing::level_enabled!(#level) {
tracing::Instrument::instrument(
fut,
__tracing_attr_span
)
.await
if !span.is_disabled() {
tracing::Instrument::instrument(
fut,
__tracing_attr_span
)
.await

which would also let us skip instrumenting the future in the case where the span's level is enabled but it was disabled by the subscriber (e.g. if its target was not enabled)

} else {
fut.await
}
Comment on lines +531 to +539
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, we could probably also do the optimization of not creating the span at all if the level is disabled:

Suggested change
if tracing::level_enabled!(#level) {
tracing::Instrument::instrument(
fut,
__tracing_attr_span
)
.await
} else {
fut.await
}
if tracing::level_enabled!(#level) {
let __tracing_attr_span = #span;
if !span.is_disabled() {
return tracing::Instrument::instrument(
fut,
__tracing_attr_span
)
.await;
}
fut.await

)
} else {
quote_spanned!(block.span()=>
let __tracing_attr_span = #span;
// See comment on the default case at the end of this function
// for why we do this a bit roundabout.
let fut = async move { #block };
if tracing::level_enabled!(#level) {
tracing::Instrument::instrument(
async move { #block },
fut,
__tracing_attr_span
)
.await
} else {
fut.await
}
)
}
} else if err {
quote_spanned!(block.span()=>
let __tracing_attr_span = #span;
let __tracing_attr_guard = __tracing_attr_span.enter();
// See comment on the default case at the end of this function
// for why we do this a bit roundabout.
let __tracing_attr_span;
let __tracing_attr_guard;
if tracing::level_enabled!(#level) {
__tracing_attr_span = #span;
__tracing_attr_guard = __tracing_attr_span.enter();
}
// pacify clippy::suspicious_else_formatting
let _ = ();
#[allow(clippy::redundant_closure_call)]
match (move || #block)() {
#[allow(clippy::unit_arg)]
Expand All @@ -552,8 +579,24 @@ fn gen_block(
)
} else {
quote_spanned!(block.span()=>
let __tracing_attr_span = #span;
let __tracing_attr_guard = __tracing_attr_span.enter();
// These variables are left uninitialized and initialized only
// if the tracing level is statically enabled at this point.
// While the tracing level is also checked at span creation
// time, that will still create a dummy span, and a dummy guard
// and drop the dummy guard later. By lazily initializing these
// variables, Rust will generate a drop flag for them and thus
// only drop the guard if it was created. This creates code that
// is very straightforward for LLVM to optimize out if the tracing
// level is statically disabled, while not causing any performance
// regression in case the level is enabled.
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
let __tracing_attr_span;
let __tracing_attr_guard;
if tracing::level_enabled!(#level) {
__tracing_attr_span = #span;
__tracing_attr_guard = __tracing_attr_span.enter();
}
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
// pacify clippy::suspicious_else_formatting
let _ = ();
#block
)
}
Expand Down