Mercurial > hg
changeset 51239:7eea2e4109ae
rust-index: using the `hg::index::Index` in ancestors iterator and lazy set
Since there is no Rust implementation for REVLOGV2/CHANGELOGv2, we declare
them to be incompatible with Rust, hence indexes in these formats will use
the implementations from Python `mercurial.ancestor`. If this is an unacceptable
performance hit for current users of these formats, we can later on add Rust
implementations based on the C index for them or implement these formats for
the Rust indexes.
Among the challenges that we had to meet, we wanted to avoid taking the GIL each
time the inner (vcsgraph) iterator has to call the parents function. This would probably
still be acceptable in terms of performance with `AncestorsIterator`, but not with
`LazyAncestors` nor for the upcoming change in `MissingAncestors`.
Hence we enclose the reference to the index in a `PySharedRef`, leading to more
rigourous checking of mutations, which does pass now that there no logically immutable
methods of `hg::index::Index` that take a mutable reference as input.
| author | Georges Racinet <georges.racinet@octobus.net> |
|---|---|
| date | Fri, 27 Oct 2023 22:11:05 +0200 |
| parents | 633408a0f2e2 |
| children | 59d81768ad6d |
| files | mercurial/cext/revlog.c mercurial/testing/revlog.py rust/hg-cpython/src/ancestors.rs rust/hg-cpython/src/revlog.rs tests/test-rust-ancestor.py tests/test-rust-revlog.py |
| diffstat | 6 files changed, 209 insertions(+), 57 deletions(-) [+] |
line wrap: on
line diff
--- a/mercurial/cext/revlog.c Fri Oct 27 23:29:29 2023 +0200 +++ b/mercurial/cext/revlog.c Fri Oct 27 22:11:05 2023 +0200 @@ -3037,7 +3037,7 @@ self->offsets = NULL; self->nodelen = 20; self->nullentry = NULL; - self->rust_ext_compat = 1; + self->rust_ext_compat = 0; self->format_version = format_v1; if (!PyArg_ParseTupleAndKeywords(args, kwargs, "OO|l", kwlist, @@ -3055,6 +3055,7 @@ } if (self->format_version == format_v1) { + self->rust_ext_compat = 1; self->entry_size = v1_entry_size; } else if (self->format_version == format_v2) { self->entry_size = v2_entry_size;
--- a/mercurial/testing/revlog.py Fri Oct 27 23:29:29 2023 +0200 +++ b/mercurial/testing/revlog.py Fri Oct 27 22:11:05 2023 +0200 @@ -21,17 +21,32 @@ b'\x00\x00\x00\x00\x00\x00\x00\x00\x00' ) +from ..revlogutils.constants import REVLOGV1 + try: from ..cext import parsers as cparsers # pytype: disable=import-error except ImportError: cparsers = None +try: + from ..rustext.revlog import MixedIndex # pytype: disable=import-error +except ImportError: + MixedIndex = None + @unittest.skipIf( cparsers is None, 'The C version of the "parsers" module is not available. It is needed for this test.', ) class RevlogBasedTestBase(unittest.TestCase): - def parseindex(self): - return cparsers.parse_index2(data_non_inlined, False)[0] + def parseindex(self, data=None): + if data is None: + data = data_non_inlined + return cparsers.parse_index2(data, False)[0] + + def parserustindex(self, data=None): + if data is None: + data = data_non_inlined + cindex = self.parseindex(data=data) + return MixedIndex(cindex, data, REVLOGV1)
--- a/rust/hg-cpython/src/ancestors.rs Fri Oct 27 23:29:29 2023 +0200 +++ b/rust/hg-cpython/src/ancestors.rs Fri Oct 27 22:11:05 2023 +0200 @@ -34,15 +34,17 @@ //! [`LazyAncestors`]: struct.LazyAncestors.html //! [`MissingAncestors`]: struct.MissingAncestors.html //! [`AncestorsIterator`]: struct.AncestorsIterator.html -use crate::revlog::pyindex_to_graph; +use crate::revlog::{py_rust_index_to_graph, pyindex_to_graph}; use crate::PyRevision; use crate::{ cindex::Index, conversion::rev_pyiter_collect, exceptions::GraphError, + revlog::PySharedIndex, }; use cpython::{ - ObjectProtocol, PyClone, PyDict, PyList, PyModule, PyObject, PyResult, - Python, PythonObject, ToPyObject, + ObjectProtocol, PyClone, PyDict, PyErr, PyList, PyModule, PyObject, + PyResult, Python, PythonObject, ToPyObject, UnsafePyLeaked, }; + use hg::MissingAncestors as CoreMissing; use hg::Revision; use std::cell::RefCell; @@ -52,11 +54,43 @@ LazyAncestors as VCGLazyAncestors, }; +// Error propagation for an [`UnsafePyLeaked`] wrapping a [`Result`] +// +// It would be nice for UnsharedPyLeaked to provide this directly as a variant +// of the `map` method with a signature such as: +// +// ``` +// unafe fn map_or_err(py: Python, +// f: impl FnOnce(T) -> Result(U, E), +// convert_err: impl FnOnce(Python, E) -> PyErr) +// ``` +// +// This would spare users of the `cpython` crate the additional `unsafe` deref +// to inspect the error and return it outside `UnsafePyLeaked`, and the +// subsequent unwrapping that this function performs. +fn pyleaked_or_map_err<T, E: std::fmt::Debug + Copy>( + py: Python, + leaked: UnsafePyLeaked<Result<T, E>>, + convert_err: impl FnOnce(Python, E) -> PyErr, +) -> PyResult<UnsafePyLeaked<T>> { + // Result.inspect_err is unstable in Rust 1.61 + if let Err(e) = *unsafe { leaked.try_borrow(py)? } { + return Err(convert_err(py, e)); + } + Ok(unsafe { + leaked.map(py, |res| { + res.expect("Error case should have already be treated") + }) + }) +} + py_class!(pub class AncestorsIterator |py| { - data inner: RefCell<Box<VCGAncestorsIterator<Index>>>; + data inner: RefCell<UnsafePyLeaked<VCGAncestorsIterator<PySharedIndex>>>; def __next__(&self) -> PyResult<Option<PyRevision>> { - match self.inner(py).borrow_mut().next() { + let mut leaked = self.inner(py).borrow_mut(); + let mut inner = unsafe { leaked.try_borrow_mut(py)? }; + match inner.next() { Some(Err(e)) => Err(GraphError::pynew_from_vcsgraph(py, e)), None => Ok(None), Some(Ok(r)) => Ok(Some(PyRevision(r))), @@ -64,7 +98,9 @@ } def __contains__(&self, rev: PyRevision) -> PyResult<bool> { - self.inner(py).borrow_mut().contains(rev.0) + let mut leaked = self.inner(py).borrow_mut(); + let mut inner = unsafe { leaked.try_borrow_mut(py)? }; + inner.contains(rev.0) .map_err(|e| GraphError::pynew_from_vcsgraph(py, e)) } @@ -79,16 +115,7 @@ stoprev: PyRevision, inclusive: bool ) -> PyResult<AncestorsIterator> { - let index = pyindex_to_graph(py, index)?; - let initvec: Vec<_> = rev_pyiter_collect(py, &initrevs, &index)?; - let ait = VCGAncestorsIterator::new( - index, - initvec.into_iter().map(|r| r.0), - stoprev.0, - inclusive, - ) - .map_err(|e| GraphError::pynew_from_vcsgraph(py, e))?; - AncestorsIterator::from_inner(py, ait) + Self::inner_new(py, index, initrevs, stoprev, inclusive) } }); @@ -96,28 +123,71 @@ impl AncestorsIterator { pub fn from_inner( py: Python, - ait: VCGAncestorsIterator<Index>, + ait: UnsafePyLeaked<VCGAncestorsIterator<PySharedIndex>>, ) -> PyResult<Self> { - Self::create_instance(py, RefCell::new(Box::new(ait))) + Self::create_instance(py, RefCell::new(ait)) + } + + pub fn inner_new( + py: Python, + index: PyObject, + initrevs: PyObject, + stoprev: PyRevision, + inclusive: bool, + ) -> PyResult<AncestorsIterator> { + let index = py_rust_index_to_graph(py, index)?; + let initvec: Vec<_> = { + let borrowed_idx = unsafe { index.try_borrow(py)? }; + rev_pyiter_collect(py, &initrevs, &*borrowed_idx)? + }; + let res_ait = unsafe { + index.map(py, |idx| { + VCGAncestorsIterator::new( + idx, + initvec.into_iter().map(|r| r.0), + stoprev.0, + inclusive, + ) + }) + }; + let ait = + pyleaked_or_map_err(py, res_ait, GraphError::pynew_from_vcsgraph)?; + AncestorsIterator::from_inner(py, ait) } } py_class!(pub class LazyAncestors |py| { - data inner: RefCell<Box<VCGLazyAncestors<Index>>>; + data inner: RefCell<UnsafePyLeaked< + RefCell<VCGLazyAncestors<PySharedIndex>> + >>; + data index: PyObject; + data initrevs: PyObject; + data stoprev: PyRevision; + data inclusive: bool; def __contains__(&self, rev: PyRevision) -> PyResult<bool> { - self.inner(py) - .borrow_mut() - .contains(rev.0) + let leaked = self.inner(py).borrow(); + let inner: &RefCell<VCGLazyAncestors<PySharedIndex>> = + &*unsafe { leaked.try_borrow(py)? }; + let inner_mut: &mut VCGLazyAncestors<PySharedIndex> = + &mut *inner.borrow_mut(); + inner_mut.contains(rev.0) .map_err(|e| GraphError::pynew_from_vcsgraph(py, e)) } def __iter__(&self) -> PyResult<AncestorsIterator> { - AncestorsIterator::from_inner(py, self.inner(py).borrow().iter()) + let index = self.index(py).clone_ref(py); + let initrevs = self.initrevs(py).clone_ref(py); + AncestorsIterator::inner_new(py, index, initrevs, + *self.stoprev(py), + *self.inclusive(py)) } def __bool__(&self) -> PyResult<bool> { - Ok(!self.inner(py).borrow().is_empty()) + let leaked = self.inner(py).borrow(); + let inner = unsafe { leaked.try_borrow(py)? }; + let empty = inner.borrow().is_empty(); + Ok(!empty) } def __new__( @@ -127,19 +197,27 @@ stoprev: PyRevision, inclusive: bool ) -> PyResult<Self> { - let index = pyindex_to_graph(py, index)?; - let initvec: Vec<_> = rev_pyiter_collect(py, &initrevs, &index)?; + let cloned_index = index.clone_ref(py); + let index = py_rust_index_to_graph(py, index)?; + let initvec: Vec<_> = { + let borrowed_idx = unsafe {index.try_borrow(py)?}; + rev_pyiter_collect(py, &initrevs, &*borrowed_idx)? + }; - let lazy = - VCGLazyAncestors::new( - index, + let res_lazy = + unsafe { index.map(py, |idx| VCGLazyAncestors::new( + idx, initvec.into_iter().map(|r| r.0), stoprev.0, inclusive - ) - .map_err(|e| GraphError::pynew_from_vcsgraph(py, e))?; - - Self::create_instance(py, RefCell::new(Box::new(lazy))) + ))}; + let lazy = pyleaked_or_map_err(py, res_lazy, + GraphError::pynew_from_vcsgraph)?; + let lazy_cell = unsafe { lazy.map(py, RefCell::new)}; + let res = Self::create_instance( + py, RefCell::new(lazy_cell), + cloned_index, initrevs, stoprev, inclusive)?; + Ok(res) } });
--- a/rust/hg-cpython/src/revlog.rs Fri Oct 27 23:29:29 2023 +0200 +++ b/rust/hg-cpython/src/revlog.rs Fri Oct 27 22:11:05 2023 +0200 @@ -16,7 +16,7 @@ exc::{IndexError, ValueError}, ObjectProtocol, PyBool, PyBytes, PyClone, PyDict, PyErr, PyInt, PyList, PyModule, PyObject, PyResult, PySet, PyString, PyTuple, Python, - PythonObject, ToPyObject, + PythonObject, ToPyObject, UnsafePyLeaked, }; use hg::{ errors::HgError, @@ -25,10 +25,11 @@ INDEX_ENTRY_SIZE, }, nodemap::{Block, NodeMapError, NodeTree}, - revlog::{nodemap::NodeMap, NodePrefix, RevlogError, RevlogIndex}, - BaseRevision, Revision, UncheckedRevision, NULL_REVISION, + revlog::{nodemap::NodeMap, Graph, NodePrefix, RevlogError, RevlogIndex}, + BaseRevision, Node, Revision, UncheckedRevision, NULL_REVISION, }; use std::{cell::RefCell, collections::HashMap}; +use vcsgraph::graph::Graph as VCSGraph; /// Return a Struct implementing the Graph trait pub(crate) fn pyindex_to_graph( @@ -41,9 +42,64 @@ } } +pub struct PySharedIndex { + /// The underlying hg-core index + pub(crate) inner: &'static hg::index::Index, +} + +/// Return a Struct implementing the Graph trait +pub(crate) fn py_rust_index_to_graph( + py: Python, + index: PyObject, +) -> PyResult<UnsafePyLeaked<PySharedIndex>> { + let midx = index.extract::<MixedIndex>(py)?; + let leaked = midx.index(py).leak_immutable(); + Ok(unsafe { leaked.map(py, |idx| PySharedIndex { inner: idx }) }) +} + +impl Clone for PySharedIndex { + fn clone(&self) -> Self { + Self { inner: self.inner } + } +} + +impl Graph for PySharedIndex { + fn parents(&self, rev: Revision) -> Result<[Revision; 2], hg::GraphError> { + self.inner.parents(rev) + } +} + +impl VCSGraph for PySharedIndex { + fn parents( + &self, + rev: BaseRevision, + ) -> Result<vcsgraph::graph::Parents, vcsgraph::graph::GraphReadError> + { + // FIXME This trait should be reworked to decide between Revision + // and UncheckedRevision, get better errors names, etc. + match Graph::parents(self, Revision(rev)) { + Ok(parents) => { + Ok(vcsgraph::graph::Parents([parents[0].0, parents[1].0])) + } + Err(hg::GraphError::ParentOutOfRange(rev)) => { + Err(vcsgraph::graph::GraphReadError::KeyedInvalidKey(rev.0)) + } + } + } +} + +impl RevlogIndex for PySharedIndex { + fn len(&self) -> usize { + self.inner.len() + } + fn node(&self, rev: Revision) -> Option<&Node> { + self.inner.node(rev) + } +} + py_class!(pub class MixedIndex |py| { data cindex: RefCell<cindex::Index>; - data index: RefCell<hg::index::Index>; + @shared data index: hg::index::Index; data nt: RefCell<Option<NodeTree>>; data docket: RefCell<Option<PyObject>>; // Holds a reference to the mmap'ed persistent nodemap data @@ -668,17 +724,15 @@ Self::create_instance( py, RefCell::new(cindex::Index::new(py, cindex)?), - RefCell::new( - hg::index::Index::new( - bytes, - IndexHeader::parse(&header.to_be_bytes()) - .expect("default header is broken") - .unwrap(), - ) - .map_err(|e| { - revlog_error_with_msg(py, e.to_string().as_bytes()) - })?, - ), + hg::index::Index::new( + bytes, + IndexHeader::parse(&header.to_be_bytes()) + .expect("default header is broken") + .unwrap(), + ) + .map_err(|e| { + revlog_error_with_msg(py, e.to_string().as_bytes()) + })?, RefCell::new(None), RefCell::new(None), RefCell::new(None),
--- a/tests/test-rust-ancestor.py Fri Oct 27 23:29:29 2023 +0200 +++ b/tests/test-rust-ancestor.py Fri Oct 27 22:11:05 2023 +0200 @@ -50,7 +50,7 @@ """ def testiteratorrevlist(self): - idx = self.parseindex() + idx = self.parserustindex() # checking test assumption about the index binary data: self.assertEqual( {i: (r[5], r[6]) for i, r in enumerate(idx)}, @@ -63,7 +63,7 @@ self.assertEqual([r for r in ait], [2, 1, 0]) def testlazyancestors(self): - idx = self.parseindex() + idx = self.parserustindex() start_count = sys.getrefcount(idx) # should be 2 (see Python doc) self.assertEqual( {i: (r[5], r[6]) for i, r in enumerate(idx)}, @@ -110,7 +110,7 @@ self.assertEqual(revs, {2, 3}) def testrefcount(self): - idx = self.parseindex() + idx = self.parserustindex() start_count = sys.getrefcount(idx) # refcount increases upon iterator init... @@ -127,13 +127,17 @@ del idx self.assertEqual(list(ait), [3, 2, 1, 0]) + # the index is not tracked by the GC, hence there is nothing more + # we can assert to check that it is properly deleted once its refcount + # drops to 0 + def testgrapherror(self): data = ( revlogtesting.data_non_inlined[: 64 + 27] + b'\xf2' + revlogtesting.data_non_inlined[64 + 28 :] ) - idx = cparsers.parse_index2(data, False)[0] + idx = self.parserustindex(data=data) with self.assertRaises(rustext.GraphError) as arc: AncestorsIterator(idx, [1], -1, False) exc = arc.exception @@ -143,7 +147,7 @@ def testwdirunsupported(self): # trying to access ancestors of the working directory raises - idx = self.parseindex() + idx = self.parserustindex() with self.assertRaises(rustext.GraphError) as arc: list(AncestorsIterator(idx, [wdirrev], -1, False))
--- a/tests/test-rust-revlog.py Fri Oct 27 23:29:29 2023 +0200 +++ b/tests/test-rust-revlog.py Fri Oct 27 22:11:05 2023 +0200 @@ -54,7 +54,7 @@ self.assertEqual(list(lazy), [3, 2, 1, 0]) # let's check bool for an empty one - self.assertFalse(LazyAncestors(idx, [0], 0, False)) + self.assertFalse(LazyAncestors(rustidx, [0], 0, False)) if __name__ == '__main__':