rust-index: use interior mutability in head revs and caches
authorGeorges Racinet on incendie.racinet.fr <georges@racinet.fr>
Fri, 27 Oct 2023 21:48:45 +0200
changeset 51234 59183a19954e
parent 51233 ca81cd96000a
child 51235 456e0fe702e8
rust-index: use interior mutability in head revs and caches For upcoming changes in `hg-cpython` switching to the `hg-core` index in ancestors iterators, we will need to avoid excessive mutability, restricting the use of mutable references on `hg::index::Index` to methods that actually logically mutate it, whereas the maintenance of caches such as `head_revs` clearly does not. We illustrate that immediately by switching to immutable borrows in the corresponding methods of `hg-cpython::MixedIndex`
rust/hg-core/src/revlog/index.rs
rust/hg-cpython/src/revlog.rs
--- a/rust/hg-core/src/revlog/index.rs	Thu Oct 26 15:26:19 2023 +0200
+++ b/rust/hg-core/src/revlog/index.rs	Fri Oct 27 21:48:45 2023 +0200
@@ -261,12 +261,13 @@
     offsets: RwLock<Option<Vec<usize>>>,
     uses_generaldelta: bool,
     is_inline: bool,
-    /// Cache of the head revisions in this index, kept in sync. Should
+    /// Cache of (head_revisions, filtered_revisions)
+    ///
+    /// The head revisions in this index, kept in sync. Should
     /// be accessed via the [`Self::head_revs`] method.
-    head_revs: Vec<Revision>,
-    /// Cache of the last filtered revisions in this index, used to make sure
+    /// The last filtered revisions in this index, used to make sure
     /// we haven't changed filters when returning the cached `head_revs`.
-    filtered_revs: HashSet<Revision>,
+    head_revs: RwLock<(Vec<Revision>, HashSet<Revision>)>,
 }
 
 impl Debug for Index {
@@ -366,8 +367,7 @@
                     offsets: RwLock::new(Some(offsets)),
                     uses_generaldelta,
                     is_inline: true,
-                    head_revs: vec![],
-                    filtered_revs: HashSet::new(),
+                    head_revs: RwLock::new((vec![], HashSet::new())),
                 })
             } else {
                 Err(HgError::corrupted("unexpected inline revlog length"))
@@ -378,8 +378,7 @@
                 offsets: RwLock::new(None),
                 uses_generaldelta,
                 is_inline: false,
-                head_revs: vec![],
-                filtered_revs: HashSet::new(),
+                head_revs: RwLock::new((vec![], HashSet::new())),
             })
         }
     }
@@ -532,17 +531,27 @@
     }
 
     /// Return the head revisions of this index
-    pub fn head_revs(&mut self) -> Result<Vec<Revision>, GraphError> {
+    pub fn head_revs(&self) -> Result<Vec<Revision>, GraphError> {
         self.head_revs_filtered(&HashSet::new())
     }
 
     /// Return the head revisions of this index
     pub fn head_revs_filtered(
-        &mut self,
+        &self,
         filtered_revs: &HashSet<Revision>,
     ) -> Result<Vec<Revision>, GraphError> {
-        if !self.head_revs.is_empty() && filtered_revs == &self.filtered_revs {
-            return Ok(self.head_revs.to_owned());
+        {
+            let guard = self
+                .head_revs
+                .read()
+                .expect("RwLock on Index.head_revs should not be poisoned");
+            let self_head_revs = &guard.0;
+            let self_filtered_revs = &guard.1;
+            if !self_head_revs.is_empty()
+                && filtered_revs == self_filtered_revs
+            {
+                return Ok(self_head_revs.to_owned());
+            }
         }
         let mut revs: HashSet<Revision, RandomState> =
             if filtered_revs.is_empty() {
@@ -570,8 +579,11 @@
         let mut as_vec: Vec<Revision> =
             revs.into_iter().map(Into::into).collect();
         as_vec.sort_unstable();
-        self.head_revs = as_vec.to_owned();
-        self.filtered_revs = filtered_revs.to_owned();
+        *self
+            .head_revs
+            .write()
+            .expect("RwLock on Index.head_revs should not be poisoned") =
+            (as_vec.to_owned(), filtered_revs.to_owned());
         Ok(as_vec)
     }
 
@@ -651,6 +663,14 @@
         Ok(())
     }
 
+    fn clear_head_revs(&self) {
+        self.head_revs
+            .write()
+            .expect("RwLock on Index.head_revs should not be poisoined")
+            .0
+            .clear()
+    }
+
     /// TODO move this to the trait probably, along with other things
     pub fn append(
         &mut self,
@@ -662,7 +682,7 @@
             offsets.push(new_offset)
         }
         self.bytes.added.extend(revision_data.into_v1().as_bytes());
-        self.head_revs.clear();
+        self.clear_head_revs();
         Ok(())
     }
 
@@ -676,16 +696,19 @@
         if let Some(offsets) = &mut *self.get_offsets_mut() {
             offsets.truncate(rev.0 as usize)
         }
-        self.head_revs.clear();
+        self.clear_head_revs();
         Ok(())
     }
 
-    pub fn clear_caches(&mut self) {
+    pub fn clear_caches(&self) {
         // We need to get the 'inline' value from Python at init and use this
         // instead of offsets to determine whether we're inline since we might
         // clear caches. This implies re-populating the offsets on-demand.
-        self.offsets = RwLock::new(None);
-        self.head_revs.clear();
+        *self
+            .offsets
+            .write()
+            .expect("RwLock on Index.offsets should not be poisoed") = None;
+        self.clear_head_revs();
     }
 
     /// Unchecked version of `is_snapshot`.
--- a/rust/hg-cpython/src/revlog.rs	Thu Oct 26 15:26:19 2023 +0200
+++ b/rust/hg-cpython/src/revlog.rs	Fri Oct 27 21:48:45 2023 +0200
@@ -225,7 +225,7 @@
         self.nt(py).borrow_mut().take();
         self.docket(py).borrow_mut().take();
         self.nodemap_mmap(py).borrow_mut().take();
-        self.index(py).borrow_mut().clear_caches();
+        self.index(py).borrow().clear_caches();
         self.call_cindex(py, "clearcaches", args, kw)
     }
 
@@ -849,7 +849,7 @@
     }
 
     fn inner_headrevs(&self, py: Python) -> PyResult<PyObject> {
-        let index = &mut *self.index(py).borrow_mut();
+        let index = &*self.index(py).borrow();
         let as_vec: Vec<PyObject> = index
             .head_revs()
             .map_err(|e| graph_error(py, e))?
@@ -881,7 +881,7 @@
         py: Python,
         py_revs: &PyTuple,
     ) -> PyResult<PyObject> {
-        let index = &mut *self.index(py).borrow_mut();
+        let index = &*self.index(py).borrow();
         let revs: Vec<_> = rev_pyiter_collect(py, py_revs.as_object(), index)?;
         let as_vec: Vec<_> = index
             .ancestors(&revs)
@@ -897,7 +897,7 @@
         py: Python,
         py_revs: &PyTuple,
     ) -> PyResult<PyObject> {
-        let index = &mut *self.index(py).borrow_mut();
+        let index = &*self.index(py).borrow();
         let revs: Vec<_> = rev_pyiter_collect(py, py_revs.as_object(), index)?;
         let as_vec: Vec<_> = index
             .common_ancestor_heads(&revs)
@@ -980,7 +980,7 @@
         target_density: f64,
         min_gap_size: usize,
     ) -> PyResult<PyObject> {
-        let index = &mut *self.index(py).borrow_mut();
+        let index = &*self.index(py).borrow();
         let revs: Vec<_> = rev_pyiter_collect(py, &revs, index)?;
         let as_nested_vec =
             index.slice_chunk_to_density(&revs, target_density, min_gap_size);