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.

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

core: Add intrusive linkedlist for callsite registry #988

Merged
merged 9 commits into from
Oct 2, 2020
218 changes: 196 additions & 22 deletions tracing-core/src/callsite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
use crate::stdlib::{
fmt,
hash::{Hash, Hasher},
sync::Mutex,
ptr,
sync::{
atomic::{AtomicPtr, Ordering},
Mutex,
},
vec::Vec,
};
use crate::{
Expand All @@ -13,15 +17,15 @@ use crate::{
};

lazy_static! {
static ref REGISTRY: Mutex<Registry> = Mutex::new(Registry {
callsites: Vec::new(),
dispatchers: Vec::new(),
});
static ref REGISTRY: Registry = Registry {
callsites: LinkedList::new(),
dispatchers: Mutex::new(Vec::new()),
};
}

struct Registry {
callsites: Vec<&'static dyn Callsite>,
dispatchers: Vec<dispatcher::Registrar>,
callsites: LinkedList,
dispatchers: Mutex<Vec<dispatcher::Registrar>>,
}

impl Registry {
Expand All @@ -30,8 +34,8 @@ impl Registry {

// Iterate over the subscribers in the registry, and — if they are
// active — register the callsite with them.
let mut interests = self
.dispatchers
let lock = self.dispatchers.lock().unwrap();
let mut interests = lock
.iter()
.filter_map(|registrar| registrar.try_register(meta));

Expand All @@ -47,9 +51,9 @@ impl Registry {
callsite.set_interest(interest)
}

fn rebuild_interest(&mut self) {
fn rebuild_interest(&self) {
let mut max_level = LevelFilter::OFF;
self.dispatchers.retain(|registrar| {
self.dispatchers.lock().unwrap().retain(|registrar| {
if let Some(dispatch) = registrar.upgrade() {
// If the subscriber did not provide a max level hint, assume
// that it may enable every level.
Expand All @@ -63,9 +67,9 @@ impl Registry {
}
});

self.callsites.iter().for_each(|&callsite| {
self.rebuild_callsite_interest(callsite);
});
self.callsites
.for_each(|cs| self.rebuild_callsite_interest(cs));

LevelFilter::set_max(max_level);
}
}
Expand All @@ -84,6 +88,9 @@ pub trait Callsite: Sync {
///
/// [metadata]: ../metadata/struct.Metadata.html
fn metadata(&self) -> &Metadata<'_>;

/// Retrive the callsites intrusive registration.
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved
fn registration(&'static self) -> &'static Registration;
}

/// Uniquely identifies a [`Callsite`]
Expand All @@ -105,6 +112,16 @@ pub struct Identifier(
pub &'static dyn Callsite,
);

/// A registration within the callsite cache.
///
/// Every callsite implementation must store this type internally to the
/// callsite and provide a `&'static Registration` reference via the
/// `Callsite` trait.
hawkw marked this conversation as resolved.
Show resolved Hide resolved
pub struct Registration<T = &'static dyn Callsite> {
callsite: T,
next: AtomicPtr<Registration<T>>,
}

/// Clear and reregister interest on every [`Callsite`]
///
/// This function is intended for runtime reconfiguration of filters on traces
Expand All @@ -125,24 +142,25 @@ pub struct Identifier(
/// [`Interest::sometimes()`]: ../subscriber/struct.Interest.html#method.sometimes
/// [`Subscriber`]: ../subscriber/trait.Subscriber.html
pub fn rebuild_interest_cache() {
let mut registry = REGISTRY.lock().unwrap();
registry.rebuild_interest();
REGISTRY.rebuild_interest();
}

/// Register a new `Callsite` with the global registry.
///
/// This should be called once per callsite after the callsite has been
/// constructed.
pub fn register(callsite: &'static dyn Callsite) {
let mut registry = REGISTRY.lock().unwrap();
registry.rebuild_callsite_interest(callsite);
registry.callsites.push(callsite);
REGISTRY.rebuild_callsite_interest(callsite);
REGISTRY.callsites.push(callsite);
}

pub(crate) fn register_dispatch(dispatch: &Dispatch) {
let mut registry = REGISTRY.lock().unwrap();
registry.dispatchers.push(dispatch.registrar());
registry.rebuild_interest();
REGISTRY
.dispatchers
.lock()
.unwrap()
.push(dispatch.registrar());
REGISTRY.rebuild_interest();
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved
}

// ===== impl Identifier =====
Expand All @@ -169,3 +187,159 @@ impl Hash for Identifier {
(self.0 as *const dyn Callsite).hash(state)
}
}

// ===== impl Registration =====

impl<T> Registration<T> {
/// Construct a new `Registration` from some `&'static dyn Callsite`
pub const fn new(callsite: T) -> Self {
Self {
callsite,
next: AtomicPtr::new(ptr::null_mut()),
}
}
}

impl fmt::Debug for Registration {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Registration").finish()
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved
}
}

// ===== impl LinkedList =====

/// An intrusive atomic push only linked-list.
hawkw marked this conversation as resolved.
Show resolved Hide resolved
struct LinkedList {
head: AtomicPtr<Registration>,
}

impl LinkedList {
fn new() -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

nit: could probably be a const fn (this isn't necessary right now, but it might be worth getting rid of the lazy_static around REGISTRY later, at least on no-std where we won't need a mutex eventually...

Copy link
Member

Choose a reason for hiding this comment

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

we can always add const fn later though.

LinkedList {
head: AtomicPtr::new(ptr::null_mut()),
}
}

fn for_each(&self, mut f: impl FnMut(&'static dyn Callsite)) {
let mut head = self.head.load(Ordering::Acquire);

while !head.is_null() {
let reg = unsafe { &*head };
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved
f(reg.callsite);

head = reg.next.load(Ordering::Acquire);
}
}

fn push(&self, cs: &'static dyn Callsite) {
let mut head = self.head.load(Ordering::Acquire);

loop {
let registration = cs.registration() as *const _ as *mut _;

cs.registration().next.store(head, Ordering::Release);

match self.head.compare_exchange(
head,
registration,
Ordering::AcqRel,
Ordering::Acquire,
Comment on lines +263 to +264
Copy link
Member

Choose a reason for hiding this comment

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

nit, take it or leave it: some of the code in this module might be a little clearer if we imported Ordering::*, but it's not a blocker.

) {
Ok(old) => {
assert_ne!(
cs.registration() as *const _,
old,
"Attempting to push a `Callsite` that already exists. \
This will cause an infinite loop when attempting to read from the \
callsite cache. This is likely a bug! You should only need to push a \
`Callsite` once."
);
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved
break;
}
Err(current) => head = current,
}
}
}
}

#[cfg(test)]
mod tests {
use super::*;

#[derive(Eq, PartialEq)]
struct Cs1;
static CS1: Cs1 = Cs1;
static REG1: Registration = Registration::new(&CS1);

impl Callsite for Cs1 {
fn set_interest(&self, _interest: Interest) {}
fn metadata(&self) -> &Metadata<'_> {
todo!()
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved
}

fn registration(&'static self) -> &'static Registration {
&REG1
}
}

struct Cs2;
static CS2: Cs2 = Cs2;
static REG2: Registration = Registration::new(&CS2);

impl Callsite for Cs2 {
fn set_interest(&self, _interest: Interest) {}
fn metadata(&self) -> &Metadata<'_> {
todo!()
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved
}

fn registration(&'static self) -> &'static Registration {
&REG2
}
}

#[test]
fn linked_list_push() {
let linked_list = LinkedList::new();

linked_list.push(&CS1);
linked_list.push(&CS2);

let mut i = 0;

linked_list.for_each(|cs| {
if i == 0 {
assert!(
ptr::eq(cs.registration(), &REG2),
"Registration pointers need to match REG2"
);
} else {
assert!(
ptr::eq(cs.registration(), &REG1),
"Registration pointers need to match REG1"
);
}

i += 1;
});
}

#[test]
#[should_panic]
fn linked_list_repeated() {
let linked_list = LinkedList::new();

linked_list.push(&CS1);
linked_list.push(&CS1);

linked_list.for_each(|_| {});
}

#[test]
fn linked_list_empty() {
let linked_list = LinkedList::new();

linked_list.for_each(|_| {
assert!(false, "List should be empty");
});
}
}
7 changes: 6 additions & 1 deletion tracing-core/src/dispatcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -783,11 +783,12 @@ impl Drop for DefaultGuard {

#[cfg(test)]
mod test {

use super::*;
#[cfg(feature = "std")]
use crate::stdlib::sync::atomic::{AtomicUsize, Ordering};
use crate::{
callsite::Callsite,
callsite::{Callsite, Registration},
metadata::{Kind, Level, Metadata},
subscriber::Interest,
};
Expand Down Expand Up @@ -820,6 +821,10 @@ mod test {
fn metadata(&self) -> &Metadata<'_> {
&TEST_META
}

fn registration(&'static self) -> &'static Registration {
todo!()
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved
}
}

#[test]
Expand Down
12 changes: 12 additions & 0 deletions tracing-core/src/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,10 @@ impl fmt::Debug for dyn Value {
fn metadata(&self) -> &crate::Metadata<'_> {
unreachable!("you somehow managed to access the null callsite?")
}

fn registration(&'static self) -> &'static crate::callsite::Registration {
unreachable!()
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved
}
}

static FIELD: Field = Field {
Expand Down Expand Up @@ -854,6 +858,10 @@ mod test {
fn metadata(&self) -> &Metadata<'_> {
&TEST_META_1
}

fn registration(&'static self) -> &'static crate::callsite::Registration {
unimplemented!()
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved
}
}

struct TestCallsite2;
Expand All @@ -875,6 +883,10 @@ mod test {
fn metadata(&self) -> &Metadata<'_> {
&TEST_META_2
}

fn registration(&'static self) -> &'static crate::callsite::Registration {
unimplemented!()
}
}

#[test]
Expand Down
6 changes: 4 additions & 2 deletions tracing-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,15 @@ extern crate alloc;
/// # #[macro_use]
/// # extern crate tracing_core;
/// use tracing_core::callsite;
/// # use tracing_core::{Metadata, subscriber::Interest};
/// # use tracing_core::{Metadata, subscriber::Interest, callsite::Registration};
/// # fn main() {
/// pub struct MyCallsite {
/// // ...
/// }
/// impl callsite::Callsite for MyCallsite {
/// # fn set_interest(&self, _: Interest) { unimplemented!() }
/// # fn metadata(&self) -> &Metadata { unimplemented!() }
/// # fn registration(&'static self) -> &'static Registration { unimplemented!() }
/// // ...
/// }
///
Expand Down Expand Up @@ -162,13 +163,14 @@ macro_rules! identify_callsite {
/// ```rust
/// # #[macro_use]
/// # extern crate tracing_core;
/// # use tracing_core::{callsite::Callsite, subscriber::Interest};
/// # use tracing_core::{callsite::{Callsite, Registration}, subscriber::Interest};
/// use tracing_core::metadata::{Kind, Level, Metadata};
/// # fn main() {
/// # pub struct MyCallsite { }
/// # impl Callsite for MyCallsite {
/// # fn set_interest(&self, _: Interest) { unimplemented!() }
/// # fn metadata(&self) -> &Metadata { unimplemented!() }
/// # fn registration(&'static self) -> &'static Registration { unimplemented!() }
/// # }
/// #
/// static FOO_CALLSITE: MyCallsite = MyCallsite {
Expand Down
Loading