This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12582

rust-repo: make `Send` by not storing functions in `LazyCell`
ClosedPublic

Authored by martinvonz on Apr 21 2022, 2:12 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG90a15199cbc6: rust-repo: make `Send` by not storing functions in `LazyCell`
rHG5376efe0314a: rust-repo: make `Send` by not storing functions in `LazyCell`
Summary

We (Google) want to use Repo in a context where we can store it in
Mutex<Repo>. However, that currently doesn't work because it's not
Send because the LazyCell initialization functions are not
Send. It's easy to fix that by passing them to the get_or_init()
and get_mut_or_init() functions. We'll probably also want Repo to
be Send (and even Sync) in core later, so this seems like a step
in the right direction.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Apr 21 2022, 2:12 PM
Alphare accepted this revision.Apr 25 2022, 4:53 AM
Alphare added a subscriber: Alphare.

Thanks!

This revision is now accepted and ready to land.Apr 25 2022, 4:53 AM
martinvonz updated this revision to Diff 33331.May 3 2022, 2:06 AM
martinvonz updated this revision to Diff 33332.May 3 2022, 11:55 AM
Alphare requested changes to this revision.Mon, May 9, 11:26 AM

Unfortunately, this does not apply anymore. Sorry for the delay in review, it's been really packed lately.

This revision now requires changes to proceed.Mon, May 9, 11:26 AM
martinvonz updated this revision to Diff 33372.Mon, May 9, 11:42 AM
martinvonz added a comment.Mon, May 9, 11:42 AM
In D12582#193131, @Alphare wrote:

Unfortunately, this does not apply anymore. Sorry for the delay in review, it's been really packed lately.

No problem. I've rebased and resolved the conflict now.

martinvonz added a commit: rHG5376efe0314a: rust-repo: make `Send` by not storing functions in `LazyCell`.Mon, May 9, 11:53 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG5376efe0314a: rust-repo: make `Send` by not storing functions in `LazyCell` (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/repo.rs (69 lines)

Diff 33385

rust/hg-core/src/repo.rs

/// A repository on disk /// A repository on disk
pub struct Repo { pub struct Repo {
working_directory: PathBuf, working_directory: PathBuf,
dot_hg: PathBuf, dot_hg: PathBuf,
store: PathBuf, store: PathBuf,
requirements: HashSet<String>, requirements: HashSet<String>,
config: Config, config: Config,
dirstate_parents: LazyCell<DirstateParents, HgError>, dirstate_parents: LazyCell<DirstateParents>,
dirstate_data_file_uuid: LazyCell<Option<Vec<u8>>, HgError>, dirstate_data_file_uuid: LazyCell<Option<Vec<u8>>>,
dirstate_map: LazyCell<OwningDirstateMap, DirstateError>, dirstate_map: LazyCell<OwningDirstateMap>,
changelog: LazyCell<Changelog, HgError>, changelog: LazyCell<Changelog>,
manifestlog: LazyCell<Manifestlog, HgError>, manifestlog: LazyCell<Manifestlog>,
} }
#[derive(Debug, derive_more::From)] #[derive(Debug, derive_more::From)]
pub enum RepoError { pub enum RepoError {
NotFound { NotFound {
at: PathBuf, at: PathBuf,
}, },
#[from] #[from]
}; };
let repo = Self { let repo = Self {
requirements: reqs, requirements: reqs,
working_directory, working_directory,
store: store_path, store: store_path,
dot_hg, dot_hg,
config: repo_config, config: repo_config,
dirstate_parents: LazyCell::new(Self::read_dirstate_parents), dirstate_parents: LazyCell::new(),
dirstate_data_file_uuid: LazyCell::new( dirstate_data_file_uuid: LazyCell::new(),
Self::read_dirstate_data_file_uuid, dirstate_map: LazyCell::new(),
), changelog: LazyCell::new(),
dirstate_map: LazyCell::new(Self::new_dirstate_map), manifestlog: LazyCell::new(),
changelog: LazyCell::new(Self::new_changelog),
manifestlog: LazyCell::new(Self::new_manifestlog),
}; };
requirements::check(&repo)?; requirements::check(&repo)?;
Ok(repo) Ok(repo)
} }
pub fn working_directory_path(&self) -> &Path { pub fn working_directory_path(&self) -> &Path {
Ok(self Ok(self
.hg_vfs() .hg_vfs()
.read("dirstate") .read("dirstate")
.io_not_found_as_none()? .io_not_found_as_none()?
.unwrap_or(Vec::new())) .unwrap_or(Vec::new()))
} }
pub fn dirstate_parents(&self) -> Result<DirstateParents, HgError> { 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> { fn read_dirstate_parents(&self) -> Result<DirstateParents, HgError> {
let dirstate = self.dirstate_file_contents()?; let dirstate = self.dirstate_file_contents()?;
let parents = if dirstate.is_empty() { let parents = if dirstate.is_empty() {
if self.has_dirstate_v2() { if self.has_dirstate_v2() {
self.dirstate_data_file_uuid.set(None); self.dirstate_data_file_uuid.set(None);
} }
self.dirstate_parents.set(parents); self.dirstate_parents.set(parents);
Ok(map) Ok(map)
} }
} }
pub fn dirstate_map( pub fn dirstate_map(
&self, &self,
) -> Result<Ref<OwningDirstateMap>, DirstateError> { ) -> 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( pub fn dirstate_map_mut(
&self, &self,
) -> Result<RefMut<OwningDirstateMap>, DirstateError> { ) -> 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> { fn new_changelog(&self) -> Result<Changelog, HgError> {
Changelog::open(&self.store_vfs(), self.has_nodemap()) Changelog::open(&self.store_vfs(), self.has_nodemap())
} }
pub fn changelog(&self) -> Result<Ref<Changelog>, HgError> { 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> { 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> { fn new_manifestlog(&self) -> Result<Manifestlog, HgError> {
Manifestlog::open(&self.store_vfs(), self.has_nodemap()) Manifestlog::open(&self.store_vfs(), self.has_nodemap())
} }
pub fn manifestlog(&self) -> Result<Ref<Manifestlog>, HgError> { 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> { 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 /// Returns the manifest of the *changeset* with the given node ID
pub fn manifest_for_node( pub fn manifest_for_node(
&self, &self,
node: impl Into<NodePrefix>, node: impl Into<NodePrefix>,
) -> Result<Manifest, RevlogError> { ) -> Result<Manifest, RevlogError> {
self.manifestlog()?.data_for_node( self.manifestlog()?.data_for_node(
/// TODO: have a `WritableRepo` type only accessible while holding the /// TODO: have a `WritableRepo` type only accessible while holding the
/// lock? /// lock?
pub fn write_dirstate(&self) -> Result<(), DirstateError> { pub fn write_dirstate(&self) -> Result<(), DirstateError> {
let map = self.dirstate_map()?; let map = self.dirstate_map()?;
// TODO: Maintain a `DirstateMap::dirty` flag, and return early here if // TODO: Maintain a `DirstateMap::dirty` flag, and return early here if
// it’s unset // it’s unset
let parents = self.dirstate_parents()?; let parents = self.dirstate_parents()?;
let (packed_dirstate, old_uuid_to_remove) = if self.has_dirstate_v2() { let (packed_dirstate, old_uuid_to_remove) = if self.has_dirstate_v2() {
let uuid_opt = self.dirstate_data_file_uuid.get_or_init(self)?; let uuid_opt = self.dirstate_data_file_uuid.get_or_init(|| self.read_dirstate_data_file_uuid())?;
let uuid_opt = uuid_opt.as_ref(); let uuid_opt = uuid_opt.as_ref();
let can_append = uuid_opt.is_some(); let can_append = uuid_opt.is_some();
let (data, tree_metadata, append, old_data_size) = let (data, tree_metadata, append, old_data_size) =
map.pack_v2(can_append)?; map.pack_v2(can_append)?;
// Reuse the uuid, or generate a new one, keeping the old for // Reuse the uuid, or generate a new one, keeping the old for
// deletion. // deletion.
let (uuid, old_uuid) = match uuid_opt { let (uuid, old_uuid) = match uuid_opt {
} }
Ok(()) Ok(())
} }
} }
/// Lazily-initialized component of `Repo` with interior mutability /// Lazily-initialized component of `Repo` with interior mutability
/// ///
/// This differs from `OnceCell` in that the value can still be "deinitialized" /// This differs from `OnceCell` in that the value can still be "deinitialized"
/// later by setting its inner `Option` to `None`. /// later by setting its inner `Option` to `None`. It also takes the
struct LazyCell<T, E> { /// initialization function as an argument when the value is requested, not
/// when the instance is created.
struct LazyCell<T> {
value: RefCell<Option<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> { impl<T> LazyCell<T> {
fn new(init: impl Fn(&Repo) -> Result<T, E> + 'static) -> Self { fn new() -> Self {
Self { Self {
value: RefCell::new(None), value: RefCell::new(None),
init: Box::new(init),
} }
} }
fn set(&self, value: T) { fn set(&self, value: T) {
*self.value.borrow_mut() = Some(value) *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(); let mut borrowed = self.value.borrow();
if borrowed.is_none() { if borrowed.is_none() {
drop(borrowed); drop(borrowed);
// Only use `borrow_mut` if it is really needed to avoid panic in // Only use `borrow_mut` if it is really needed to avoid panic in
// case there is another outstanding borrow but mutation is not // case there is another outstanding borrow but mutation is not
// needed. // needed.
*self.value.borrow_mut() = Some((self.init)(repo)?); *self.value.borrow_mut() = Some(init()?);
borrowed = self.value.borrow() borrowed = self.value.borrow()
} }
Ok(Ref::map(borrowed, |option| option.as_ref().unwrap())) 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(); let mut borrowed = self.value.borrow_mut();
if borrowed.is_none() { if borrowed.is_none() {
*borrowed = Some((self.init)(repo)?); *borrowed = Some(init()?);
} }
Ok(RefMut::map(borrowed, |option| option.as_mut().unwrap())) Ok(RefMut::map(borrowed, |option| option.as_mut().unwrap()))
} }
} }