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

Author: martinvonz

gix-pack/src/cache/delta/traverse/mod.rs

@@ -9,7 +9,7 @@ use gix_features::{
 };
 
 use crate::{
-    cache::delta::{traverse::util::ItemSliceSend, Item, Tree},
+    cache::delta::{traverse::util::ItemSliceSync, Item, Tree},
     data::EntryRange,
 };
 
@@ -133,25 +133,23 @@ where
         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| {
-                        let _ = &child_items;
-                        resolve::State {
-                            delta_bytes: Vec::<u8>::with_capacity(4096),
-                            fully_resolved_delta_bytes: Vec::<u8>::with_capacity(4096),
-                            progress: Box::new(
-                                threading::lock(&object_progress).add_child(format!("thread {thread_index}")),
-                            ),
-                            resolve: resolve.clone(),
-                            modify_base: inspect_object.clone(),
-                            child_items: child_items.clone(),
-                        }
+                    move |thread_index| resolve::State {
+                        delta_bytes: Vec::<u8>::with_capacity(4096),
+                        fully_resolved_delta_bytes: Vec::<u8>::with_capacity(4096),
+                        progress: Box::new(
+                            threading::lock(&object_progress).add_child(format!("thread {thread_index}")),
+                        ),
+                        resolve: resolve.clone(),
+                        modify_base: inspect_object.clone(),
+                        child_items,
                     }
                 }
             },

gix-pack/src/cache/delta/traverse/resolve.rs

@@ -7,7 +7,7 @@ use gix_features::{progress::Progress, threading, zlib};
 
 use crate::{
     cache::delta::{
-        traverse::{util::ItemSliceSend, Context, Error},
+        traverse::{util::ItemSliceSync, Context, Error},
         Item,
     },
     data,
@@ -17,7 +17,7 @@ use crate::{
 /// 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> {
@@ -51,7 +51,7 @@ 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,
         })
     }
 }
@@ -62,7 +62,7 @@ pub(crate) struct State<'items, F, MBFN, T: Send> {
     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)]
@@ -106,13 +106,7 @@ where
     // 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![(
-        root_level,
-        Node {
-            item,
-            child_items: child_items.clone(),
-        },
-    )];
+    let mut nodes: Vec<_> = vec![(root_level, Node { item, child_items })];
     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

@@ -1,19 +1,19 @@
 use std::marker::PhantomData;
 
-pub(crate) struct ItemSliceSend<'a, T>
+pub(crate) struct ItemSliceSync<'a, T>
 where
     T: Send,
 {
     items: *mut T,
     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,
         }
@@ -27,20 +27,10 @@ where
     }
 }
 
-/// 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
-///         don't violate aliasing rules.
-impl<T> Clone for ItemSliceSend<'_, T>
-where
-    T: Send,
-{
-    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.
+// SAFETY: T is `Send`, and we only use the pointer for creating new pointers.
+#[allow(unsafe_code)]
+unsafe impl<T> Send for ItemSliceSync<'_, T> where T: Send {}
+// SAFETY: T is `Send`, and as long as the user follows the contract of
+// `get_mut()`, we only ever access one T at a time.
 #[allow(unsafe_code)]
-unsafe impl<T> Send for ItemSliceSend<'_, T> where T: Send {}
+unsafe impl<T> Sync for ItemSliceSync<'_, T> where T: Send {}