( )⚙ D5441 rust-cpython: binding for LazyAncestors

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5441

rust-cpython: binding for LazyAncestors
ClosedPublic

Authored by gracinet on Dec 15 2018, 6:42 AM.

Details

Reviewers
kevincox
Group Reviewers
hg-reviewers
Commits
rHGb31a41f24864: rust-cpython: binding for LazyAncestors
Summary

The mercurial.rustext.ancestor module will not in the foreseeable
future be a drop-in replacement for the pure mercurial.ancestor, because the
Rust variants take the index at instantiation whereas the Python ones
take a parents function. From the Python side, using the index from ancestor
would leak internal details out of mercurial.revlog, and that's unwanted.
Therefore, given that classes defined in
rust-cpython have the same names in both language, we keep the Rust naming
convention (CamelCase).

Eventually, though, the ancestor module can be placed under control of
mercurial.policy, but it will still be up to revlog to be aware of
that and play the role of a factory for instantiation.

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

gracinet created this revision.Dec 15 2018, 6:42 AM
kevincox accepted this revision.Dec 15 2018, 8:14 AM
kevincox added inline comments.
rust/hg-cpython/src/ancestors.rs
83

It should be fine with tbe GIL. RefCell is basically a single-thread lock. In this case it should be fine to have no renterance.

tests/test-rust-ancestor.py
93

list(lazy)

yuja added a subscriber: yuja.Dec 15 2018, 11:45 PM
m.add_class::<AncestorsIterator>(py)?;

+ m.add_class::<lazyancestors>(py)?;

Nit: While it's correct per our naming convention, I prefer calling it
as LazyAncestors in Rust, and export as lazyancestors.

gracinet updated this revision to Diff 12952.Dec 22 2018, 11:24 AM
kevincox accepted this revision.Dec 22 2018, 11:57 AM
kevincox added inline comments.
rust/hg-cpython/src/ancestors.rs
109
.map_err(|e| GraphError::new(py, format!("{:?}", e)))

Or even better is to implement Into so that you can use ?

gracinet added inline comments.Dec 22 2018, 2:48 PM
rust/hg-cpython/src/ancestors.rs
83

Yes, I'll remove these comments, same with the ones for RW lock, it's pretty clear to me now that such would be relevant only for code meant to be executed in a allow_threads closure.

109

Ah yes, forgot to upgrade that one, thanks. The current good spelling would be the one used in __contains__, where GraphError::pynew encloses the conversion details.

I believe implementing Into is not readily possible with the rust-cpython way of creating Python exceptions : we can't do it without the py marker (it's actually listed as one of the advantages of PyO3). And since Python<'p> is not defined by our crate, we'd need to enclose in a wrapper in the excepions module:

pub struct ExceptionConverter<'p>(pub Python<'p>, pub hg::GraphError);

impl <'p> From<ExceptionConverter<'p>> for PyErr {

    fn from(ec: ExceptionConverter<'p>) -> Self {
        GraphError::pynew(ec.0, ec.1)
    }
}

and then

def __new__(_cls, index: PyObject, initrevs: PyObject, stoprev: Revision,
            inclusive: bool) -> PyResult<Self> {
    let initvec = reviter_to_revvec(py, initrevs)?;

    let lazy = CoreLazy::new(
        Index::new(py, index)?, initvec, stoprev, inclusive).map_err(|e| ExceptionConverter(py, e))?;
    Self::create_instance(py, RefCell::new(Box::new(lazy)))
    }

This does compile (did not think hard about the namings), and I'd be surprised if it didn't the right thng, but that's a lot of effort for a result that's only a bit better. What do you think ?

yuja added a comment.Dec 23 2018, 11:57 PM

+/ The purpose of this module is to hide identifiers from other Rust users
+/
+/ Some of the identifiers we are defining are meant for consumption
+/ from Python with other naming conventions. For instance, py_class!
+/ does not let us make a distinction between the Python and the Rust name,
+/ and that name ends up in particular in the __class__ attribute, so that
+/ renaming it inside its Python module is not enough.
+/
+/ For Rust consumers, we will reexport these definitions, following the
+/ appropriate naming convention.
+mod concealed_detail {
+ use super::*;
+
+ py_class!(pub class lazyancestors |py| {

Does the class name actually matter? Personally I don't care if
lazyancestors() function returns a LazyAncestors object. We'll anyway
need a wrapper function to make pure ancestors and rustext ancestors
compatible.

gracinet marked 2 inline comments as done.Dec 27 2018, 2:04 PM

Does the class name actually matter? Personally I don't care if
lazyancestors() function returns a LazyAncestors object. We'll anyway
need a wrapper function to make pure ancestors and rustext ancestors
compatible.

Yes, that's in line with your other comments, whereas I was pursuing the
goal of putting the whole ancestor module under responsibility of
policy.importmod : in that case, it would have been necessary to have
the same name.

I think I still prefer to put all the dispatching in the revlog module
(ie have it play the factory role) rather than put a factory function
inside ancestor. Unless you disagree, I'm going to make a new revision
going in that direction, ie

  • the class would be called LazyAncestors
  • the dispatching would stay in revlog
  • maybe for clarity, ancestor.rustlazyancestors should be renamed

and in the next series (bindings for MissingAncestors), similar
dispatching would occur in incrementalmissingrevs

For now, revlog can choose according to whether rustext is (fully)
importable (some other comment about that).

Cheers,

yuja added a comment.Dec 28 2018, 12:46 AM
> Does the class name actually matter? Personally I don't care if
>  lazyancestors() function returns a LazyAncestors object. We'll anyway
>  need a wrapper function to make pure ancestors and rustext ancestors
>  compatible.
Yes, that's in line with your other comments, whereas I was pursuing the
goal of putting the whole `ancestor` module under responsibility of
`policy.importmod` : in that case, it would have been necessary to have
the same name.

I was wondering if it would be possible to export a module attribute in all
lower-caps without renaming the type. (i.e. lazyancestors = LazyAncestors.)

Anyway, I just thought using CamelCase in Rust would be slightly nicer and
simpler. If that makes things complicated, I'll drop my idea and stick to
anything that is simpler.

I think I still prefer to put all the dispatching in the `revlog` module
(ie have it play the factory role) rather than put a factory function
inside `ancestor`. Unless you disagree, I'm going to make a new revision
going in that direction, ie
- the class would be called `LazyAncestors`
- the dispatching would stay in `revlog`

Both sound good.

  • maybe for clarity, ancestor.rustlazyancestors should be renamed

I think rustlazyancestors will be removed in favor of rust-cpython
implementation.

kevincox added a comment.Jan 3 2019, 10:24 AM
In D5441#81222, @yuja wrote:

Anyway, I just thought using CamelCase in Rust would be slightly nicer and
simpler. If that makes things complicated, I'll drop my idea and stick to
anything that is simpler.

I think using the Rust naming convention in Rust and the Python convention in Python is probably the best. It does make it slightly harder to grep the codebase for these things but since you would find the explicit registration linking the two names I don't think it is a major issue.

I believe implementing Into is not readily possible with the rust-cpython way of creating Python exceptions : we can't do it without the py marker

Ah right, I missed this. In this case I think the manual conversion is fine.

gracinet marked an inline comment as done.Jan 4 2019, 1:30 PM
gracinet edited the summary of this revision. (Show Details)
gracinet updated this revision to Diff 13007.
gracinet added a comment.Jan 4 2019, 1:30 PM

@yuja this new version exports the LazyAncestors name to Python, expecting revlog.ancestors to do the right thing (as a factory then). I agree that the destiny of the older rustlazyancestors is to disappear, but I'd prefer to wait a bit before doing that
@kevincox I'd agree in principle, but since we only have one caller, whose role is precisely to abstract those differencies, we decided that we actually don't need to maintain that convention (and rustext can ne considered a foreign module, so that this is not really a breaking of the naming convention for new classes within hg)

Closed by commit rHGb31a41f24864: rust-cpython: binding for LazyAncestors (authored by gracinet). · Explain WhyJan 5 2019, 8:55 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 13014

rust/hg-cpython/src/ancestors.rs

// ancestors.rs // ancestors.rs
// //
// Copyright 2018 Georges Racinet <gracinet@anybox.fr> // Copyright 2018 Georges Racinet <gracinet@anybox.fr>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
//! Bindings for the hg::ancestors module provided by the //! Bindings for the hg::ancestors module provided by the
//! `hg-core` crate. From Python, this will be seen as `rustext.ancestor` //! `hg-core` crate. From Python, this will be seen as `rustext.ancestor`
use cindex::Index; use cindex::Index;
use cpython::{ use cpython::{
ObjectProtocol, PyClone, PyDict, PyModule, PyObject, PyResult, Python, ObjectProtocol, PyClone, PyDict, PyModule, PyObject, PyResult, Python,
}; };
use exceptions::GraphError; use exceptions::GraphError;
use hg; use hg;
use hg::AncestorsIterator as CoreIterator;
use hg::Revision; use hg::Revision;
use hg::{AncestorsIterator as CoreIterator, LazyAncestors as CoreLazy};
use std::cell::RefCell; use std::cell::RefCell;
/// Utility function to convert a Python iterable into a Vec<Revision> /// Utility function to convert a Python iterable into a Vec<Revision>
/// ///
/// We need this to feed to AncestorIterators constructors because /// We need this to feed to AncestorIterators constructors because
/// a PyErr can arise at each step of iteration, whereas our inner objects /// a PyErr can arise at each step of iteration, whereas our inner objects
/// expect iterables over Revision, not over some Result<Revision, PyErr> /// expect iterables over Revision, not over some Result<Revision, PyErr>
fn reviter_to_revvec(py: Python, revs: PyObject) -> PyResult<Vec<Revision>> { fn reviter_to_revvec(py: Python, revs: PyObject) -> PyResult<Vec<Revision>> {
}); });
impl AncestorsIterator { impl AncestorsIterator {
pub fn from_inner(py: Python, ait: CoreIterator<Index>) -> PyResult<Self> { pub fn from_inner(py: Python, ait: CoreIterator<Index>) -> PyResult<Self> {
Self::create_instance(py, RefCell::new(Box::new(ait))) Self::create_instance(py, RefCell::new(Box::new(ait)))
} }
} }
py_class!(class LazyAncestors |py| {
data inner: RefCell<Box<CoreLazy<Index>>>;
def __contains__(&self, rev: Revision) -> PyResult<bool> {
self.inner(py)
.borrow_mut()
.contains(rev)
.map_err(|e| GraphError::pynew(py, e))
}
def __iter__(&self) -> PyResult<AncestorsIterator> {
kevincoxUnsubmitted
Not Done

It should be fine with tbe GIL. RefCell is basically a single-thread lock. In this case it should be fine to have no renterance.

kevincox: It should be fine with tbe GIL. RefCell is basically a single-thread lock. In this case it…
gracinetAuthorUnsubmitted
Not Done

Yes, I'll remove these comments, same with the ones for RW lock, it's pretty clear to me now that such would be relevant only for code meant to be executed in a allow_threads closure.

gracinet: Yes, I'll remove these comments, same with the ones for RW lock, it's pretty clear to me now…
AncestorsIterator::from_inner(py, self.inner(py).borrow().iter())
}
def __bool__(&self) -> PyResult<bool> {
Ok(!self.inner(py).borrow().is_empty())
}
def __new__(_cls, index: PyObject, initrevs: PyObject, stoprev: Revision,
inclusive: bool) -> PyResult<Self> {
let initvec = reviter_to_revvec(py, initrevs)?;
let lazy =
CoreLazy::new(Index::new(py, index)?, initvec, stoprev, inclusive)
.map_err(|e| GraphError::pynew(py, e))?;
Self::create_instance(py, RefCell::new(Box::new(lazy)))
}
});
/// Create the module, with __package__ given from parent /// Create the module, with __package__ given from parent
pub fn init_module(py: Python, package: &str) -> PyResult<PyModule> { pub fn init_module(py: Python, package: &str) -> PyResult<PyModule> {
let dotted_name = &format!("{}.ancestor", package); let dotted_name = &format!("{}.ancestor", package);
let m = PyModule::new(py, dotted_name)?; let m = PyModule::new(py, dotted_name)?;
m.add(py, "__package__", package)?; m.add(py, "__package__", package)?;
m.add( m.add(
kevincoxUnsubmitted
Done
.map_err(|e| GraphError::new(py, format!("{:?}", e)))

Or even better is to implement Into so that you can use ?

kevincox: ``` .map_err(|e| GraphError::new(py, format!("{:?}", e))) ``` Or even better is to implement…
gracinetAuthorUnsubmitted
Done

Ah yes, forgot to upgrade that one, thanks. The current good spelling would be the one used in __contains__, where GraphError::pynew encloses the conversion details.

I believe implementing Into is not readily possible with the rust-cpython way of creating Python exceptions : we can't do it without the py marker (it's actually listed as one of the advantages of PyO3). And since Python<'p> is not defined by our crate, we'd need to enclose in a wrapper in the excepions module:

pub struct ExceptionConverter<'p>(pub Python<'p>, pub hg::GraphError);

impl <'p> From<ExceptionConverter<'p>> for PyErr {

    fn from(ec: ExceptionConverter<'p>) -> Self {
        GraphError::pynew(ec.0, ec.1)
    }
}

and then

def __new__(_cls, index: PyObject, initrevs: PyObject, stoprev: Revision,
            inclusive: bool) -> PyResult<Self> {
    let initvec = reviter_to_revvec(py, initrevs)?;

    let lazy = CoreLazy::new(
        Index::new(py, index)?, initvec, stoprev, inclusive).map_err(|e| ExceptionConverter(py, e))?;
    Self::create_instance(py, RefCell::new(Box::new(lazy)))
    }

This does compile (did not think hard about the namings), and I'd be surprised if it didn't the right thng, but that's a lot of effort for a result that's only a bit better. What do you think ?

gracinet: Ah yes, forgot to upgrade that one, thanks. The current good spelling would be the one used in…
py, py,
"__doc__", "__doc__",
"Generic DAG ancestor algorithms - Rust implementation", "Generic DAG ancestor algorithms - Rust implementation",
)?; )?;
m.add_class::<AncestorsIterator>(py)?; m.add_class::<AncestorsIterator>(py)?;
m.add_class::<LazyAncestors>(py)?;
let sys = PyModule::import(py, "sys")?; let sys = PyModule::import(py, "sys")?;
let sys_modules: PyDict = sys.get(py, "modules")?.extract(py)?; let sys_modules: PyDict = sys.get(py, "modules")?.extract(py)?;
sys_modules.set_item(py, dotted_name, &m)?; sys_modules.set_item(py, dotted_name, &m)?;
// Example C code (see pyexpat.c and import.c) will "give away the // Example C code (see pyexpat.c and import.c) will "give away the
// reference", but we won't because it will be consumed once the // reference", but we won't because it will be consumed once the
// Rust PyObject is dropped. // Rust PyObject is dropped.
Ok(m) Ok(m)
} }

tests/test-rust-ancestor.py

from __future__ import absolute_import from __future__ import absolute_import
import sys import sys
import unittest import unittest
try: try:
from mercurial import rustext from mercurial import rustext
rustext.__name__ # trigger immediate actual import rustext.__name__ # trigger immediate actual import
except ImportError: except ImportError:
rustext = None rustext = None
else: else:
# this would fail already without appropriate ancestor.__package__ # this would fail already without appropriate ancestor.__package__
from mercurial.rustext.ancestor import AncestorsIterator from mercurial.rustext.ancestor import (
AncestorsIterator,
LazyAncestors
)
try: try:
from mercurial.cext import parsers as cparsers from mercurial.cext import parsers as cparsers
except ImportError: except ImportError:
cparsers = None cparsers = None
# picked from test-parse-index2, copied rather than imported # picked from test-parse-index2, copied rather than imported
# so that it stays stable even if test-parse-index2 changes or disappears. # so that it stays stable even if test-parse-index2 changes or disappears.
2: (1, -1), 2: (1, -1),
3: (2, -1)}) 3: (2, -1)})
ait = AncestorsIterator(idx, [3], 0, True) ait = AncestorsIterator(idx, [3], 0, True)
self.assertEqual([r for r in ait], [3, 2, 1, 0]) self.assertEqual([r for r in ait], [3, 2, 1, 0])
ait = AncestorsIterator(idx, [3], 0, False) ait = AncestorsIterator(idx, [3], 0, False)
self.assertEqual([r for r in ait], [2, 1, 0]) self.assertEqual([r for r in ait], [2, 1, 0])
def testlazyancestors(self):
idx = self.parseindex()
start_count = sys.getrefcount(idx) # should be 2 (see Python doc)
self.assertEqual({i: (r[5], r[6]) for i, r in enumerate(idx)},
{0: (-1, -1),
1: (0, -1),
2: (1, -1),
3: (2, -1)})
lazy = LazyAncestors(idx, [3], 0, True)
# we have two more references to the index:
# - in its inner iterator for __contains__ and __bool__
# - in the LazyAncestors instance itself (to spawn new iterators)
self.assertEqual(sys.getrefcount(idx), start_count + 2)
self.assertTrue(2 in lazy)
self.assertTrue(bool(lazy))
self.assertEqual(list(lazy), [3, 2, 1, 0])
kevincoxUnsubmitted
Done

list(lazy)

kevincox: `list(lazy)`
# a second time to validate that we spawn new iterators
self.assertEqual(list(lazy), [3, 2, 1, 0])
# now let's watch the refcounts closer
ait = iter(lazy)
self.assertEqual(sys.getrefcount(idx), start_count + 3)
del ait
self.assertEqual(sys.getrefcount(idx), start_count + 2)
del lazy
self.assertEqual(sys.getrefcount(idx), start_count)
# let's check bool for an empty one
self.assertFalse(LazyAncestors(idx, [0], 0, False))
def testrefcount(self): def testrefcount(self):
idx = self.parseindex() idx = self.parseindex()
start_count = sys.getrefcount(idx) start_count = sys.getrefcount(idx)
# refcount increases upon iterator init... # refcount increases upon iterator init...
ait = AncestorsIterator(idx, [3], 0, True) ait = AncestorsIterator(idx, [3], 0, True)
self.assertEqual(sys.getrefcount(idx), start_count + 1) self.assertEqual(sys.getrefcount(idx), start_count + 1)
self.assertEqual(next(ait), 3) self.assertEqual(next(ait), 3)
# and decreases once the iterator is removed # and decreases once the iterator is removed
del ait del ait
self.assertEqual(sys.getrefcount(idx), start_count) self.assertEqual(sys.getrefcount(idx), start_count)
# and removing ref to the index after iterator init is no issue # and removing ref to the index after iterator init is no issue
ait = AncestorsIterator(idx, [3], 0, True) ait = AncestorsIterator(idx, [3], 0, True)
del idx del idx
self.assertEqual([r for r in ait], [3, 2, 1, 0]) self.assertEqual(list(ait), [3, 2, 1, 0])
def testgrapherror(self): def testgrapherror(self):
data = (data_non_inlined[:64 + 27] + data = (data_non_inlined[:64 + 27] +
b'\xf2' + b'\xf2' +
data_non_inlined[64 + 28:]) data_non_inlined[64 + 28:])
idx = cparsers.parse_index2(data, False)[0] idx = cparsers.parse_index2(data, False)[0]
with self.assertRaises(rustext.GraphError) as arc: with self.assertRaises(rustext.GraphError) as arc:
AncestorsIterator(idx, [1], -1, False) AncestorsIterator(idx, [1], -1, False)