This is an archive of the discontinued Mercurial Phabricator instance.

revlog: made C Capsule an array of function pointers
ClosedPublic

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

Details

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

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