refactor (#287)

Remove VecDeque in favor of two vecs which will keep their indices
stable no matter what, while making much clearer what's happening.

It's surprising I wasn't able to do this refactor the first time around
and shows how hard it was to understand (for me) :D.

Author: Byron

git-odb/src/store_impls/loose/verify.rs

@@ -52,7 +52,7 @@ impl Store {
 
         let mut num_objects = 0;
         let start = Instant::now();
-        let mut progress = progress.add_child("validating");
+        let mut progress = progress.add_child("Validating");
         progress.init(None, git_features::progress::count("objects"));
         for id in self.iter().filter_map(Result::ok) {
             let object = self

git-pack/src/cache/delta/iter.rs

@@ -4,16 +4,12 @@ use crate::cache::delta::{Item, Tree};
 impl<T> Tree<T> {
     /// Return an iterator over chunks of roots. Roots are not children themselves, they have no parents.
     pub fn iter_root_chunks(&mut self, chunk_size: usize) -> impl Iterator<Item = Chunk<'_, T>> + '_ {
-        let (front, back) = self.items.as_mut_slices();
-        assert_eq!(
-            front.len(),
-            self.roots,
-            "This should not happen unless we added more nodes than were declared in the constructor"
-        );
+        let roots = self.root_items.as_mut_slice();
+        let children = self.child_items.as_mut_slice();
 
-        front.chunks_mut(chunk_size).map(move |c| Chunk {
+        roots.chunks_mut(chunk_size).map(move |c| Chunk {
             inner: c.iter_mut(),
-            children: back as *mut [Item<T>],
+            children: children as *mut [Item<T>],
         })
     }
 }

git-pack/src/cache/delta/mod.rs

@@ -1,5 +1,3 @@
-use std::collections::VecDeque;
-
 /// Returned when using various methods on a [`Tree`]
 #[derive(thiserror::Error, Debug)]
 #[allow(missing_docs)]
@@ -32,15 +30,23 @@ pub struct Item<T> {
     /// Indices into our Tree's `items`, one for each pack entry that depends on us.
     children: Vec<usize>,
 }
+
+/// Identify what kind of node we have last seen
+enum NodeKind {
+    Root,
+    Child,
+}
+
 /// A tree that allows one-time iteration over all nodes and their children, consuming it in the process,
 /// while being shareable among threads without a lock.
 /// It does this by making the guarantee that iteration only happens once.
 pub struct Tree<T> {
-    /// Roots are first, then children.
-    items: VecDeque<Item<T>>,
-    roots: usize,
-    /// The last child index into the `items` array
-    last_index: usize,
+    /// The root nodes, i.e. base objects
+    root_items: Vec<Item<T>>,
+    /// The child nodes, i.e. those that rely a base object, like ref and ofs delta objets
+    child_items: Vec<Item<T>>,
+    /// The last encountered node was either a root or a child.
+    last_seen: Option<NodeKind>,
     /// Future child offsets, associating their offset into the pack with their index in the items array.
     /// (parent_offset, child_index)
     future_child_offsets: Vec<(crate::data::Offset, usize)>,
@@ -50,22 +56,27 @@ impl<T> Tree<T> {
     /// Instantiate a empty tree capable of storing `num_objects` amounts of items.
     pub fn with_capacity(num_objects: usize) -> Result<Self, Error> {
         Ok(Tree {
-            items: VecDeque::with_capacity(num_objects),
-            roots: 0,
-            last_index: 0,
+            root_items: Vec::with_capacity(num_objects / 2),
+            child_items: Vec::with_capacity(num_objects / 2),
+            last_seen: None,
             future_child_offsets: Vec::new(),
         })
     }
 
+    fn num_items(&self) -> usize {
+        self.root_items.len() + self.child_items.len()
+    }
+
     fn assert_is_incrementing_and_update_next_offset(&mut self, offset: crate::data::Offset) -> Result<(), Error> {
-        if self.items.is_empty() {
-            return Ok(());
-        }
-        let item = &mut self.items[self.last_index];
-        let last_offset = item.offset;
-        if offset <= last_offset {
+        let items = match &self.last_seen {
+            Some(NodeKind::Root) => &mut self.root_items,
+            Some(NodeKind::Child) => &mut self.child_items,
+            None => return Ok(()),
+        };
+        let item = &mut items.last_mut().expect("last seen won't lie");
+        if offset <= item.offset {
             return Err(Error::InvariantIncreasingPackOffset {
-                last_pack_offset: last_offset,
+                last_pack_offset: item.offset,
                 pack_offset: offset,
             });
         }
@@ -77,22 +88,12 @@ impl<T> Tree<T> {
         &mut self,
         pack_entries_end: crate::data::Offset,
     ) -> Result<(), traverse::Error> {
-        if self.items.is_empty() {
-            return Ok(());
-        };
-
         if !self.future_child_offsets.is_empty() {
-            let (roots, children) = self.items.as_mut_slices();
-            assert_eq!(
-                roots.len(),
-                self.roots,
-                "item deque has been resized, maybe we added more nodes than we declared in the constructor?"
-            );
             for (parent_offset, child_index) in self.future_child_offsets.drain(..) {
-                if let Ok(i) = children.binary_search_by_key(&parent_offset, |i| i.offset) {
-                    children[i].children.push(child_index);
-                } else if let Ok(i) = roots.binary_search_by(|i| parent_offset.cmp(&i.offset)) {
-                    roots[i].children.push(child_index);
+                if let Ok(i) = self.child_items.binary_search_by_key(&parent_offset, |i| i.offset) {
+                    self.child_items[i].children.push(child_index);
+                } else if let Ok(i) = self.root_items.binary_search_by_key(&parent_offset, |i| i.offset) {
+                    self.root_items[i].children.push(child_index);
                 } else {
                     return Err(traverse::Error::OutOfPackRefDelta {
                         base_pack_offset: parent_offset,
@@ -101,22 +102,22 @@ impl<T> Tree<T> {
             }
         }
 
-        self.items[self.last_index].next_offset = pack_entries_end;
+        self.assert_is_incrementing_and_update_next_offset(pack_entries_end)
+            .expect("BUG: pack now is smaller than all previously seen entries");
         Ok(())
     }
 
     /// Add a new root node, one that only has children but is not a child itself, at the given pack `offset` and associate
     /// custom `data` with it.
     pub fn add_root(&mut self, offset: crate::data::Offset, data: T) -> Result<(), Error> {
         self.assert_is_incrementing_and_update_next_offset(offset)?;
-        self.last_index = 0;
-        self.items.push_front(Item {
+        self.last_seen = NodeKind::Root.into();
+        self.root_items.push(Item {
             offset,
             next_offset: 0,
             data,
             children: Vec::new(),
         });
-        self.roots += 1;
         Ok(())
     }
 
@@ -128,22 +129,18 @@ impl<T> Tree<T> {
         data: T,
     ) -> Result<(), Error> {
         self.assert_is_incrementing_and_update_next_offset(offset)?;
-        let (roots, children) = self.items.as_mut_slices();
-        assert_eq!(
-            roots.len(),
-            self.roots,
-            "item deque has been resized, maybe we added more nodes than we declared in the constructor?"
-        );
-        let next_child_index = children.len();
-        if let Ok(i) = children.binary_search_by_key(&base_offset, |i| i.offset) {
-            children[i].children.push(next_child_index);
-        } else if let Ok(i) = roots.binary_search_by(|i| base_offset.cmp(&i.offset)) {
-            roots[i].children.push(next_child_index);
+
+        let next_child_index = self.child_items.len();
+        if let Ok(i) = self.child_items.binary_search_by_key(&base_offset, |i| i.offset) {
+            self.child_items[i].children.push(next_child_index);
+        } else if let Ok(i) = self.root_items.binary_search_by_key(&base_offset, |i| i.offset) {
+            self.root_items[i].children.push(next_child_index);
         } else {
             self.future_child_offsets.push((base_offset, next_child_index));
         }
-        self.last_index = self.items.len();
-        self.items.push_back(Item {
+
+        self.last_seen = NodeKind::Child.into();
+        self.child_items.push(Item {
             offset,
             next_offset: 0,
             data,
@@ -153,8 +150,8 @@ impl<T> Tree<T> {
     }
 
     /// Transform this `Tree` into its items.
-    pub fn into_items(self) -> VecDeque<Item<T>> {
-        self.items
+    pub fn into_items(self) -> (Vec<Item<T>>, Vec<Item<T>>) {
+        (self.root_items, self.child_items)
     }
 }
 

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

@@ -1,7 +1,4 @@
-use std::{
-    collections::VecDeque,
-    sync::atomic::{AtomicBool, Ordering},
-};
+use std::sync::atomic::{AtomicBool, Ordering};
 
 use git_features::{
     parallel,
@@ -107,7 +104,7 @@ where
             should_interrupt,
             object_hash,
         }: Options<'_, P1, P2>,
-    ) -> Result<VecDeque<Item<T>>, Error>
+    ) -> Result<(Vec<Item<T>>, Vec<Item<T>>), Error>
     where
         F: for<'r> Fn(EntryRange, &'r mut Vec<u8>) -> Option<()> + Send + Clone,
         P1: Progress,
@@ -119,7 +116,7 @@ where
         let (chunk_size, thread_limit, _) = parallel::optimize_chunk_size_and_thread_limit(1, None, thread_limit, None);
         let object_progress = OwnShared::new(Mutable::new(object_progress));
 
-        let num_objects = self.items.len();
+        let num_objects = self.num_items();
         in_parallel_if(
             should_run_in_parallel,
             self.iter_root_chunks(chunk_size),

git-pack/src/index/traverse/with_index.rs

@@ -1,7 +1,4 @@
-use std::{
-    collections::VecDeque,
-    sync::atomic::{AtomicBool, Ordering},
-};
+use std::sync::atomic::{AtomicBool, Ordering};
 
 use git_features::{parallel, progress::Progress};
 
@@ -163,10 +160,15 @@ impl From<crate::index::Entry> for Entry {
     }
 }
 
-fn digest_statistics(items: VecDeque<crate::cache::delta::Item<Entry>>) -> index::traverse::Statistics {
+fn digest_statistics(
+    (roots, children): (
+        Vec<crate::cache::delta::Item<Entry>>,
+        Vec<crate::cache::delta::Item<Entry>>,
+    ),
+) -> index::traverse::Statistics {
     let mut res = index::traverse::Statistics::default();
     let average = &mut res.average;
-    for item in &items {
+    for item in roots.iter().chain(children.iter()) {
         res.total_compressed_entries_size += item.data.compressed_size;
         res.total_decompressed_entries_size += item.data.decompressed_size;
         res.total_object_size += item.data.object_size;
@@ -185,10 +187,11 @@ fn digest_statistics(items: VecDeque<crate::cache::delta::Item<Entry>>) -> index
         };
     }
 
-    average.decompressed_size /= items.len() as u64;
-    average.compressed_size /= items.len();
-    average.object_size /= items.len() as u64;
-    average.num_deltas /= items.len() as u32;
+    let num_nodes = roots.len() + children.len();
+    average.decompressed_size /= num_nodes as u64;
+    average.compressed_size /= num_nodes;
+    average.object_size /= num_nodes as u64;
+    average.num_deltas /= num_nodes as u32;
 
     res
 }

git-pack/src/index/write/encode.rs

@@ -1,4 +1,4 @@
-use std::{cmp::Ordering, collections::VecDeque, io};
+use std::{cmp::Ordering, io};
 
 pub(crate) const LARGE_OFFSET_THRESHOLD: u64 = 0x7fff_ffff;
 pub(crate) const HIGH_BIT: u32 = 0x8000_0000;
@@ -13,7 +13,7 @@ use crate::index::{util::Count, V2_SIGNATURE};
 
 pub(crate) fn write_to(
     out: impl io::Write,
-    entries_sorted_by_oid: VecDeque<crate::cache::delta::Item<crate::index::write::TreeEntry>>,
+    entries_sorted_by_oid: Vec<crate::cache::delta::Item<crate::index::write::TreeEntry>>,
     pack_hash: &git_hash::ObjectId,
     kind: crate::index::Version,
     mut progress: impl Progress,

git-pack/src/index/write/mod.rs

@@ -156,7 +156,7 @@ impl crate::index::File {
         let resolver = make_resolver()?;
         let sorted_pack_offsets_by_oid = {
             let in_parallel_if_pack_is_big_enough = || bytes_to_process > 5_000_000;
-            let mut items = tree.traverse(
+            let (roots, children) = tree.traverse(
                 in_parallel_if_pack_is_big_enough,
                 resolver,
                 pack_entries_end,
@@ -181,9 +181,11 @@ impl crate::index::File {
             )?;
             root_progress.inc();
 
+            let mut items = roots;
+            items.extend(children);
             {
                 let _progress = root_progress.add_child("sorting by id");
-                items.make_contiguous().sort_by_key(|e| e.data.id);
+                items.sort_by_key(|e| e.data.id);
             }
 
             root_progress.inc();