Patchwork D7543: revlog: made C Capsule an array of function pointers

login
register
mail settings
Submitter phabricator
Date Dec. 2, 2019, 2:31 p.m.
Message ID <differential-rev-PHID-DREV-wtfc4ayernft7aiewj6u-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43567/
State Superseded
Headers show

Comments

phabricator - Dec. 2, 2019, 2:31 p.m.
gracinet created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

REVISION 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.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D7543

AFFECTED FILES
  mercurial/cext/revlog.c
  rust/hg-cpython/src/cindex.rs

CHANGE DETAILS




To: gracinet, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
phabricator - Dec. 23, 2019, 5:53 p.m.
This revision is now accepted and ready to land.
indygreg added a comment.
indygreg accepted this revision.


  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.

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7543/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7543

To: gracinet, #hg-reviewers, indygreg
Cc: indygreg, durin42, kevincox, mercurial-devel
Yuya Nishihara - Jan. 4, 2020, 9:34 a.m.
> +typedef struct {
> +	int (*index_parents)(PyObject *, int, int *);
> +} Revlog_CAPI;

Do we have any plan to detect C-Rust ABI mismatch? Renaming the symbol?
phabricator - Jan. 4, 2020, 9:35 a.m.
yuja added a comment.


  > +typedef struct {
  > +	int (*index_parents)(PyObject *, int, int *);
  > +} Revlog_CAPI;
  
  Do we have any plan to detect C-Rust ABI mismatch? Renaming the symbol?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7543/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7543

To: gracinet, #hg-reviewers, indygreg
Cc: yuja, indygreg, durin42, kevincox, mercurial-devel
phabricator - Jan. 4, 2020, 10:42 a.m.
gracinet added a comment.


  @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 ?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7543/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7543

To: gracinet, #hg-reviewers, indygreg
Cc: yuja, indygreg, durin42, kevincox, mercurial-devel
Yuya Nishihara - Jan. 4, 2020, 1:02 p.m.
>   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.
phabricator - Jan. 4, 2020, 1:04 p.m.
yuja added a comment.


  >   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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7543/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7543

To: gracinet, #hg-reviewers, indygreg
Cc: yuja, indygreg, durin42, kevincox, mercurial-devel

Patch

diff --git a/rust/hg-cpython/src/cindex.rs b/rust/hg-cpython/src/cindex.rs
--- a/rust/hg-cpython/src/cindex.rs
+++ b/rust/hg-cpython/src/cindex.rs
@@ -14,15 +14,18 @@ 
 use hg::{Graph, GraphError, Revision, WORKING_DIRECTORY_REVISION};
 use libc::c_int;
 
-py_capsule_fn!(
-    from mercurial.cext.parsers import index_get_parents_CAPI
-        as get_parents_capi
-        signature (
-            index: *mut RawPyObject,
-            rev: c_int,
-            ps: *mut [c_int; 2],
-        ) -> c_int
-);
+#[repr(C)]
+pub struct Revlog_CAPI {
+    index_parents: unsafe extern "C" fn(
+        index: *mut revlog_capi::RawPyObject,
+        rev: c_int,
+        ps: *mut [c_int; 2],
+    ) -> 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
 ///
@@ -58,14 +61,14 @@ 
 /// mechanisms in other contexts.
 pub struct Index {
     index: PyObject,
-    parents: get_parents_capi::CapsuleFn,
+    capi: &'static Revlog_CAPI,
 }
 
 impl Index {
     pub fn new(py: Python, index: PyObject) -> PyResult<Self> {
         Ok(Index {
             index: index,
-            parents: get_parents_capi::retrieve(py)?,
+            capi: unsafe { revlog_capi::retrieve(py)? },
         })
     }
 }
@@ -75,7 +78,7 @@ 
         let guard = Python::acquire_gil();
         Index {
             index: self.index.clone_ref(guard.python()),
-            parents: self.parents.clone(),
+            capi: self.capi,
         }
     }
 }
@@ -88,7 +91,7 @@ 
         }
         let mut res: [c_int; 2] = [0; 2];
         let code = unsafe {
-            (self.parents)(
+            (self.capi.index_parents)(
                 self.index.as_ptr(),
                 rev as c_int,
                 &mut res as *mut [c_int; 2],
diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
--- a/mercurial/cext/revlog.c
+++ b/mercurial/cext/revlog.c
@@ -37,6 +37,10 @@ 
 	int children[16];
 } nodetreenode;
 
+typedef struct {
+	int (*index_parents)(PyObject *, int, int *);
+} Revlog_CAPI;
+
 /*
  * A base-16 trie for fast node->rev mapping.
  *
@@ -3029,6 +3033,10 @@ 
 };
 #endif /* WITH_RUST */
 
+static Revlog_CAPI CAPI = {
+    HgRevlogIndex_GetParents,
+};
+
 void revlog_module_init(PyObject *mod)
 {
 	PyObject *caps = NULL;
@@ -3052,11 +3060,9 @@ 
 	if (nullentry)
 		PyObject_GC_UnTrack(nullentry);
 
-	caps = PyCapsule_New(HgRevlogIndex_GetParents,
-	                     "mercurial.cext.parsers.index_get_parents_CAPI",
-	                     NULL);
+	caps = PyCapsule_New(&CAPI, "mercurial.cext.parsers.revlog_CAPI", NULL);
 	if (caps != NULL)
-		PyModule_AddObject(mod, "index_get_parents_CAPI", caps);
+		PyModule_AddObject(mod, "revlog_CAPI", caps);
 
 #ifdef WITH_RUST
 	rustlazyancestorsType.tp_new = PyType_GenericNew;