[Updated] [+-- ] D12582: rust-repo: make `Send` by not storing functions in `LazyCell`

martinvonz (Martin von Zweigbergk) phabricator at mercurial-scm.org
Tue May 3 06:06:47 UTC 2022


martinvonz updated this revision to Diff 33331.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D12582?vs=33302&id=33331

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D12582/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D12582

AFFECTED FILES
  rust/hg-core/src/repo.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/repo.rs b/rust/hg-core/src/repo.rs
--- a/rust/hg-core/src/repo.rs
+++ b/rust/hg-core/src/repo.rs
@@ -29,11 +29,11 @@
     store: PathBuf,
     requirements: HashSet<String>,
     config: Config,
-    dirstate_parents: LazyCell<DirstateParents, HgError>,
-    dirstate_data_file_uuid: LazyCell<Option<Vec<u8>>, HgError>,
-    dirstate_map: LazyCell<OwningDirstateMap, DirstateError>,
-    changelog: LazyCell<Changelog, HgError>,
-    manifestlog: LazyCell<Manifestlog, HgError>,
+    dirstate_parents: LazyCell<DirstateParents>,
+    dirstate_data_file_uuid: LazyCell<Option<Vec<u8>>>,
+    dirstate_map: LazyCell<OwningDirstateMap>,
+    changelog: LazyCell<Changelog>,
+    manifestlog: LazyCell<Manifestlog>,
 }
 
 #[derive(Debug, derive_more::From)]
@@ -182,13 +182,11 @@
             store: store_path,
             dot_hg,
             config: repo_config,
-            dirstate_parents: LazyCell::new(Self::read_dirstate_parents),
-            dirstate_data_file_uuid: LazyCell::new(
-                Self::read_dirstate_data_file_uuid,
-            ),
-            dirstate_map: LazyCell::new(Self::new_dirstate_map),
-            changelog: LazyCell::new(Self::new_changelog),
-            manifestlog: LazyCell::new(Self::new_manifestlog),
+            dirstate_parents: LazyCell::new(),
+            dirstate_data_file_uuid: LazyCell::new(),
+            dirstate_map: LazyCell::new(),
+            changelog: LazyCell::new(),
+            manifestlog: LazyCell::new(),
         };
 
         requirements::check(&repo)?;
@@ -260,7 +258,9 @@
     }
 
     pub fn dirstate_parents(&self) -> Result<DirstateParents, HgError> {
-        Ok(*self.dirstate_parents.get_or_init(self)?)
+        Ok(*self
+            .dirstate_parents
+            .get_or_init(|| self.read_dirstate_parents())?)
     }
 
     fn read_dirstate_parents(&self) -> Result<DirstateParents, HgError> {
@@ -340,13 +340,14 @@
     pub fn dirstate_map(
         &self,
     ) -> Result<Ref<OwningDirstateMap>, DirstateError> {
-        self.dirstate_map.get_or_init(self)
+        self.dirstate_map.get_or_init(|| self.new_dirstate_map())
     }
 
     pub fn dirstate_map_mut(
         &self,
     ) -> Result<RefMut<OwningDirstateMap>, DirstateError> {
-        self.dirstate_map.get_mut_or_init(self)
+        self.dirstate_map
+            .get_mut_or_init(|| self.new_dirstate_map())
     }
 
     fn new_changelog(&self) -> Result<Changelog, HgError> {
@@ -354,11 +355,11 @@
     }
 
     pub fn changelog(&self) -> Result<Ref<Changelog>, HgError> {
-        self.changelog.get_or_init(self)
+        self.changelog.get_or_init(|| self.new_changelog())
     }
 
     pub fn changelog_mut(&self) -> Result<RefMut<Changelog>, HgError> {
-        self.changelog.get_mut_or_init(self)
+        self.changelog.get_mut_or_init(|| self.new_changelog())
     }
 
     fn new_manifestlog(&self) -> Result<Manifestlog, HgError> {
@@ -366,11 +367,11 @@
     }
 
     pub fn manifestlog(&self) -> Result<Ref<Manifestlog>, HgError> {
-        self.manifestlog.get_or_init(self)
+        self.manifestlog.get_or_init(|| self.new_manifestlog())
     }
 
     pub fn manifestlog_mut(&self) -> Result<RefMut<Manifestlog>, HgError> {
-        self.manifestlog.get_mut_or_init(self)
+        self.manifestlog.get_mut_or_init(|| self.new_manifestlog())
     }
 
     /// Returns the manifest of the *changeset* with the given node ID
@@ -423,8 +424,10 @@
         // TODO: Maintain a `DirstateMap::dirty` flag, and return early here if
         // it’s unset
         let parents = self.dirstate_parents()?;
-        let (packed_dirstate, old_uuid_to_remove) = if self.has_dirstate_v2() {
-            let uuid = self.dirstate_data_file_uuid.get_or_init(self)?;
+        let packed_dirstate = if self.has_dirstate_v2() {
+            let (packed_dirstate, old_uuid_to_remove) = self
+                .dirstate_data_file_uuid
+                .get_or_init(|| self.read_dirstate_data_file_uuid())?;
             let mut uuid = uuid.as_ref();
             let can_append = uuid.is_some();
             let (data, tree_metadata, append, old_data_size) =
@@ -501,19 +504,17 @@
 /// Lazily-initialized component of `Repo` with interior mutability
 ///
 /// This differs from `OnceCell` in that the value can still be "deinitialized"
-/// later by setting its inner `Option` to `None`.
-struct LazyCell<T, E> {
+/// later by setting its inner `Option` to `None`. It also takes the
+/// initialization function as an argument when the value is requested, not
+/// when the instance is created.
+struct LazyCell<T> {
     value: RefCell<Option<T>>,
-    // `Fn`s that don’t capture environment are zero-size, so this box does
-    // not allocate:
-    init: Box<dyn Fn(&Repo) -> Result<T, E>>,
 }
 
-impl<T, E> LazyCell<T, E> {
-    fn new(init: impl Fn(&Repo) -> Result<T, E> + 'static) -> Self {
+impl<T> LazyCell<T> {
+    fn new() -> Self {
         Self {
             value: RefCell::new(None),
-            init: Box::new(init),
         }
     }
 
@@ -521,23 +522,29 @@
         *self.value.borrow_mut() = Some(value)
     }
 
-    fn get_or_init(&self, repo: &Repo) -> Result<Ref<T>, E> {
+    fn get_or_init<E>(
+        &self,
+        init: impl Fn() -> Result<T, E>,
+    ) -> Result<Ref<T>, E> {
         let mut borrowed = self.value.borrow();
         if borrowed.is_none() {
             drop(borrowed);
             // Only use `borrow_mut` if it is really needed to avoid panic in
             // case there is another outstanding borrow but mutation is not
             // needed.
-            *self.value.borrow_mut() = Some((self.init)(repo)?);
+            *self.value.borrow_mut() = Some(init()?);
             borrowed = self.value.borrow()
         }
         Ok(Ref::map(borrowed, |option| option.as_ref().unwrap()))
     }
 
-    fn get_mut_or_init(&self, repo: &Repo) -> Result<RefMut<T>, E> {
+    fn get_mut_or_init<E>(
+        &self,
+        init: impl Fn() -> Result<T, E>,
+    ) -> Result<RefMut<T>, E> {
         let mut borrowed = self.value.borrow_mut();
         if borrowed.is_none() {
-            *borrowed = Some((self.init)(repo)?);
+            *borrowed = Some(init()?);
         }
         Ok(RefMut::map(borrowed, |option| option.as_mut().unwrap()))
     }



To: martinvonz, #hg-reviewers, Alphare
Cc: Alphare, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mercurial-scm.org/pipermail/mercurial-patches/attachments/20220503/fde95ece/attachment-0001.html>


More information about the Mercurial-patches mailing list