remove `Clone` impl of `ItemSliceSend` and make it `Sync` instead

The `Clone` impl allows two `ItemSliceSend` to be backed by the same slice. That makes the behavior of `get_mut()` harder to reason about because we have to consider clones of the `ItemSliceSend` to determine if it's safe. By making it `Sync`, we make it more obvious that multiple threads referring to the same instance need to be considered. I renamed the type to `ItemSliceSync` since it's now also `Sync`. We could make the type also check the bounds and that an individual element has not been borrowed more than once. Then we could make `get_mut()` not `unsafe`.

Martin von Zweigbergk committed 2 years ago

Commit 43bca111 parent 7301ca13 files · +26 −44Browse files

Unified · Split

3 files

gix-pack/src/cache/delta/traverse/mod.rs +12 −14
gix-pack/src/cache/delta/traverse/resolve.rs +5 −11
gix-pack/src/cache/delta/traverse/util.rs +9 −19

gix-pack/src/cache/delta/traverse/mod.rs+12−14

 };
 use crate::{
-    cache::delta::{traverse::util::ItemSliceSend, Item, Tree},
+    cache::delta::{traverse::util::ItemSliceSync, Item, Tree},
     data::EntryRange,
 };
         let object_progress = OwnShared::new(Mutable::new(object_progress));
         let start = std::time::Instant::now();
+        let child_items = ItemSliceSync::new(&mut self.child_items);
+        let child_items = &child_items;
         in_parallel_with_slice(
             &mut self.root_items,
             thread_limit,
             {
-                let child_items = ItemSliceSend::new(&mut self.child_items);
                 {
                     let object_progress = object_progress.clone();
-                    move |thread_index| {
+                    move |thread_index| resolve::State {
-                        let _ = &child_items;
+                        delta_bytes: Vec::<u8>::with_capacity(4096),
-                        resolve::State {
+                        fully_resolved_delta_bytes: Vec::<u8>::with_capacity(4096),
-                            delta_bytes: Vec::<u8>::with_capacity(4096),
+                        progress: Box::new(
-                            fully_resolved_delta_bytes: Vec::<u8>::with_capacity(4096),
+                            threading::lock(&object_progress).add_child(format!("thread {thread_index}")),
-                            progress: Box::new(
+                        ),
-                                threading::lock(&object_progress).add_child(format!("thread {thread_index}")),
+                        resolve: resolve.clone(),
-                            ),
+                        modify_base: inspect_object.clone(),
-                            resolve: resolve.clone(),
+                        child_items,
-                            modify_base: inspect_object.clone(),
-                            child_items: child_items.clone(),
-                        }
                     }
                 }
             },

gix-pack/src/cache/delta/traverse/resolve.rs+5−11

 use crate::{
     cache::delta::{
-        traverse::{util::ItemSliceSend, Context, Error},
+        traverse::{util::ItemSliceSync, Context, Error},
         Item,
     },
     data,
 /// An item returned by `iter_root_chunks`, allowing access to the `data` stored alongside nodes in a [`Tree`].
 struct Node<'a, T: Send> {
     item: &'a mut Item<T>,
-    child_items: ItemSliceSend<'a, Item<T>>,
+    child_items: &'a ItemSliceSync<'a, Item<T>>,
 }
 impl<'a, T: Send> Node<'a, T> {
         #[allow(unsafe_code)]
         self.item.children.iter().map(move |&index| Node {
             item: unsafe { children.get_mut(index as usize) },
-            child_items: children.clone(),
+            child_items: children,
         })
     }
 }
     pub progress: Box<dyn Progress>,
     pub resolve: F,
     pub modify_base: MBFN,
-    pub child_items: ItemSliceSend<'items, Item<T>>,
+    pub child_items: &'items ItemSliceSync<'items, Item<T>>,
 }
 #[allow(clippy::too_many_arguments)]
     // each node is a base, and its children always start out as deltas which become a base after applying them.
     // These will be pushed onto our stack until all are processed
     let root_level = 0;
-    let mut nodes: Vec<_> = vec![(
+    let mut nodes: Vec<_> = vec![(root_level, Node { item, child_items })];
-        root_level,
-        Node {
-            item,
-            child_items: child_items.clone(),
-        },
-    )];
     while let Some((level, mut base)) = nodes.pop() {
         if should_interrupt.load(Ordering::Relaxed) {
             return Err(Error::Interrupted);

gix-pack/src/cache/delta/traverse/util.rs+9−19

 use std::marker::PhantomData;
-pub(crate) struct ItemSliceSend<'a, T>
+pub(crate) struct ItemSliceSync<'a, T>
 where
     T: Send,
 {
     phantom: PhantomData<&'a T>,
 }
-impl<'a, T> ItemSliceSend<'a, T>
+impl<'a, T> ItemSliceSync<'a, T>
 where
     T: Send,
 {
     pub fn new(items: &'a mut [T]) -> Self {
-        ItemSliceSend {
+        ItemSliceSync {
             items: items.as_mut_ptr(),
             phantom: PhantomData,
         }
     }
 }
-/// SAFETY: This would be unsafe if this would ever be abused, but it's used internally and only in a way that assure that the pointers
+// SAFETY: T is `Send`, and we only use the pointer for creating new pointers.
-///         don't violate aliasing rules.
+#[allow(unsafe_code)]
-impl<T> Clone for ItemSliceSend<'_, T>
+unsafe impl<T> Send for ItemSliceSync<'_, T> where T: Send {}
-where
+// SAFETY: T is `Send`, and as long as the user follows the contract of
-    T: Send,
+// `get_mut()`, we only ever access one T at a time.
-{
-    fn clone(&self) -> Self {
-        ItemSliceSend {
-            items: self.items,
-            phantom: self.phantom,
-        }
-    }
-}
-// SAFETY: T is `Send`, and we only ever access one T at a time. And, ptrs need that assurance, I wonder if it's always right.
 #[allow(unsafe_code)]
-unsafe impl<T> Send for ItemSliceSend<'_, T> where T: Send {}
+unsafe impl<T> Sync for ItemSliceSync<'_, T> where T: Send {}