core: Add intrusive linkedlist for callsite registry #988

LucioFranco · 2020-09-25T20:02:56Z

This change adds an intrusive LinkedList for the
callsite registry. The linked list is lock-free and
can be written to from multiple threads. This list
also does not require any allocations due to its
intrusive nature. This though does require a
breaking change to the Callsite trait to allow
callsites to store the pointer to the next item in
the intrusive list.

Properoties of the intrusive atomically linked-list:

Only supports LinkedList::push and LinkedList::for_each.
The items in the list can only be added and not removed.

Closes #860

This change adds an intrusive `LinkedList` for the callsite registry. The linked list is lock-free and can be written to from multiple threads. This list also does not require any allocations due to its intrusive nature. This though does require a breaking change to the `Callsite` trait to allow callsites to store the pointer to the next item in the intrusive list. Properoties of the intrusive atomically linked-list: - Only supports `LinkedList::push` and `LinkedList::for_each`. - The items in the list can only be added and not removed. Closes #860

tracing-core/src/callsite.rs

tracing-core/src/field.rs

tracing-core/src/callsite.rs

Co-authored-by: Eliza Weisman <[email protected]>

tracing-core/src/callsite.rs

tracing-core/src/dispatcher.rs

tracing-core/src/field.rs

Co-authored-by: Eliza Weisman <[email protected]>

tracing-core/src/callsite.rs

hawkw

lgtm, although I commented on a couple style, formatting, and docs nits you may want to address.

it would be nice to benchmark this with RwLock/no locking around the dispatcher list, but that's a medium-sized change to dispatcher internals, so let's not block on this for now. even if this doesn't have a huge impact on registration performance yet without also getting rid of the mutex, this should result in a pretty big reduction in heap usage, since we no longer need to store a pointer to every callsite on the heap — i think it's worth merging even without the changes on the dispatch side. let's do those later?

tracing-core/src/callsite.rs

hawkw · 2020-09-30T16:31:58Z

tracing-core/src/callsite.rs

+}
+
+impl LinkedList {
+    fn new() -> Self {


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...

we can always add const fn later though.

tracing-core/src/callsite.rs

hawkw · 2020-09-30T16:38:14Z

tracing-core/src/callsite.rs

+                Ordering::AcqRel,
+                Ordering::Acquire,


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.

tracing-core/src/callsite.rs

* upstream/master: subscriber: warn if trying to enable a statically disabled level (tokio-rs#990) subscriber: use macros for module declarations (tokio-rs#1009) chore: remove `stdlib.rs` (tokio-rs#1008) core: fix linked list tests reusing `Registration`s (tokio-rs#1016) subscriber: support dash in target names (tokio-rs#1012) docs: switch to intra-doc links in tracing-core (tokio-rs#1010) tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007) core: add intrusive linked list for callsite registry (tokio-rs#988) serde: allow tracing-serde to work on no_std. (tokio-rs#960) tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003) tracing: make `Entered` `!Send` (tokio-rs#1001) chore: fix nightly clippy warnings (tokio-rs#991) chore: bump all crate versions (tokio-rs#998) macros: fix the `tracing-macros` crate not compiling (tokio-rs#1000) tracing: prepare to release 0.1.21 (tokio-rs#997) core: prepare to release 0.1.17 (tokio-rs#996) subscriber: make `PartialOrd` & `Ord` impls more correct (tokio-rs#995) core, tracing: don't inline dispatcher::get_default (tokio-rs#994) core: make `Level` and `LevelFilter` `Copy` (tokio-rs#992)

…spatch-as-ref-tokio-rs#455 * upstream/master: (28 commits) opentelemetry: prepare for 0.8.0 release (tokio-rs#1036) docs: add favicon for extra pretty docs (tokio-rs#1033) subscriber: fix `reload` ergonomics (tokio-rs#1035) chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031) opentelemetry: Assign default ids if missing (tokio-rs#1027) chore: remove deprecated add-path from CI (tokio-rs#1026) attributes: fix `#[instrument(err)]` in case of early returns (tokio-rs#1006) core: remove mandatory liballoc dependency with no-std (tokio-rs#1017) chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023) subscriber: warn if trying to enable a statically disabled level (tokio-rs#990) subscriber: use macros for module declarations (tokio-rs#1009) chore: remove `stdlib.rs` (tokio-rs#1008) core: fix linked list tests reusing `Registration`s (tokio-rs#1016) subscriber: support dash in target names (tokio-rs#1012) docs: switch to intra-doc links in tracing-core (tokio-rs#1010) tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007) core: add intrusive linked list for callsite registry (tokio-rs#988) serde: allow tracing-serde to work on no_std. (tokio-rs#960) tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003) tracing: make `Entered` `!Send` (tokio-rs#1001) ...

…cros * upstream/master: (30 commits) chore(deps): update pin-project requirement from 0.4 to 1.0 (tokio-rs#1038) chore: remove duplicated section from tracing/README.md (tokio-rs#1046) opentelemetry: prepare for 0.8.0 release (tokio-rs#1036) docs: add favicon for extra pretty docs (tokio-rs#1033) subscriber: fix `reload` ergonomics (tokio-rs#1035) chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031) opentelemetry: Assign default ids if missing (tokio-rs#1027) chore: remove deprecated add-path from CI (tokio-rs#1026) attributes: fix `#[instrument(err)]` in case of early returns (tokio-rs#1006) core: remove mandatory liballoc dependency with no-std (tokio-rs#1017) chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023) subscriber: warn if trying to enable a statically disabled level (tokio-rs#990) subscriber: use macros for module declarations (tokio-rs#1009) chore: remove `stdlib.rs` (tokio-rs#1008) core: fix linked list tests reusing `Registration`s (tokio-rs#1016) subscriber: support dash in target names (tokio-rs#1012) docs: switch to intra-doc links in tracing-core (tokio-rs#1010) tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007) core: add intrusive linked list for callsite registry (tokio-rs#988) serde: allow tracing-serde to work on no_std. (tokio-rs#960) ...

…gger * upstream/master: (31 commits) chore: fix tracing-macros::dbg (tokio-rs#1054) chore(deps): update pin-project requirement from 0.4 to 1.0 (tokio-rs#1038) chore: remove duplicated section from tracing/README.md (tokio-rs#1046) opentelemetry: prepare for 0.8.0 release (tokio-rs#1036) docs: add favicon for extra pretty docs (tokio-rs#1033) subscriber: fix `reload` ergonomics (tokio-rs#1035) chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031) opentelemetry: Assign default ids if missing (tokio-rs#1027) chore: remove deprecated add-path from CI (tokio-rs#1026) attributes: fix `#[instrument(err)]` in case of early returns (tokio-rs#1006) core: remove mandatory liballoc dependency with no-std (tokio-rs#1017) chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023) subscriber: warn if trying to enable a statically disabled level (tokio-rs#990) subscriber: use macros for module declarations (tokio-rs#1009) chore: remove `stdlib.rs` (tokio-rs#1008) core: fix linked list tests reusing `Registration`s (tokio-rs#1016) subscriber: support dash in target names (tokio-rs#1012) docs: switch to intra-doc links in tracing-core (tokio-rs#1010) tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007) core: add intrusive linked list for callsite registry (tokio-rs#988) ...

…1.x (#2083) ## Motivation Currently on `v0.1.x`, the global callsite registry is implemented as a `Mutex<Vec<&'static dyn Callsite>>`. This has a few downsides: * Every time a callsite is registered, the `Mutex` must be locked. This can cause a deadlock when a `register_callsite` implementation calls into code that _also_ registers a callsite. See #2020 for details. * Registering callsites is relatively slow and forces the program to synchronize on the callsite registry (and not on *individual* callsites). This means that if two threads are both registering totally different callsites, both threads must wait for the lock. Although this overhead is amortized over the entire rest of the program, it does have an impact in short-running applications where any given callsite may only be hit once or twice. * Memory allocation may occur while the registry is locked. This makes the use of `tracing` inside of memory allocators difficult or impossible. On the `master` branch (v0.2.x), PR #988 rewrote the callsite registry to use an intrusive atomic singly-linked list of `Callsite`s. This removed the need for locking the callsite registry, and made it possible to register callsites without ever allocating memory. Because the callsite registry on v0.2 will *never* allocate, this also makes it possible to compile `tracing-core` for `no_std` platforms without requiring `liballoc`. Unfortunately, however, we cannot use an identical implementation on v0.1.x, because using the intrusive linked-list registry for *all* callsites requires a breaking change to the `Callsite` trait (in order to add a way to get the callsite's linked-list node), which we cannot make on v0.1.x. ## Solution This branch adds a linked-list callsite registry for v0.1.x in a way that allows us to benefit from *some* of the advantages of this approach in a majority of cases. The trick is introducing a new `DefaultCallsite` type in `tracing-core` that implements the `Callsite` trait. This type can contain an intrusive list node, and *when a callsite is a `DefaultCallsite`*, we can register it without having to push it to the `Mutex<Vec<...>>`. The locked vec still _exists_, for `Callsite`s that are *not* `DefaultCallsite`s, so we can't remove the `liballoc` dependency, but in most cases, we can avoid the mutex and allocation. Naturally, `tracing` has been updated to use the `DefaultCallsite` type from `tracing-core`, so the `Vec` will only be used in the following cases: * User code has a custom implementation of the `Callsite` trait, which is [not terribly common][1]. * An older version of the `tracing` crate is in use. This fixes the deadlock described in #2020 when `DefaultCallsite`s are used. Additionally, it should reduce the performance impact and memory usage of the callsite registry. Furthermore, I've changed the subscriber registry so that a `RwLock<Vec<dyn Dispatch>>` is only used when there actually are multiple subscribers in use. When there's only a global default subscriber, we can once again avoid locking for the registry of subscribers. When the `std` feature is disabled, thread-local scoped dispatchers are unavailable, so the single global subscriber will always be used on `no_std`. We can make additional changes, such as the ones from #2020, to _also_ resolve potential deadlocks when non-default callsites are in use, but since this branch rewrites a lot of the callsite registry code, that should probably be done in a follow-up. [1]: https://cs.github.com/?scopeName=All+repos&scope=&q=%28%2Fimpl+.*Callsite%2F+AND+language%3Arust%29+NOT+%28path%3A%2Ftracing%2F**+OR+path%3A%2Ftracing-*%2F**+OR+path%3A%2Ftokio-trace*%2F**%29%29 Signed-off-by: Eliza Weisman <[email protected]>

…1.x (tokio-rs#2083) ## Motivation Currently on `v0.1.x`, the global callsite registry is implemented as a `Mutex<Vec<&'static dyn Callsite>>`. This has a few downsides: * Every time a callsite is registered, the `Mutex` must be locked. This can cause a deadlock when a `register_callsite` implementation calls into code that _also_ registers a callsite. See tokio-rs#2020 for details. * Registering callsites is relatively slow and forces the program to synchronize on the callsite registry (and not on *individual* callsites). This means that if two threads are both registering totally different callsites, both threads must wait for the lock. Although this overhead is amortized over the entire rest of the program, it does have an impact in short-running applications where any given callsite may only be hit once or twice. * Memory allocation may occur while the registry is locked. This makes the use of `tracing` inside of memory allocators difficult or impossible. On the `master` branch (v0.2.x), PR tokio-rs#988 rewrote the callsite registry to use an intrusive atomic singly-linked list of `Callsite`s. This removed the need for locking the callsite registry, and made it possible to register callsites without ever allocating memory. Because the callsite registry on v0.2 will *never* allocate, this also makes it possible to compile `tracing-core` for `no_std` platforms without requiring `liballoc`. Unfortunately, however, we cannot use an identical implementation on v0.1.x, because using the intrusive linked-list registry for *all* callsites requires a breaking change to the `Callsite` trait (in order to add a way to get the callsite's linked-list node), which we cannot make on v0.1.x. ## Solution This branch adds a linked-list callsite registry for v0.1.x in a way that allows us to benefit from *some* of the advantages of this approach in a majority of cases. The trick is introducing a new `DefaultCallsite` type in `tracing-core` that implements the `Callsite` trait. This type can contain an intrusive list node, and *when a callsite is a `DefaultCallsite`*, we can register it without having to push it to the `Mutex<Vec<...>>`. The locked vec still _exists_, for `Callsite`s that are *not* `DefaultCallsite`s, so we can't remove the `liballoc` dependency, but in most cases, we can avoid the mutex and allocation. Naturally, `tracing` has been updated to use the `DefaultCallsite` type from `tracing-core`, so the `Vec` will only be used in the following cases: * User code has a custom implementation of the `Callsite` trait, which is [not terribly common][1]. * An older version of the `tracing` crate is in use. This fixes the deadlock described in tokio-rs#2020 when `DefaultCallsite`s are used. Additionally, it should reduce the performance impact and memory usage of the callsite registry. Furthermore, I've changed the subscriber registry so that a `RwLock<Vec<dyn Dispatch>>` is only used when there actually are multiple subscribers in use. When there's only a global default subscriber, we can once again avoid locking for the registry of subscribers. When the `std` feature is disabled, thread-local scoped dispatchers are unavailable, so the single global subscriber will always be used on `no_std`. We can make additional changes, such as the ones from tokio-rs#2020, to _also_ resolve potential deadlocks when non-default callsites are in use, but since this branch rewrites a lot of the callsite registry code, that should probably be done in a follow-up. [1]: https://cs.github.com/?scopeName=All+repos&scope=&q=%28%2Fimpl+.*Callsite%2F+AND+language%3Arust%29+NOT+%28path%3A%2Ftracing%2F**+OR+path%3A%2Ftracing-*%2F**+OR+path%3A%2Ftokio-trace*%2F**%29%29 Signed-off-by: Eliza Weisman <[email protected]>

LucioFranco requested a review from hawkw September 25, 2020 20:02

LucioFranco requested a review from a team as a code owner September 25, 2020 20:02

hawkw reviewed Sep 25, 2020

View reviewed changes

LucioFranco added 2 commits September 28, 2020 15:28

Add registration contrusctor and address comments

486103b

Address clippy warnings

2638e89

LucioFranco requested a review from hawkw September 28, 2020 15:29

hawkw reviewed Sep 28, 2020

View reviewed changes

Apply suggestions from code review

c28c714

Co-authored-by: Eliza Weisman <[email protected]>

hawkw reviewed Sep 28, 2020

View reviewed changes

tracing-core/src/callsite.rs Outdated Show resolved Hide resolved

hawkw reviewed Sep 28, 2020

View reviewed changes

tracing-core/src/callsite.rs Outdated Show resolved Hide resolved

tracing-core/src/dispatcher.rs Outdated Show resolved Hide resolved

tracing-core/src/field.rs Outdated Show resolved Hide resolved

LucioFranco and others added 2 commits September 28, 2020 12:00

Apply suggestions from code review

699e526

Co-authored-by: Eliza Weisman <[email protected]>

Move assert and clean up locking

d7ae64b

LucioFranco requested a review from hawkw September 28, 2020 16:28

hawkw reviewed Sep 29, 2020

View reviewed changes

tracing-core/src/callsite.rs Outdated Show resolved Hide resolved

Remove Callsite::registration

57c19ee

LucioFranco requested a review from hawkw September 29, 2020 20:50

hawkw approved these changes Sep 30, 2020

View reviewed changes

hawkw added this to the tracing-core 0.2 milestone Sep 30, 2020

Apply suggestions from code review

93483e6

hawkw reviewed Oct 1, 2020

View reviewed changes

tracing-core/src/callsite.rs Outdated Show resolved Hide resolved

Update tracing-core/src/callsite.rs

3fd30ad

hawkw merged commit 0fcc9fd into master Oct 2, 2020

hawkw mentioned this pull request Oct 5, 2020

Remove stdlib.rs #1008

Merged

hawkw mentioned this pull request Apr 19, 2022

core: add a limited form of the linked-list callsite registry for v0.1.x #2083

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Add intrusive linkedlist for callsite registry #988

core: Add intrusive linkedlist for callsite registry #988

LucioFranco commented Sep 25, 2020

hawkw left a comment

hawkw Sep 30, 2020

hawkw Sep 30, 2020

hawkw Sep 30, 2020

core: Add intrusive linkedlist for callsite registry #988

core: Add intrusive linkedlist for callsite registry #988

Conversation

LucioFranco commented Sep 25, 2020

hawkw left a comment

Choose a reason for hiding this comment

hawkw Sep 30, 2020

Choose a reason for hiding this comment

hawkw Sep 30, 2020

Choose a reason for hiding this comment

hawkw Sep 30, 2020

Choose a reason for hiding this comment