This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6394

rust-dirstate: add "dirs" rust-cpython binding
ClosedPublic

Authored by Alphare on May 17 2019, 6:10 AM.

Details

Reviewers
kevincox
Group Reviewers
hg-reviewers
Commits
rHGce94f9622acd: rust-dirstate: add "dirs" rust-cpython binding
Summary

There is an obvious performance and memory issue with those bindings on larger
repos as it copies and allocates everything at once, round-trip. Like in the
previous patch series, this is only temporary and will only get better once
we don't have large data structures going to and from Python.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Alphare created this revision.May 17 2019, 6:10 AM
Alphare updated this revision to Diff 15342.Jun 5 2019, 12:23 PM
kevincox accepted this revision.Jun 10 2019, 9:06 AM
kevincox added inline comments.
rust/hg-cpython/src/dirstate.rs
218
let dirs_map = if ...

Then just let the if statement evaluate to this value.

Alphare added inline comments.Jun 17 2019, 11:36 AM
rust/hg-cpython/src/dirstate.rs
218

Ah yes, I always forget... Done in a future patch.

Alphare updated this revision to Diff 15671.Jun 27 2019, 9:16 AM
Alphare added a commit: rHGce94f9622acd: rust-dirstate: add "dirs" rust-cpython binding.Jun 28 2019, 11:14 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGce94f9622acd: rust-dirstate: add "dirs" rust-cpython binding (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Jun 29 2019, 11:41 PM

+ def contains(&self, item: PyObject) -> PyResult<bool> {
+ Ok(self
+ .dirs_map(py)
+ .borrow()
+ .get(&item.extract::<PyBytes>(py)?.data(py).to_owned())
+ .is_some())

.contains_key(..) and .as_ref() instead of &...to_owned().

I'm surprised by the use of Deref in the previous patch. Is it legit
to leverage Deref to expose the inner type?

Alphare added a comment.Jul 1 2019, 4:46 AM
In D6394#96115, @yuja wrote:

+ def contains(&self, item: PyObject) -> PyResult<bool> {
+ Ok(self
+ .dirs_map(py)
+ .borrow()
+ .get(&item.extract::<PyBytes>(py)?.data(py).to_owned())
+ .is_some())

.contains_key(..) and .as_ref() instead of &...to_owned().
I'm surprised by the use of Deref in the previous patch. Is it legit
to leverage Deref to expose the inner type?

Ah yes, I don't know what I was thinking. I'll write a follow up.
To my eyes, the point of Deref is to make the struct act as a "fat pointer" from the outer interface code, but maybe this does not really help that much?

yuja added a comment.Jul 1 2019, 8:42 AM
To my eyes, the point of `Deref` is to make the `struct` act as a "fat pointer" from the outer interface code, but maybe this does not really help that much?

I feel DirsMultiset isn't a HashMap, but is implemented by using a HashMap.
For example, inner.get().unwrap() returns a refcount of the path, but which
is an implementation detail.

So I think it's better to define methods explicitly instead of proxying
everything to HashMap.

Alphare added a comment.Jul 1 2019, 8:47 AM
In D6394#96189, @yuja wrote:
To my eyes, the point of `Deref` is to make the `struct` act as a "fat pointer" from the outer interface code, but maybe this does not really help that much?

I feel DirsMultiset isn't a HashMap, but is implemented by using a HashMap.
For example, inner.get().unwrap() returns a refcount of the path, but which
is an implementation detail.
So I think it's better to define methods explicitly instead of proxying
everything to HashMap.

I see your point. I'll follow up later today.

Revision Contents
Changeset List

PathPackages
Mrust/hg-cpython/src/dirstate.rs (143 lines)

Diff 15161

rust/hg-cpython/src/dirstate.rs

// dirstate.rs // dirstate.rs
// //
// Copyright 2019 Raphaël Gomès <rgomes@octobus.net> // Copyright 2019 Raphaël Gomès <rgomes@octobus.net>
// //
// 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::dirstate` module provided by the //! Bindings for the `hg::dirstate` module provided by the
//! `hg-core` package. //! `hg-core` package.
//! //!
//! From Python, this will be seen as `mercurial.rustext.dirstate` //! From Python, this will be seen as `mercurial.rustext.dirstate`
use cpython::{ use cpython::{
exc, PyBytes, PyDict, PyErr, PyInt, PyModule, PyObject, PyResult, exc, ObjectProtocol, PyBytes, PyDict, PyErr, PyInt, PyModule, PyObject,
PySequence, PyTuple, Python, ToPyObject, PyResult, PySequence, PyTuple, Python, ToPyObject,
}; };
use hg::{ use hg::{
pack_dirstate, parse_dirstate, CopyVecEntry, DirstateEntry, pack_dirstate, parse_dirstate, CopyVecEntry, DirsIterable, DirsMultiset,
DirstatePackError, DirstateParents, DirstateParseError, DirstateVec, DirstateEntry, DirstateMapError, DirstatePackError, DirstateParents,
DirstateParseError, DirstateVec,
}; };
use std::collections::HashMap; use std::collections::HashMap;
use std::ffi::CStr; use std::ffi::CStr;
#[cfg(feature = "python27")] #[cfg(feature = "python27")]
extern crate python27_sys as python_sys; extern crate python27_sys as python_sys;
#[cfg(feature = "python3")] #[cfg(feature = "python3")]
extern crate python3_sys as python_sys; extern crate python3_sys as python_sys;
use self::python_sys::PyCapsule_Import; use self::python_sys::PyCapsule_Import;
use libc::{c_char, c_int}; use libc::{c_char, c_int};
use std::cell::RefCell;
use std::mem::transmute; use std::mem::transmute;
/// C code uses a custom `dirstate_tuple` type, checks in multiple instances /// C code uses a custom `dirstate_tuple` type, checks in multiple instances
/// for this type, and raises a Python `Exception` if the check does not pass. /// for this type, and raises a Python `Exception` if the check does not pass.
/// Because this type differs only in name from the regular Python tuple, it /// Because this type differs only in name from the regular Python tuple, it
/// would be a good idea in the near future to remove it entirely to allow /// would be a good idea in the near future to remove it entirely to allow
/// for a pure Python tuple of the same effective structure to be used, /// for a pure Python tuple of the same effective structure to be used,
/// rendering this type and the capsule below useless. /// rendering this type and the capsule below useless.
"overflow in dirstate".to_string() "overflow in dirstate".to_string()
} }
DirstateParseError::CorruptedEntry(e) => e, DirstateParseError::CorruptedEntry(e) => e,
}, },
)), )),
} }
} }
fn pack_dirstate_wrapper( fn extract_dirstate_vec(
py: Python, py: Python,
dmap: PyDict, dmap: &PyDict,
copymap: PyDict, ) -> Result<DirstateVec, PyErr> {
pl: PyTuple, dmap.items(py)
now: PyInt,
) -> PyResult<PyBytes> {
let p1 = pl.get_item(py, 0).extract::<PyBytes>(py)?;
let p1: &[u8] = p1.data(py);
let p2 = pl.get_item(py, 1).extract::<PyBytes>(py)?;
let p2: &[u8] = p2.data(py);
let dirstate_vec: Result<DirstateVec, PyErr> = dmap
.items(py)
.iter() .iter()
.map(|(filename, stats)| { .map(|(filename, stats)| {
let stats = stats.extract::<PySequence>(py)?; let stats = stats.extract::<PySequence>(py)?;
let state = stats.get_item(py, 0)?.extract::<PyBytes>(py)?; let state = stats.get_item(py, 0)?.extract::<PyBytes>(py)?;
let state = state.data(py)[0] as i8; let state = state.data(py)[0] as i8;
let mode = stats.get_item(py, 1)?.extract(py)?; let mode = stats.get_item(py, 1)?.extract(py)?;
let size = stats.get_item(py, 2)?.extract(py)?; let size = stats.get_item(py, 2)?.extract(py)?;
let mtime = stats.get_item(py, 3)?.extract(py)?; let mtime = stats.get_item(py, 3)?.extract(py)?;
let filename = filename.extract::<PyBytes>(py)?; let filename = filename.extract::<PyBytes>(py)?;
let filename = filename.data(py); let filename = filename.data(py);
Ok(( Ok((
filename.to_owned(), filename.to_owned(),
DirstateEntry { DirstateEntry {
state, state,
mode, mode,
size, size,
mtime, mtime,
}, },
)) ))
}) })
.collect(); .collect()
}
fn pack_dirstate_wrapper(
py: Python,
dmap: PyDict,
copymap: PyDict,
pl: PyTuple,
now: PyInt,
) -> PyResult<PyBytes> {
let p1 = pl.get_item(py, 0).extract::<PyBytes>(py)?;
let p1: &[u8] = p1.data(py);
let p2 = pl.get_item(py, 1).extract::<PyBytes>(py)?;
let p2: &[u8] = p2.data(py);
let dirstate_vec = extract_dirstate_vec(py, &dmap)?;
let copies: Result<HashMap<Vec<u8>, Vec<u8>>, PyErr> = copymap let copies: Result<HashMap<Vec<u8>, Vec<u8>>, PyErr> = copymap
.items(py) .items(py)
.iter() .iter()
.map(|(key, value)| { .map(|(key, value)| {
Ok(( Ok((
key.extract::<PyBytes>(py)?.data(py).to_owned(), key.extract::<PyBytes>(py)?.data(py).to_owned(),
value.extract::<PyBytes>(py)?.data(py).to_owned(), value.extract::<PyBytes>(py)?.data(py).to_owned(),
)) ))
}) })
.collect(); .collect();
match pack_dirstate( match pack_dirstate(
&dirstate_vec?, &dirstate_vec,
&copies?, &copies?,
DirstateParents { p1, p2 }, DirstateParents { p1, p2 },
now.value(py) as i32, now.value(py) as i32,
) { ) {
Ok((packed, new_dirstate_vec)) => { Ok((packed, new_dirstate_vec)) => {
for ( for (
filename, filename,
DirstateEntry { DirstateEntry {
DirstatePackError::BadSize(expected, actual) => { DirstatePackError::BadSize(expected, actual) => {
format!("bad dirstate size: {} != {}", actual, expected) format!("bad dirstate size: {} != {}", actual, expected)
} }
}, },
)), )),
} }
} }
py_class!(pub class Dirs |py| {
data dirs_map: RefCell<DirsMultiset>;
// `map` is either a `dict` or a flat iterator (usually a `set`, sometimes
// a `list`)
def __new__(
_cls,
map: PyObject,
skip: Option<PyObject> = None
) -> PyResult<Self> {
let mut skip_state: Option<i8> = None;
if let Some(skip) = skip {
skip_state = Some(skip.extract::<PyBytes>(py)?.data(py)[0] as i8);
}
let dirs_map;
kevincoxUnsubmitted
Not Done
let dirs_map = if ...

Then just let the if statement evaluate to this value.

kevincox: ``` let dirs_map = if ... ``` Then just let the if statement evaluate to this value.
AlphareAuthorUnsubmitted
Done

Ah yes, I always forget... Done in a future patch.

Alphare: Ah yes, I always forget... Done in a future patch.
if let Ok(map) = map.cast_as::<PyDict>(py) {
let dirstate_vec = extract_dirstate_vec(py, &map)?;
dirs_map = DirsMultiset::new(
DirsIterable::Dirstate(dirstate_vec),
skip_state,
)
} else {
let map: Result<Vec<Vec<u8>>, PyErr> = map
.iter(py)?
.map(|o| Ok(o?.extract::<PyBytes>(py)?.data(py).to_owned()))
.collect();
dirs_map = DirsMultiset::new(
DirsIterable::Manifest(map?),
skip_state,
)
}
Self::create_instance(py, RefCell::new(dirs_map))
}
def addpath(&self, path: PyObject) -> PyResult<PyObject> {
self.dirs_map(py).borrow_mut().add_path(
path.extract::<PyBytes>(py)?.data(py).to_owned(),
);
Ok(py.None())
}
def delpath(&self, path: PyObject) -> PyResult<PyObject> {
self.dirs_map(py).borrow_mut().delete_path(
path.extract::<PyBytes>(py)?.data(py).to_owned(),
)
.and(Ok(py.None()))
.or_else(|e| {
match e {
DirstateMapError::PathNotFound(_p) => {
Err(PyErr::new::<exc::ValueError, _>(
py,
"expected a value, found none".to_string(),
))
}
DirstateMapError::EmptyPath => {
Ok(py.None())
}
}
})
}
// This is really inefficient on top of being ugly, but it's an easy way
// of having it work to continue working on the rest of the module
// hopefully bypassing Python entirely pretty soon.
def __iter__(&self) -> PyResult<PyObject> {
let dict = PyDict::new(py);
for (key, value) in self.dirs_map(py).borrow().iter() {
dict.set_item(
py,
PyBytes::new(py, &key[..]),
value.to_py_object(py),
)?;
}
let locals = PyDict::new(py);
locals.set_item(py, "obj", dict)?;
py.eval("iter(obj)", None, Some(&locals))
}
def __contains__(&self, item: PyObject) -> PyResult<bool> {
Ok(self
.dirs_map(py)
.borrow()
.get(&item.extract::<PyBytes>(py)?.data(py).to_owned())
.is_some())
}
});
/// 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!("{}.dirstate", package); let dotted_name = &format!("{}.dirstate", 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(py, "__doc__", "Dirstate - Rust implementation")?; m.add(py, "__doc__", "Dirstate - Rust implementation")?;
m.add( m.add(
py, py,
"parse_dirstate", "parse_dirstate",
py_fn!( py_fn!(
py, py,
parse_dirstate_wrapper(dmap: PyDict, copymap: PyDict, st: PyBytes) parse_dirstate_wrapper(dmap: PyDict, copymap: PyDict, st: PyBytes)
), ),
)?; )?;
m.add( m.add(
py, py,
"pack_dirstate", "pack_dirstate",
py_fn!( py_fn!(
py, py,
pack_dirstate_wrapper( pack_dirstate_wrapper(
dmap: PyDict, dmap: PyDict,
copymap: PyDict, copymap: PyDict,
pl: PyTuple, pl: PyTuple,
now: PyInt now: PyInt
) )
), ),
)?; )?;
m.add_class::<Dirs>(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)?;
Ok(m) Ok(m)
} }