This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7543

revlog: made C Capsule an array of function pointers
ClosedPublic

Authored by gracinet on Dec 2 2019, 9:31 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHGf384d68d8ea8: revlog: made C Capsule an array of function pointers
Summary

Although it's perfectly valid to put a function pointer in
a capsule, as we've been doing since the start of rust/hg-cpython,
an array of function pointers has several advantages:

  • it can hold several functions. That's our main motivation here. We plan to expose index_length() and index_node(), which will be needed for a Rust implementation of nodemap.
  • it could also have data
  • (probably minor in the case of Mercurial) proper support for architectures for which data and code pointers don't have the same size.

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 2 2019, 9:31 AM
indygreg accepted this revision.Dec 23 2019, 12:53 PM
indygreg added a subscriber: indygreg.

I was going to say this requires a bump of the module version. But for some reason, we don't track an explicit version of this C extension. Weird.

This revision is now accepted and ready to land.Dec 23 2019, 12:53 PM
gracinet added a commit: rHGf384d68d8ea8: revlog: made C Capsule an array of function pointers.Dec 23 2019, 12:55 PM
Closed by commit rHGf384d68d8ea8: revlog: made C Capsule an array of function pointers (authored by gracinet). · Explain Why
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Jan 4 2020, 4:35 AM

+typedef struct {
+ int (*index_parents)(PyObject *, int, int *);
+} Revlog_CAPI;

Do we have any plan to detect C-Rust ABI mismatch? Renaming the symbol?

gracinet added a comment.Jan 4 2020, 5:42 AM

@yuja for now I'm indeed implicitely assuming both were built from the same source tree, but I understand your concern.
A possibility, since we're now on this data + functions capsule would be to introduce an ABI version number in the capsule.
At least, that's the first thing that comes to mind.

If that check were in Index::new(), I'm confident it wouldn't have any performance impact.
What kind of error feedback could we be sending, then ?

yuja added a comment.Jan 4 2020, 8:04 AM
A possibility, since we're now on this data + functions capsule would be to introduce an ABI version number in the capsule.
At least, that's the first thing that comes to mind.

Yeah, parsers.c:version could be embedded, for example.

If that check were in `Index::new()`, I'm confident it wouldn't have any performance impact.
What kind of error feedback could we be sending, then ?

I think any Python-level exception should be fine. My only concern is somehow
incompatible C+Rust extensions happen to work (e.g. memory layout was similar),
but causes data loss. Import-time error might be better if that can be easily
implemented.

Revision Contents
Changeset List

PathPackages
Mmercurial/cext/revlog.c (14 lines)
Mrust/hg-cpython/src/cindex.rs (29 lines)

Diff 18918

mercurial/cext/revlog.c

#endif #endif
typedef struct indexObjectStruct indexObject; typedef struct indexObjectStruct indexObject;
typedef struct { typedef struct {
int children[16]; int children[16];
} nodetreenode; } nodetreenode;
typedef struct {
int (*index_parents)(PyObject *, int, int *);
} Revlog_CAPI;
/* /*
* A base-16 trie for fast node->rev mapping. * A base-16 trie for fast node->rev mapping.
* *
* Positive value is index of the next node in the trie * Positive value is index of the next node in the trie
* Negative value is a leaf: -(rev + 2) * Negative value is a leaf: -(rev + 2)
* Zero is empty * Zero is empty
*/ */
typedef struct { typedef struct {
0, /* tp_descr_get */ 0, /* tp_descr_get */
0, /* tp_descr_set */ 0, /* tp_descr_set */
0, /* tp_dictoffset */ 0, /* tp_dictoffset */
(initproc)rustla_init, /* tp_init */ (initproc)rustla_init, /* tp_init */
0, /* tp_alloc */ 0, /* tp_alloc */
}; };
#endif /* WITH_RUST */ #endif /* WITH_RUST */
static Revlog_CAPI CAPI = {
HgRevlogIndex_GetParents,
};
void revlog_module_init(PyObject *mod) void revlog_module_init(PyObject *mod)
{ {
PyObject *caps = NULL; PyObject *caps = NULL;
HgRevlogIndex_Type.tp_new = PyType_GenericNew; HgRevlogIndex_Type.tp_new = PyType_GenericNew;
if (PyType_Ready(&HgRevlogIndex_Type) < 0) if (PyType_Ready(&HgRevlogIndex_Type) < 0)
return; return;
Py_INCREF(&HgRevlogIndex_Type); Py_INCREF(&HgRevlogIndex_Type);
PyModule_AddObject(mod, "index", (PyObject *)&HgRevlogIndex_Type); PyModule_AddObject(mod, "index", (PyObject *)&HgRevlogIndex_Type);
nodetreeType.tp_new = PyType_GenericNew; nodetreeType.tp_new = PyType_GenericNew;
if (PyType_Ready(&nodetreeType) < 0) if (PyType_Ready(&nodetreeType) < 0)
return; return;
Py_INCREF(&nodetreeType); Py_INCREF(&nodetreeType);
PyModule_AddObject(mod, "nodetree", (PyObject *)&nodetreeType); PyModule_AddObject(mod, "nodetree", (PyObject *)&nodetreeType);
if (!nullentry) { if (!nullentry) {
nullentry = nullentry =
Py_BuildValue(PY23("iiiiiiis#", "iiiiiiiy#"), 0, 0, 0, -1, Py_BuildValue(PY23("iiiiiiis#", "iiiiiiiy#"), 0, 0, 0, -1,
-1, -1, -1, nullid, (Py_ssize_t)20); -1, -1, -1, nullid, (Py_ssize_t)20);
} }
if (nullentry) if (nullentry)
PyObject_GC_UnTrack(nullentry); PyObject_GC_UnTrack(nullentry);
caps = PyCapsule_New(HgRevlogIndex_GetParents, caps = PyCapsule_New(&CAPI, "mercurial.cext.parsers.revlog_CAPI", NULL);
"mercurial.cext.parsers.index_get_parents_CAPI",
NULL);
if (caps != NULL) if (caps != NULL)
PyModule_AddObject(mod, "index_get_parents_CAPI", caps); PyModule_AddObject(mod, "revlog_CAPI", caps);
#ifdef WITH_RUST #ifdef WITH_RUST
rustlazyancestorsType.tp_new = PyType_GenericNew; rustlazyancestorsType.tp_new = PyType_GenericNew;
if (PyType_Ready(&rustlazyancestorsType) < 0) if (PyType_Ready(&rustlazyancestorsType) < 0)
return; return;
Py_INCREF(&rustlazyancestorsType); Py_INCREF(&rustlazyancestorsType);
PyModule_AddObject(mod, "rustlazyancestors", PyModule_AddObject(mod, "rustlazyancestors",
(PyObject *)&rustlazyancestorsType); (PyObject *)&rustlazyancestorsType);
#endif #endif
} }

rust/hg-cpython/src/cindex.rs

// cindex.rs // cindex.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 to use the Index defined by the parsers C extension //! Bindings to use the Index defined by the parsers C extension
//! //!
//! Ideally, we should use an Index entirely implemented in Rust, //! Ideally, we should use an Index entirely implemented in Rust,
//! but this will take some time to get there. //! but this will take some time to get there.
use cpython::{PyClone, PyObject, PyResult, Python}; use cpython::{PyClone, PyObject, PyResult, Python};
use hg::{Graph, GraphError, Revision, WORKING_DIRECTORY_REVISION}; use hg::{Graph, GraphError, Revision, WORKING_DIRECTORY_REVISION};
use libc::c_int; use libc::c_int;
py_capsule_fn!( #[repr(C)]
from mercurial.cext.parsers import index_get_parents_CAPI pub struct Revlog_CAPI {
as get_parents_capi index_parents: unsafe extern "C" fn(
signature ( index: *mut revlog_capi::RawPyObject,
index: *mut RawPyObject,
rev: c_int, rev: c_int,
ps: *mut [c_int; 2], ps: *mut [c_int; 2],
) -> c_int ) -> c_int,
); }
py_capsule!(
from mercurial.cext.parsers import revlog_CAPI
as revlog_capi for Revlog_CAPI);
/// A `Graph` backed up by objects and functions from revlog.c /// A `Graph` backed up by objects and functions from revlog.c
/// ///
/// This implementation of the `Graph` trait, relies on (pointers to) /// This implementation of the `Graph` trait, relies on (pointers to)
/// - the C index object (`index` member) /// - the C index object (`index` member)
/// - the `index_get_parents()` function (`parents` member) /// - the `index_get_parents()` function (`parents` member)
/// ///
/// # Safety /// # Safety
/// parameter in the relevant methods that need one. /// parameter in the relevant methods that need one.
/// ///
/// Another possibility would be to introduce an abstract lock handle into /// Another possibility would be to introduce an abstract lock handle into
/// the core API, that would be tied to `GILGuard` / `Python<'p>` /// the core API, that would be tied to `GILGuard` / `Python<'p>`
/// in the case of the `cpython` crate bindings yet could leave room for other /// in the case of the `cpython` crate bindings yet could leave room for other
/// mechanisms in other contexts. /// mechanisms in other contexts.
pub struct Index { pub struct Index {
index: PyObject, index: PyObject,
parents: get_parents_capi::CapsuleFn, capi: &'static Revlog_CAPI,
} }
impl Index { impl Index {
pub fn new(py: Python, index: PyObject) -> PyResult<Self> { pub fn new(py: Python, index: PyObject) -> PyResult<Self> {
Ok(Index { Ok(Index {
index: index, index: index,
parents: get_parents_capi::retrieve(py)?, capi: unsafe { revlog_capi::retrieve(py)? },
}) })
} }
} }
impl Clone for Index { impl Clone for Index {
fn clone(&self) -> Self { fn clone(&self) -> Self {
let guard = Python::acquire_gil(); let guard = Python::acquire_gil();
Index { Index {
index: self.index.clone_ref(guard.python()), index: self.index.clone_ref(guard.python()),
parents: self.parents.clone(), capi: self.capi,
} }
} }
} }
impl Graph for Index { impl Graph for Index {
/// wrap a call to the C extern parents function /// wrap a call to the C extern parents function
fn parents(&self, rev: Revision) -> Result<[Revision; 2], GraphError> { fn parents(&self, rev: Revision) -> Result<[Revision; 2], GraphError> {
if rev == WORKING_DIRECTORY_REVISION { if rev == WORKING_DIRECTORY_REVISION {
return Err(GraphError::WorkingDirectoryUnsupported); return Err(GraphError::WorkingDirectoryUnsupported);
} }
let mut res: [c_int; 2] = [0; 2]; let mut res: [c_int; 2] = [0; 2];
let code = unsafe { let code = unsafe {
(self.parents)( (self.capi.index_parents)(
self.index.as_ptr(), self.index.as_ptr(),
rev as c_int, rev as c_int,
&mut res as *mut [c_int; 2], &mut res as *mut [c_int; 2],
) )
}; };
match code { match code {
0 => Ok(res), 0 => Ok(res),
_ => Err(GraphError::ParentOutOfRange(rev)), _ => Err(GraphError::ParentOutOfRange(rev)),
} }
} }
} }