Merge #30

30: Implement OnceLock r=bertptrs a=bertptrs

1.70 brought `OnceLock`. This needs a new wrapper and should replace all usage of `lazy_static!`, as well as the current internal implementation of OnceLock.

Co-authored-by: Bert Peters <bert@bertptrs.nl>

.github/workflows/ci.yml

@@ -21,28 +21,48 @@ jobs:
           - nightly
 
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v2
 
-      - uses: dtolnay/rust-toolchain@v1
+      - uses: actions-rs/toolchain@v1
         with:
+          profile: minimal
           toolchain: ${{ matrix.rust }}
+          override: true
           components: rustfmt, clippy
 
-      - run: cargo build --all-features --all-targets
-      - run: cargo test --all-features
-      - run: cargo fmt --all -- --check
-      - run: cargo clippy --all-features --all-targets -- -D warnings
+      - uses: actions-rs/cargo@v1
+        with:
+          command: build
+          # --all-targets ensures that we also build the benchmarks and tests already.
+          args: --all-features --all-targets
+
+      - uses: actions-rs/cargo@v1
+        with:
+          command: test
+          args: --all-features
+
+      - uses: actions-rs/cargo@v1
+        with:
+          command: fmt
+          args: --all -- --check
+
+      - uses: actions-rs/cargo@v1
+        with:
+          command: clippy
+          args: --all-features --all-targets -- -D warnings
 
   docs:
     name: Documentation build
     runs-on: ubuntu-latest
 
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v2
 
-      - uses: dtolnay/rust-toolchain@v1
+      - uses: actions-rs/toolchain@v1
         with:
+          profile: minimal
           toolchain: nightly
+          override: true
 
       - name: Build documentation
         env:

CHANGELOG.md

@@ -6,26 +6,16 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
 
-## [0.3.0] - 2023-09-09
-
 ### Added
 
 - The minimum supported Rust version is now defined as 1.70. Previously it was undefined.
 - Wrappers for `std::sync` primitives can now be `const` constructed.
 - Add support for `std::sync::OnceLock`
-- Added backtraces of mutex allocations to the cycle report. Capturing backtraces does incur some
-  overhead, this can be mitigated by disabling the `backtraces` feature which is enabled by default.
 
 ### Breaking
 
 - Update [`parking_lot`][parking_lot] dependency to `0.12`.
-- Restructured the crate to reduce typename verbosity. Wrapper names now match the name of the
-  primitive they wrap. Specific always/debug tracing versions have now moved to separate modules.
-  For example, `tracing_mutex::stdsync::TracingMutex` is now
-  `tracing_mutex::stdsync::tracing::Mutex`, and `tracing_mutex::stdsync::DebugMutex` is now called
-  `tracing_mutex::stdsync::Mutex`. This hopefully reduces the visual noise while reading code that
-  uses this in practice. Unwrapped primitives are reexported under `tracing_mutex::stdsync::raw` for
-  convenience.
+- Restructured the crate to reduce typename verbosity. For details, see: #25.
 
 ### Fixed
 
@@ -93,8 +83,7 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 Initial release.
 
-[Unreleased]: https://github.com/bertptrs/tracing-mutex/compare/v0.3.0...HEAD
-[0.3.0]: https://github.com/bertptrs/tracing-mutex/compare/v0.2.1...v0.3.0
+[Unreleased]: https://github.com/bertptrs/tracing-mutex/compare/v0.2.1...HEAD
 [0.2.1]: https://github.com/bertptrs/tracing-mutex/compare/v0.2.0...v0.2.1
 [0.2.0]: https://github.com/bertptrs/tracing-mutex/compare/v0.1.2...v0.2.0
 [0.1.2]: https://github.com/bertptrs/tracing-mutex/compare/v0.1.1...v0.1.2

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "tracing-mutex"
-version = "0.3.0"
+version = "0.2.1"
 authors = ["Bert Peters <bert@bertptrs.nl>"]
 edition = "2021"
 license = "MIT OR Apache-2.0"
@@ -23,7 +23,7 @@ lock_api = { version = "0.4", optional = true }
 parking_lot = { version = "0.12", optional = true }
 
 [dev-dependencies]
-criterion = "0.5"
+criterion = "0.3"
 rand = "0.8"
 
 [[bench]]
@@ -31,8 +31,6 @@ name = "mutex"
 harness = false
 
 [features]
-default = ["backtraces"]
-backtraces = []
 # Feature names do not match crate names pending namespaced features.
 lockapi = ["lock_api"]
 parkinglot = ["parking_lot", "lockapi"]

examples/mutex_cycle.rs

@@ -1,26 +0,0 @@
-//! Show what a crash looks like
-//!
-//! This shows what a traceback of a cycle detection looks like. It is expected to crash.
-use tracing_mutex::stdsync::Mutex;
-
-fn main() {
-    let a = Mutex::new(());
-    let b = Mutex::new(());
-    let c = Mutex::new(());
-
-    // Create an edge from a to b
-    {
-        let _a = a.lock();
-        let _b = b.lock();
-    }
-
-    // Create an edge from b to c
-    {
-        let _b = b.lock();
-        let _c = c.lock();
-    }
-
-    // Now crash by trying to add an edge from c to a
-    let _c = c.lock();
-    let _a = a.lock(); // This line will crash
-}

src/graph.rs

@@ -1,5 +1,4 @@
 use std::cell::Cell;
-use std::collections::hash_map::Entry;
 use std::collections::HashMap;
 use std::collections::HashSet;
 use std::hash::Hash;
@@ -20,24 +19,23 @@ type Order = usize;
 /// visibly changed.
 ///
 /// [paper]: https://whileydave.com/publications/pk07_jea/
-#[derive(Debug)]
-pub struct DiGraph<V, E>
+#[derive(Default, Debug)]
+pub struct DiGraph<V>
 where
     V: Eq + Hash + Copy,
 {
-    nodes: HashMap<V, Node<V, E>>,
-    // Instead of reordering the orders in the graph whenever a node is deleted, we maintain a list
-    // of unused ids that can be handed out later again.
-    unused_order: Vec<Order>,
+    nodes: HashMap<V, Node<V>>,
+    /// Next topological sort order
+    next_ord: Order,
 }
 
 #[derive(Debug)]
-struct Node<V, E>
+struct Node<V>
 where
     V: Eq + Hash + Clone,
 {
     in_edges: HashSet<V>,
-    out_edges: HashMap<V, E>,
+    out_edges: HashSet<V>,
     // The "Ord" field is a Cell to ensure we can update it in an immutable context.
     // `std::collections::HashMap` doesn't let you have multiple mutable references to elements, but
     // this way we can use immutable references and still update `ord`. This saves quite a few
@@ -45,7 +43,7 @@ where
     ord: Cell<Order>,
 }
 
-impl<V, E> DiGraph<V, E>
+impl<V> DiGraph<V>
 where
     V: Eq + Hash + Copy,
 {
@@ -56,18 +54,12 @@ where
     /// the node in the topological order.
     ///
     /// New nodes are appended to the end of the topological order when added.
-    fn add_node(&mut self, n: V) -> (&mut HashSet<V>, &mut HashMap<V, E>, Order) {
-        // need to compute next id before the call to entry() to avoid duplicate borrow of nodes
-        let fallback_id = self.nodes.len();
+    fn add_node(&mut self, n: V) -> (&mut HashSet<V>, &mut HashSet<V>, Order) {
+        let next_ord = &mut self.next_ord;
 
         let node = self.nodes.entry(n).or_insert_with(|| {
-            let order = if let Some(id) = self.unused_order.pop() {
-                // Reuse discarded ordering entry
-                id
-            } else {
-                // Allocate new order id
-                fallback_id
-            };
+            let order = *next_ord;
+            *next_ord = next_ord.checked_add(1).expect("Topological order overflow");
 
             Node {
                 ord: Cell::new(order),
@@ -85,12 +77,9 @@ where
             Some(Node {
                 out_edges,
                 in_edges,
-                ord,
+                ..
             }) => {
-                // Return ordering to the pool of unused ones
-                self.unused_order.push(ord.get());
-
-                out_edges.into_keys().for_each(|m| {
+                out_edges.into_iter().for_each(|m| {
                     self.nodes.get_mut(&m).unwrap().in_edges.remove(&n);
                 });
 
@@ -107,29 +96,18 @@ where
     ///
     /// Nodes, both from and to, are created as needed when creating new edges. If the new edge
     /// would introduce a cycle, the edge is rejected and `false` is returned.
-    ///
-    /// # Errors
-    ///
-    /// If the edge would introduce the cycle, the underlying graph is not modified and a list of
-    /// all the edge data in the would-be cycle is returned instead.
-    pub(crate) fn add_edge(&mut self, x: V, y: V, e: impl FnOnce() -> E) -> Result<(), Vec<E>>
-    where
-        E: Clone,
-    {
+    pub(crate) fn add_edge(&mut self, x: V, y: V) -> bool {
         if x == y {
             // self-edges are always considered cycles
-            return Err(Vec::new());
+            return false;
         }
 
         let (_, out_edges, ub) = self.add_node(x);
 
-        match out_edges.entry(y) {
-            Entry::Occupied(_) => {
+        if !out_edges.insert(y) {
             // Edge already exists, nothing to be done
-                return Ok(());
+            return true;
         }
-            Entry::Vacant(entry) => entry.insert(e()),
-        };
 
         let (in_edges, _, lb) = self.add_node(y);
 
@@ -141,7 +119,7 @@ where
             let mut delta_f = Vec::new();
             let mut delta_b = Vec::new();
 
-            if let Err(cycle) = self.dfs_f(&self.nodes[&y], ub, &mut visited, &mut delta_f) {
+            if !self.dfs_f(&self.nodes[&y], ub, &mut visited, &mut delta_f) {
                 // This edge introduces a cycle, so we want to reject it and remove it from the
                 // graph again to keep the "does not contain cycles" invariant.
 
@@ -151,7 +129,7 @@ where
                 self.nodes.get_mut(&x).map(|node| node.out_edges.remove(&y));
 
                 // No edge was added
-                return Err(cycle);
+                return false;
             }
 
             // No need to check as we should've found the cycle on the forward pass
@@ -163,49 +141,44 @@ where
             self.reorder(delta_f, delta_b);
         }
 
-        Ok(())
+        true
     }
 
     /// Forwards depth-first-search
     fn dfs_f<'a>(
         &'a self,
-        n: &'a Node<V, E>,
+        n: &'a Node<V>,
         ub: Order,
         visited: &mut HashSet<V>,
-        delta_f: &mut Vec<&'a Node<V, E>>,
-    ) -> Result<(), Vec<E>>
-    where
-        E: Clone,
-    {
+        delta_f: &mut Vec<&'a Node<V>>,
+    ) -> bool {
         delta_f.push(n);
 
-        for (w, e) in &n.out_edges {
+        n.out_edges.iter().all(|w| {
             let node = &self.nodes[w];
             let ord = node.ord.get();
 
             if ord == ub {
                 // Found a cycle
-                return Err(vec![e.clone()]);
+                false
             } else if !visited.contains(w) && ord < ub {
                 // Need to check recursively
                 visited.insert(*w);
-                if let Err(mut chain) = self.dfs_f(node, ub, visited, delta_f) {
-                    chain.push(e.clone());
-                    return Err(chain);
+                self.dfs_f(node, ub, visited, delta_f)
+            } else {
+                // Already seen this one or not interesting
+                true
             }
-            }
-        }
-
-        Ok(())
+        })
     }
 
     /// Backwards depth-first-search
     fn dfs_b<'a>(
         &'a self,
-        n: &'a Node<V, E>,
+        n: &'a Node<V>,
         lb: Order,
         visited: &mut HashSet<V>,
-        delta_b: &mut Vec<&'a Node<V, E>>,
+        delta_b: &mut Vec<&'a Node<V>>,
     ) {
         delta_b.push(n);
 
@@ -219,7 +192,7 @@ where
         }
     }
 
-    fn reorder(&self, mut delta_f: Vec<&Node<V, E>>, mut delta_b: Vec<&Node<V, E>>) {
+    fn reorder(&self, mut delta_f: Vec<&Node<V>>, mut delta_b: Vec<&Node<V>>) {
         self.sort(&mut delta_f);
         self.sort(&mut delta_b);
 
@@ -240,25 +213,12 @@ where
         }
     }
 
-    fn sort(&self, ids: &mut [&Node<V, E>]) {
+    fn sort(&self, ids: &mut [&Node<V>]) {
         // Can use unstable sort because mutex ids should not be equal
         ids.sort_unstable_by_key(|v| &v.ord);
     }
 }
 
-// Manual `Default` impl as derive causes unnecessarily strong bounds.
-impl<V, E> Default for DiGraph<V, E>
-where
-    V: Eq + Hash + Copy,
-{
-    fn default() -> Self {
-        Self {
-            nodes: Default::default(),
-            unused_order: Default::default(),
-        }
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use rand::seq::SliceRandom;
@@ -266,14 +226,12 @@ mod tests {
 
     use super::*;
 
-    fn nop() {}
-
     #[test]
     fn test_no_self_cycle() {
         // Regression test for https://github.com/bertptrs/tracing-mutex/issues/7
         let mut graph = DiGraph::default();
 
-        assert!(graph.add_edge(1, 1, nop).is_err());
+        assert!(!graph.add_edge(1, 1));
     }
 
     #[test]
@@ -281,16 +239,16 @@ mod tests {
         let mut graph = DiGraph::default();
 
         // Add some safe edges
-        assert!(graph.add_edge(0, 1, nop).is_ok());
-        assert!(graph.add_edge(1, 2, nop).is_ok());
-        assert!(graph.add_edge(2, 3, nop).is_ok());
-        assert!(graph.add_edge(4, 2, nop).is_ok());
+        assert!(graph.add_edge(0, 1));
+        assert!(graph.add_edge(1, 2));
+        assert!(graph.add_edge(2, 3));
+        assert!(graph.add_edge(4, 2));
 
         // Try to add an edge that introduces a cycle
-        assert!(graph.add_edge(3, 1, nop).is_err());
+        assert!(!graph.add_edge(3, 1));
 
         // Add an edge that should reorder 0 to be after 4
-        assert!(graph.add_edge(4, 0, nop).is_ok());
+        assert!(graph.add_edge(4, 0));
     }
 
     /// Fuzz the DiGraph implementation by adding a bunch of valid edges.
@@ -298,7 +256,7 @@ mod tests {
     /// This test generates all possible forward edges in a 100-node graph consisting of natural
     /// numbers, shuffles them, then adds them to the graph. This will always be a valid directed,
     /// acyclic graph because there is a trivial order (the natural numbers) but because the edges
-    /// are added in a random order the DiGraph will still occasionally need to reorder nodes.
+    /// are added in a random order the DiGraph will still occassionally need to reorder nodes.
     #[test]
     fn fuzz_digraph() {
         // Note: this fuzzer is quadratic in the number of nodes, so this cannot be too large or it
@@ -319,7 +277,7 @@ mod tests {
         let mut graph = DiGraph::default();
 
         for (x, y) in edges {
-            assert!(graph.add_edge(x, y, nop).is_ok());
+            assert!(graph.add_edge(x, y));
         }
     }
 }

src/lib.rs

@@ -18,23 +18,8 @@
 //! # Structure
 //!
 //! Each module in this crate exposes wrappers for a specific base-mutex with dependency trakcing
-//! added. This includes [`stdsync`] which provides wrappers for the base locks in the standard
-//! library, and more depending on enabled compile-time features. More back-ends may be added as
-//! features in the future.
-//!
-//! # Feature flags
-//!
-//! `tracing-mutex` uses feature flags to reduce the impact of this crate on both your compile time
-//! and runtime overhead. Below are the available flags. Modules are annotated with the features
-//! they require.
-//!
-//! - `backtraces`: Enables capturing backtraces of mutex dependencies, to make it easier to
-//!   determine what sequence of events would trigger a deadlock. This is enabled by default, but if
-//!   the performance overhead is unaccceptable, it can be disabled by disabling default features.
-//!
-//! - `lockapi`: Enables the wrapper lock for [`lock_api`][lock_api] locks
-//!
-//! - `parkinglot`: Enables wrapper types for [`parking_lot`][parking_lot] mutexes
+//! added. For now, that is limited to [`stdsync`] which provides wrappers for the base locks in the
+//! standard library. More back-ends may be added as features in the future.
 //!
 //! # Performance considerations
 //!
@@ -59,13 +44,7 @@
 //! (such as [`stdsync::Mutex`]) which evaluate to a tracing mutex when debug assertions are
 //! enabled, and to the underlying mutex when they're not.
 //!
-//! For ease of debugging, this crate will, by default, capture a backtrace when establishing a new
-//! dependency between two mutexes. This has an additional overhead of over 60%. If this additional
-//! debugging aid is not required, it can be disabled by disabling default features.
-//!
 //! [paper]: https://whileydave.com/publications/pk07_jea/
-//! [lock_api]: https://docs.rs/lock_api/0.4/lock_api/index.html
-//! [parking_lot]: https://docs.rs/parking_lot/0.12.1/parking_lot/
 #![cfg_attr(docsrs, feature(doc_cfg))]
 use std::cell::RefCell;
 use std::fmt;
@@ -85,8 +64,6 @@ pub use lock_api;
 #[cfg(feature = "parkinglot")]
 #[cfg_attr(docsrs, doc(cfg(feature = "parkinglot")))]
 pub use parking_lot;
-use reporting::Dep;
-use reporting::Reportable;
 
 use crate::graph::DiGraph;
 
@@ -97,7 +74,6 @@ pub mod lockapi;
 #[cfg(feature = "parkinglot")]
 #[cfg_attr(docsrs, doc(cfg(feature = "parkinglot")))]
 pub mod parkinglot;
-mod reporting;
 pub mod stdsync;
 
 thread_local! {
@@ -161,18 +137,19 @@ impl MutexId {
     ///
     /// This method panics if the new dependency would introduce a cycle.
     pub fn mark_held(&self) {
-        let opt_cycle = HELD_LOCKS.with(|locks| {
+        let creates_cycle = HELD_LOCKS.with(|locks| {
             if let Some(&previous) = locks.borrow().last() {
                 let mut graph = get_dependency_graph();
 
-                graph.add_edge(previous, self.value(), Dep::capture).err()
+                !graph.add_edge(previous, self.value())
             } else {
-                None
+                false
             }
         });
 
-        if let Some(cycle) = opt_cycle {
-            panic!("{}", Dep::panic_message(&cycle))
+        if creates_cycle {
+            // Panic without holding the lock to avoid needlessly poisoning it
+            panic!("Mutex order graph should not have cycles");
         }
 
         HELD_LOCKS.with(|locks| locks.borrow_mut().push(self.value()));
@@ -283,8 +260,8 @@ impl<'a> Drop for BorrowedMutex<'a> {
 }
 
 /// Get a reference to the current dependency graph
-fn get_dependency_graph() -> impl DerefMut<Target = DiGraph<usize, Dep>> {
-    static DEPENDENCY_GRAPH: OnceLock<Mutex<DiGraph<usize, Dep>>> = OnceLock::new();
+fn get_dependency_graph() -> impl DerefMut<Target = DiGraph<usize>> {
+    static DEPENDENCY_GRAPH: OnceLock<Mutex<DiGraph<usize>>> = OnceLock::new();
 
     DEPENDENCY_GRAPH
         .get_or_init(Default::default)
@@ -315,11 +292,11 @@ mod tests {
         let c = LazyMutexId::new();
 
         let mut graph = get_dependency_graph();
-        assert!(graph.add_edge(a.value(), b.value(), Dep::capture).is_ok());
-        assert!(graph.add_edge(b.value(), c.value(), Dep::capture).is_ok());
+        assert!(graph.add_edge(a.value(), b.value()));
+        assert!(graph.add_edge(b.value(), c.value()));
 
         // Creating an edge c → a should fail as it introduces a cycle.
-        assert!(graph.add_edge(c.value(), a.value(), Dep::capture).is_err());
+        assert!(!graph.add_edge(c.value(), a.value()));
 
         // Drop graph handle so we can drop vertices without deadlocking
         drop(graph);
@@ -327,9 +304,7 @@ mod tests {
         drop(b);
 
         // If b's destructor correctly ran correctly we can now add an edge from c to a.
-        assert!(get_dependency_graph()
-            .add_edge(c.value(), a.value(), Dep::capture)
-            .is_ok());
+        assert!(get_dependency_graph().add_edge(c.value(), a.value()));
     }
 
     /// Test creating a cycle, then panicking.

src/reporting.rs

@@ -1,64 +0,0 @@
-//! Cycle reporting primitives
-//!
-//! This module exposes [`Dep`], which resolves to either something that tracks dependencies or to
-//! something that doesn't.  It should only be assumed to implement the [`Reportable`] trait.
-use std::backtrace::Backtrace;
-use std::borrow::Cow;
-use std::fmt::Write;
-use std::sync::Arc;
-
-#[cfg(feature = "backtraces")]
-pub type Dep = MutexDep<Arc<Backtrace>>;
-#[cfg(not(feature = "backtraces"))]
-pub type Dep = MutexDep<()>;
-
-// Base message to be reported when cycle is detected
-const BASE_MESSAGE: &str = "Found cycle in mutex dependency graph:";
-
-pub trait Reportable: Clone {
-    /// Capture the current state
-    fn capture() -> Self;
-
-    /// Format a trace of state for human readable consumption.
-    fn panic_message(trace: &[Self]) -> Cow<'static, str>;
-}
-
-#[derive(Clone)]
-pub struct MutexDep<T>(T);
-
-/// Use a unit as tracing data: no tracing.
-///
-/// This should have no runtime overhead for capturing traces and should therefore be cheap enough
-/// for most purposes.
-impl Reportable for MutexDep<()> {
-    fn capture() -> Self {
-        Self(())
-    }
-
-    fn panic_message(_trace: &[Self]) -> Cow<'static, str> {
-        Cow::Borrowed(BASE_MESSAGE)
-    }
-}
-
-/// Use a full backtrace as tracing data
-///
-/// Capture the entire backtrace which may be expensive. This implementation does not force capture
-/// in the event that backtraces are disabled at runtime, so the exact overhead can still be
-/// controlled a little.
-///
-/// N.B. the [`Backtrace`] needs to be wrapped in an Arc as backtraces are not [`Clone`].
-impl Reportable for MutexDep<Arc<Backtrace>> {
-    fn capture() -> Self {
-        Self(Arc::new(Backtrace::capture()))
-    }
-
-    fn panic_message(trace: &[Self]) -> Cow<'static, str> {
-        let mut message = format!("{BASE_MESSAGE}\n");
-
-        for entry in trace {
-            let _ = writeln!(message, "{}", entry.0);
-        }
-
-        message.into()
-    }
-}

src/stdsync.rs

@@ -696,7 +696,7 @@ pub mod tracing {
         }
 
         #[test]
-        #[should_panic(expected = "Found cycle in mutex dependency graph")]
+        #[should_panic(expected = "Mutex order graph should not have cycles")]
         fn test_detect_cycle() {
             let a = Mutex::new(());
             let b = Mutex::new(());