-
Notifications
You must be signed in to change notification settings - Fork 729
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
tracing: ensmallerate assembly generated by macro expansions #943
Changes from 8 commits
f810039
540a158
182d1b4
1d31fe1
283dcb9
4a52a12
ec43888
9d458e2
248c7d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -907,9 +907,9 @@ pub mod subscriber; | |
|
||
#[doc(hidden)] | ||
pub mod __macro_support { | ||
pub use crate::callsite::Callsite as _; | ||
pub use crate::callsite::Callsite; | ||
use crate::stdlib::sync::atomic::{AtomicUsize, Ordering}; | ||
use crate::{subscriber::Interest, Callsite, Metadata}; | ||
use crate::{subscriber::Interest, Metadata}; | ||
use tracing_core::Once; | ||
|
||
/// Callsite implementation used by macro-generated code. | ||
|
@@ -938,59 +938,90 @@ pub mod __macro_support { | |
/// without warning. | ||
pub const fn new(meta: &'static Metadata<'static>) -> Self { | ||
Self { | ||
interest: AtomicUsize::new(0), | ||
interest: AtomicUsize::new(0xDEADFACED), | ||
meta, | ||
registration: Once::new(), | ||
} | ||
} | ||
|
||
/// Returns `true` if the callsite is enabled by a cached interest, or | ||
/// by the current `Dispatch`'s `enabled` method if the cached | ||
/// `Interest` is `sometimes`. | ||
/// Registers this callsite with the global callsite registry. | ||
/// | ||
/// If the callsite is already registered, this does nothing. | ||
/// | ||
/// /!\ WARNING: This is *not* a stable API! /!\ | ||
/// This method, and all code contained in the `__macro_support` module, is | ||
/// a *private* API of `tracing`. It is exposed publicly because it is used | ||
/// by the `tracing` macros, but it is not part of the stable versioned API. | ||
/// Breaking changes to this module may occur in small-numbered versions | ||
/// without warning. | ||
#[inline(always)] | ||
pub fn is_enabled(&self) -> bool { | ||
let interest = self.interest(); | ||
if interest.is_always() { | ||
return true; | ||
} | ||
if interest.is_never() { | ||
return false; | ||
#[inline(never)] | ||
// This only happens once (or if the cached interest value was corrupted). | ||
#[cold] | ||
pub fn register(&'static self) -> Interest { | ||
self.registration | ||
.call_once(|| crate::callsite::register(self)); | ||
match self.interest.load(Ordering::Relaxed) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it actually worse here to call the inlined interest fn here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be fine for it to be inlined into this function, since this function shouldn't be inlined. The reason it doesn't is that the This shouldn't happen ever, but if there's a bug elsewhere, I'd rather just assume the cached value is invalid and return |
||
0 => Interest::never(), | ||
2 => Interest::always(), | ||
_ => Interest::sometimes(), | ||
} | ||
|
||
crate::dispatcher::get_default(|current| current.enabled(self.meta)) | ||
} | ||
|
||
/// Registers this callsite with the global callsite registry. | ||
/// | ||
/// If the callsite is already registered, this does nothing. | ||
/// Returns the callsite's cached Interest, or registers it for the | ||
hawkw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// first time if it has not yet been registered. | ||
/// | ||
/// /!\ WARNING: This is *not* a stable API! /!\ | ||
/// This method, and all code contained in the `__macro_support` module, is | ||
/// a *private* API of `tracing`. It is exposed publicly because it is used | ||
/// by the `tracing` macros, but it is not part of the stable versioned API. | ||
/// Breaking changes to this module may occur in small-numbered versions | ||
/// without warning. | ||
#[inline(always)] | ||
pub fn register(&'static self) { | ||
self.registration | ||
.call_once(|| crate::callsite::register(self)); | ||
} | ||
|
||
#[inline(always)] | ||
fn interest(&self) -> Interest { | ||
#[inline] | ||
pub fn interest(&'static self) -> Interest { | ||
match self.interest.load(Ordering::Relaxed) { | ||
0 => Interest::never(), | ||
1 => Interest::sometimes(), | ||
2 => Interest::always(), | ||
_ => Interest::sometimes(), | ||
_ => self.register(), | ||
} | ||
} | ||
|
||
pub fn dispatch_event(&'static self, interest: Interest, f: impl FnOnce(&crate::Dispatch)) { | ||
tracing_core::dispatcher::get_current(|current| { | ||
if interest.is_always() || current.enabled(self.meta) { | ||
f(current) | ||
} | ||
}); | ||
} | ||
|
||
#[inline] | ||
#[cfg(feature = "log")] | ||
pub fn disabled_span(&self) -> crate::Span { | ||
crate::Span::new_disabled(self.meta) | ||
} | ||
|
||
#[inline] | ||
#[cfg(not(feature = "log"))] | ||
pub fn disabled_span(&self) -> crate::Span { | ||
crate::Span::none() | ||
} | ||
|
||
pub fn dispatch_span( | ||
&'static self, | ||
interest: Interest, | ||
f: impl FnOnce(&crate::Dispatch) -> crate::Span, | ||
) -> crate::Span { | ||
if interest.is_never() { | ||
return self.disabled_span(); | ||
} | ||
|
||
tracing_core::dispatcher::get_current(|current| { | ||
if interest.is_always() || current.enabled(self.meta) { | ||
return f(current); | ||
} | ||
self.disabled_span() | ||
}) | ||
.unwrap_or_else(|| self.disabled_span()) | ||
} | ||
} | ||
|
||
impl Callsite for MacroCallsite { | ||
|
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.
Adding this was necessary since constructing the
ValueSet
in a closure means we needed aFnOnce
so that values can be moved into the closure (which happens primarily when users wrap values infield::display
/field::debug
without borrowing). Not supporting this would break at least some code. I opted to add a new function that takes aFnOnce
, rather than capturing anOption<FnOnce>
into theFnMut
andtake
ing it, because that seemed significantly less efficient.We could probably commit to making this a public API (and even deprecate
get_default
, the version that takes aFnMut
). This is probably a more useful API thanget_default
taking aFnMut
... I didn't make it public here, though, because introducing a new public API seemed like a job for another PR.Also, in 0.2, the
Value
changes will makeValue::debug
andValue::display
also borrow rather than move. So, theFnOnce
will no longer be necessary, and we could, alternatively, remove this function in 0.2, rather than deprecating the existingget_current
.So, there should probably be an actual design discussion... :)