WIP feedback

Author: ErichDonGubler

lock-analyzer/src/main.rs

@@ -13,13 +13,17 @@
 //! See `wgpu_core/src/lock/observing.rs` for a general explanation of
 //! this analysis.
 
+#[path = "../../wgpu-core/src/lock/observing/action.rs"]
+mod action;
+
 use std::sync::Arc;
 use std::{
     collections::{btree_map::Entry, BTreeMap, BTreeSet, HashMap},
     fmt,
     path::PathBuf,
 };
 
+use action::Action;
 use anyhow::{Context, Result};
 
 fn main() -> Result<()> {
@@ -56,7 +60,7 @@ fn main() -> Result<()> {
             if line.is_empty() {
                 continue;
             }
-            let action = ron::de::from_bytes::<Action>(line)
+            let action = ron::de::from_bytes::<Action<LocationAddress>>(line)
                 .with_context(|| format!("Error parsing action from {}", name.display()))?;
             match action {
                 Action::Location {
@@ -67,7 +71,7 @@ fn main() -> Result<()> {
                 } => {
                     let file = match file.split_once("src/") {
                         Some((_, after)) => after.to_string(),
-                        None => file,
+                        None => file.into_owned(),
                     };
                     assert!(locations
                         .insert(address, Arc::new(Location { file, line, column }))
@@ -85,8 +89,8 @@ fn main() -> Result<()> {
                     }
                     Entry::Vacant(vacant) => {
                         vacant.insert(Rank {
-                            member_name,
-                            const_name,
+                            member_name: member_name.into_owned(),
+                            const_name: const_name.into_owned(),
                             acquisitions: BTreeMap::default(),
                         });
                     }
@@ -181,42 +185,6 @@ fn main() -> Result<()> {
     Ok(())
 }
 
-#[derive(Debug, serde::Deserialize)]
-#[serde(deny_unknown_fields)]
-enum Action {
-    /// A location that we will refer to in later actions.
-    Location {
-        address: LocationAddress,
-        file: String,
-        line: u32,
-        column: u32,
-    },
-
-    /// A lock rank that we will refer to in later actions.
-    Rank {
-        bit: u32,
-        member_name: String,
-        const_name: String,
-    },
-
-    /// An attempt to acquire a lock while holding another lock.
-    Acquisition {
-        /// The number of the already acquired lock's rank.
-        older_rank: u32,
-
-        /// The source position at which we acquired it. Specifically,
-        /// its `Location`'s address, as an integer.
-        older_location: LocationAddress,
-
-        /// The number of the rank of the lock we are acquiring.
-        newer_rank: u32,
-
-        /// The source position at which we are acquiring it.
-        /// Specifically, its `Location`'s address, as an integer.
-        newer_location: LocationAddress,
-    },
-}
-
 /// The memory address at which the `Location` was stored in the
 /// observed process.
 ///

wgpu-core/src/lock/observing.rs

@@ -39,9 +39,12 @@ use std::{
     vec::Vec,
 };
 
+use self::action::Action;
 use super::rank::{LockRank, LockRankSet};
 use crate::FastHashSet;
 
+mod action;
+
 /// A `Mutex` instrumented for lock acquisition order observation.
 ///
 /// This is just a wrapper around a [`parking_lot::Mutex`], along with
@@ -383,22 +386,22 @@ impl ObservationLog {
         if self.locations_seen.insert(location) {
             self.write_action(&Action::Location {
                 address: addr(location),
-                file: location.file(),
-                line: location.line(),
-                column: location.column(),
+                file: location.file().into(),
+                line: location.line().into(),
+                column: location.column().into(),
             });
         }
     }
 
     fn write_rank(&mut self, rank: LockRankSet) {
         self.write_action(&Action::Rank {
             bit: rank.number(),
-            member_name: rank.member_name(),
-            const_name: rank.const_name(),
+            member_name: rank.member_name().into(),
+            const_name: rank.const_name().into(),
         });
     }
 
-    fn write_action(&mut self, action: &Action) {
+    fn write_action(&mut self, action: &Action<usize>) {
         use std::io::Write;
 
         self.buffer.clear();
@@ -411,66 +414,6 @@ impl ObservationLog {
     }
 }
 
-/// An action logged by a thread that is observing lock acquisition order.
-///
-/// Each thread's log file is a sequence of these enums, serialized
-/// using the [`ron`] crate, one action per line.
-///
-/// Lock observation cannot assume that there will be any convenient
-/// finalization point before the program exits, so in practice,
-/// actions must be written immediately when they occur. This means we
-/// can't, say, accumulate tables and write them out when they're
-/// complete. The `lock-analyzer` binary is then responsible for
-/// consolidating the data into a single table of observed transitions.
-#[derive(serde::Serialize)]
-enum Action {
-    /// A location that we will refer to in later actions.
-    ///
-    /// We write one of these events the first time we see a
-    /// particular `Location`. Treating this as a separate action
-    /// simply lets us avoid repeating the content over and over
-    /// again in every [`Acquisition`] action.
-    ///
-    /// [`Acquisition`]: Action::Acquisition
-    Location {
-        address: usize,
-        file: &'static str,
-        line: u32,
-        column: u32,
-    },
-
-    /// A lock rank that we will refer to in later actions.
-    ///
-    /// We write out one these events for every lock rank at the
-    /// beginning of each thread's log file. Treating this as a
-    /// separate action simply lets us avoid repeating the names over
-    /// and over again in every [`Acquisition`] action.
-    ///
-    /// [`Acquisition`]: Action::Acquisition
-    Rank {
-        bit: u32,
-        member_name: &'static str,
-        const_name: &'static str,
-    },
-
-    /// An attempt to acquire a lock while holding another lock.
-    Acquisition {
-        /// The number of the already acquired lock's rank.
-        older_rank: u32,
-
-        /// The source position at which we acquired it. Specifically,
-        /// its `Location`'s address, as an integer.
-        older_location: usize,
-
-        /// The number of the rank of the lock we are acquiring.
-        newer_rank: u32,
-
-        /// The source position at which we are acquiring it.
-        /// Specifically, its `Location`'s address, as an integer.
-        newer_location: usize,
-    },
-}
-
 impl LockRankSet {
     /// Return the number of this rank's first member.
     fn number(self) -> u32 {

wgpu-core/src/lock/observing/action.rs

@@ -0,0 +1,62 @@
+use std::borrow::Cow;
+
+/// An action logged by a thread that is observing lock acquisition order.
+///
+/// Each thread's log file is a sequence of these enums, serialized
+/// using the [`ron`] crate, one action per line.
+///
+/// Lock observation cannot assume that there will be any convenient
+/// finalization point before the program exits, so in practice,
+/// actions must be written immediately when they occur. This means we
+/// can't, say, accumulate tables and write them out when they're
+/// complete. The `lock-analyzer` binary is then responsible for
+/// consolidating the data into a single table of observed transitions.
+#[derive(Debug, serde::Deserialize, serde::Serialize)]
+#[serde(deny_unknown_fields)]
+pub(super) enum Action<LocationAddress> {
+    /// A location that we will refer to in later actions.
+    ///
+    /// We write one of these events the first time we see a
+    /// particular `Location`. Treating this as a separate action
+    /// simply lets us avoid repeating the content over and over
+    /// again in every [`Acquisition`] action.
+    ///
+    /// [`Acquisition`]: Action::Acquisition
+    Location {
+        address: LocationAddress,
+        file: Cow<'static, str>,
+        line: u32,
+        column: u32,
+    },
+
+    /// A lock rank that we will refer to in later actions.
+    ///
+    /// We write out one these events for every lock rank at the
+    /// beginning of each thread's log file. Treating this as a
+    /// separate action simply lets us avoid repeating the names over
+    /// and over again in every [`Acquisition`] action.
+    ///
+    /// [`Acquisition`]: Action::Acquisition
+    Rank {
+        bit: u32,
+        member_name: Cow<'static, str>,
+        const_name: Cow<'static, str>,
+    },
+
+    /// An attempt to acquire a lock while holding another lock.
+    Acquisition {
+        /// The number of the already acquired lock's rank.
+        older_rank: u32,
+
+        /// The source position at which we acquired it. Specifically,
+        /// its `Location`'s address, as an integer.
+        older_location: LocationAddress,
+
+        /// The number of the rank of the lock we are acquiring.
+        newer_rank: u32,
+
+        /// The source position at which we are acquiring it.
+        /// Specifically, its `Location`'s address, as an integer.
+        newer_location: LocationAddress,
+    },
+}