WIPvariance

tools/analysis/traceevent/src/buffer.rs

@@ -1,5 +1,4 @@
 use core::{
-    cell::Cell,
     fmt::{Debug, Formatter},
     ops::{Deref, DerefMut as _},
     marker::PhantomData,
@@ -39,8 +38,8 @@ use crate::{
 // some event ids are very large. Since most traces will contain just a few
 // types of events, build up the smallest mapping as it goes.
 struct EventDescMap<T, InitDescF> {
-    // We use pointers instead of &'h here as EventDescMap is held in a Cell in EventVisitor, which
-    // would make it invariant in 'h, but we want to stay covariant.
+    // We use pointers instead of &'h here as EventDescMap is held behind an &'edm mut in
+    // EventVisitor, which would make it invariant in 'h, but we want to stay covariant.
     header: *const Header,
     cold_map: BTreeMap<EventId, *const EventDesc>,
     hot_map: BTreeMap<EventId, (*const EventDesc, T)>,
@@ -87,10 +86,8 @@ where
                 // TODO: Creating a &'h means we are creating an unbounded lifetime here, since 'h
                 // is not contained in the type. It could therefore differ between the call to
                 // new() and lookup(), so we fully rely on the caller doing something sensible.
-                // However, we removed 'h from the type in order to make Cell<EventDescMap<...>>
-                // covariant in 'h so we cannot just add it back. Maybe we need to make a Cell<Option<&'a T>>
-                // that is covariant in 'a and T so that we can then just put the 'h back in
-                // EventDescMap.
+                // However, we removed 'h from the type in order to make &'edm mut EventDescMap<...>>
+                // covariant in 'h so we cannot just add it back.
                 let val: &'h $ty = unsafe { &*ptr };
                 val
             }}
@@ -123,11 +120,10 @@ pub struct EventVisitor<'i, 'h, 'edm, InitDescF, T = ()> {
     pub timestamp: Timestamp,
     pub buffer_id: &'h BufferId,
 
-    // Using Cell<T> here means EventVisitor is invariant in any lifetime contained in T.  The
-    // reason for that Cell invariance is the same as why "&'a mut" is invariant in 'a. However,
-    // the only value we store back in the Cell is 'static (None) so that invariance is overkill.
-    // In order to stay invariant in 'h, we use *mut insteaf of &'edm mut.
-    desc_map: Cell<Option<*mut EventDescMap<T, InitDescF>>>,
+    // Using &'edm mut here means EventVisitor is invariant in any lifetime contained in T.
+    // However, the only values we store in the EventDescMap are either owned by it or have a
+    // longer lifetime ('h outlives 'edm), so it's sound to be covariant in 'edm.
+    desc_map: *mut EventDescMap<T, InitDescF>,
     _phantom_desc_map: PhantomData<&'edm mut ()>,
 
     scratch: &'i ScratchAlloc,
@@ -158,7 +154,7 @@ impl<'i, 'h, 'edm, InitDescF, T> EventVisitor<'i, 'h, 'edm, InitDescF, T> {
             timestamp,
             buffer_id,
             scratch,
-            desc_map: Cell::new(Some(desc_map)),
+            desc_map,
             event_desc: OnceCell::new(),
             _phantom_desc_map: PhantomData,
         }
@@ -281,28 +277,24 @@ where
         Ok(&desc.name)
     }
 
-    fn desc_map(&self) -> Option<&'edm mut EventDescMap<T, InitDescF>> {
-        Cell::take(&self.desc_map).map(|desc_map| {
-            // SAFETY: This comes from an &'edm mut reference in the first place, and since
-            // EventVisitor::new() requires an &'edm mut EventDescMap, we cannot accidentally
-            // borrow it mutably more than once. This makes it safe to turn it back to an &'edm
-            // mut.
-            let desc_map: &'edm mut EventDescMap<T, InitDescF> = unsafe {
-                &mut *desc_map
-            };
-            desc_map
-        })
+    fn desc_map(&self) -> &'edm mut EventDescMap<T, InitDescF> {
+        // SAFETY: This comes from an &'edm mut reference in the first place, and since
+        // EventVisitor::new() requires an &'edm mut EventDescMap, we cannot accidentally
+        // borrow it mutably more than once. This makes it safe to turn it back to an &'edm
+        // mut.
+        let desc_map = self.desc_map;
+        let desc_map: &'edm mut EventDescMap<T, InitDescF> = unsafe {
+            &mut *desc_map
+        };
+        desc_map
     }
 
     fn event_entry(&self) -> Result<(&'h EventDesc, &'edm T), BufferError> {
         self.event_desc
             .get_or_try_init(|| {
                 let event_id = self.event_id()?;
                 let not_found = || BufferError::EventDescriptorNotFound(event_id);
-                // If the desc_map has already been taken, it means we already
-                // tried to get the event_desc before and it failed.
-                let desc_map = self.desc_map().ok_or_else(not_found)?;
-                let (desc, user) = desc_map.lookup(event_id).ok_or_else(not_found)?;
+                let (desc, user) = self.desc_map().lookup(event_id).ok_or_else(not_found)?;
                 Ok((desc, user))
             }).map(|(desc, user)| {
                 let user: *const T = *user;