Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EXTREMELY WIP] Attempt to use the Python Stable ABI #308

Closed
wants to merge 9 commits into from

Conversation

Techcable
Copy link
Contributor

@Techcable Techcable commented Jan 28, 2021

The benefit of this is that jep could compile once and support multiple python versions. This could potentially allow the distribution of compiled binaries/wheels ahead of time.

Mostly this just means moving from fast access macros
to the checked runtime calls. This comes at some very minor performance cost (that is only present when we're compiling against the limited ABI).
Most of the changes so far involved the Unicode APIs. We can't have direct access to the internal PyUnicode_KIND representation.

Right now I've just hardcoded it on with a #define PY_LIMITED_API at the top of the jep_platform.h file, but of course I intend to make it configurable later.

It looks like some of the functions required for embedding are missing.

Not only are flags like Py_VerboseFlag and Py_OptimizeFlag
unavailable, so are things like PyRun_String.

We might have to sacrifice the Py_VerboseFlag and other config options
entirely, I'm not seeing a replacement available.
However, the other problems I'm sure I can work around....

@Techcable
Copy link
Contributor Author

SHOOOT! There was a git mixup. I'll rebase to master soon 😆

@bsteffensmeier
Copy link
Member

I recommend you work off the dev_4.0 branch so you don't have to work with all the python 2 compatibility.

@Techcable Techcable force-pushed the refactor/use-stable-api branch from b622b10 to 7993c2d Compare February 3, 2021 04:37
@Techcable Techcable changed the base branch from master to dev_4.0 February 3, 2021 04:37
@Techcable Techcable force-pushed the refactor/use-stable-api branch from 7993c2d to 805003d Compare February 3, 2021 04:38
We have to wrap the buffer interface with our own support
for `bytes` and `bytearray`. It is pretty disgusting.

I'm starting to make all the types we use heap-allocated instead
of statically declared.

TODO Soon:
- Finish convert all the types to heap types
- Finish the various missing functions from `pyembed.c`
In the stable ABI, this is the only way to declare types.
We should have better cross-version compatibility now.
It's depreciated and un-nessicarry
TODO: What is the story here with the stable API

I'm under the impression it was included (and nessicarry for old
versions), however the python 3.9 docs claim it will be 'removed in
version 3.11'.
Same thing is needed for PyRun_String
This is needed on the stable ABI, where those symbols aren't
defined.
@Techcable Techcable force-pushed the refactor/use-stable-api branch from 805003d to 4920dc9 Compare February 16, 2021 02:50
@Techcable
Copy link
Contributor Author

Techcable commented Feb 16, 2021

Okay, I merged this onto the development branching the GitHub Web UI. Technically, this can't break the build because it wasn't working in the first place 🙈

This is definitely still unstable

I want to bring up a couple of changes that I have made in this, some of which impact regular (non-stable API) compilations.

Major changes so far

  1. Using heap types for all extension types
    • In the python extension world, declaring Heap types are definitely less common than declaring type structures directly
    • This is absolutely needed for any version stability since PyTypeObject is opaque on the stable ABI and changes throughout versions
    • We really can't work around this, probably the only mandatory change to get this PR through.
  2. Switch a lot of fast access macros like PyList_GET_ITEM to their equivalent functions (like PyList_GetItem)
    • This should very rarely have a significant performance benefit. All it adds is bounds checks + type sanity.
    • This actually comes with the upside of better compatibility with PyPy and other alternative implementations.
  3. Using new abstractions to emulate missing APIs
    • Some of the methods that JEP use are missing from the stable API. If I can't (or don't want to) remove their usages, I will wrap them in our own abstraction.
    • Unfortunately, many of the APIS relating to interpreter initialization, thread state, and sub interpreters are not in the stable API
      • Honestly, this makes sense given the impending 'GIL per sub interpreter' changes
    • TODO: Remove my internal JepBuffer abstraction
      • The abstraction I hacked together seems particularly ugly and probably will cause bugs for standard code down the line.
      • As far as I can tell, the conversion logic for python buffers also handles bytes and bytearray objects, so I didn't want to remove it outright
      • It is probably far simpler to change the conversion logic to fallback to different behavior on the stable API than trying to emulate the entire buffer interface just for the benefit of bytes and bytearray

I don't think this code would be worth a review in its present state (it doesn't compile, and I plan to remove the buffer API). However, I would like to if you have any objections to the overall design.

Have a good day 😄

When the limited ABI is enabled, we can't access `PyThreadState.interp`.
Instead, we have to use the API function `PyThreadState_GetInterpreter`.

Unfortunately, this API is new as of Python 3.9 so versions
before that simply cannot know the current interpreter that
is running in the main thread......
As such, I include a new define `JEP_ASSUME_SINGLE_INTERPRETER`
where we just kind of wing it and assume there is only one active
interpreter (which is actually pretty common in
CPython land - a lot of extensions aren't built with subinterpreters
in mind).
Everything compiles now, but it crashes on first load ^_^
@Techcable Techcable force-pushed the refactor/use-stable-api branch from 9879f85 to 3d35060 Compare March 6, 2021 21:32
This properly creates a 'bases' tuple for passing
to `PyType_FromSpecAndBases`. Better error handling too.
This fixes a SEGFAULT we had....
Copy link
Contributor Author

@Techcable Techcable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a very good idea, because of the limitation of the Python CAPI.
It's not designed with embedding in mind, and jep is such a niche use case it's unlikely to support everything we need even if it becomes more popular....

* To work around it, we define the extern function ref by hand.
*/
#if defined(Py_LIMITED_API) && Py_LIMITED_API+0 < 0x030A0000
extern const char *PyUnicode_AsUTF8AndSize(PyObject *unicode, Py_ssize_t *size);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a hack

*
* TODO: Make this configuration optional
*/
#define Py_LIMITED_API 0x03080000
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a hack.

Arguably, we could make this some sort of build flag given in the makfile or setup.py.
However, it's too unstable and feature-incomplete to ever become the default, so why bother?

compile_commands.json

# Clangd
.cache/clangd
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, these should just be here anyways 😉

abort(); // Not a fast sequence: Undefined behavior
}
}
static inline PyObject *PySequence_Fast_GET_ITEM(PyObject *target, Py_ssize_t index) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the performance matter here?
If YES, then why are we paying the performance price of the stable ABI in other places?
If NO, then why are we reimplementing this whole method here?

@@ -169,4 +169,6 @@ extern jclass JDOUBLE_ARRAY_TYPE;
#define DEFINE_CLASS_GLOBAL(var, name) extern jclass var;
CLASS_TABLE(DEFINE_CLASS_GLOBAL)

int jep_util_type_ready(PyTypeObject **target_type, PyType_Spec *spec, PyTypeObject *base);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice utility. I feel like in general, the project should switch to using heap-allocated types.

That seems like it's becoming the new sort of standard for CAPI extensions.

@@ -551,7 +591,7 @@ void pyembed_shared_import(JNIEnv *env, jstring module)
{
const char *moduleName = NULL;
PyObject *pymodule = NULL;
PyEval_AcquireThread(mainThreadState);
PyEval_AcquireThread(get_main_pystate());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is gross. We're emulating yet another API on top of the Stable API......

*/
PyGILState_STATE state = PyGILState_Ensure();
jepThread->tstate = PyThreadState_Get();
PyGILState_Release(state);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this hack supposed to be more 'stable'?

#ifdef Py_LIMITED_API
inum = (int) PyList_Size(interfaces);
if (inum < 0) {
assert(PyErr_Occurred());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the heck?

// Change target to UTF8 cstring, then delegate
target.target_type = JEP_RUN_STRING_C;
target.c_string = result_string;
exec_result = pyembed_util_run(target, file_name, input_type, locals, globals);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets not reimplement functionality that's already in CPython!!!

@@ -516,49 +525,31 @@ static PyMethodDef pyjobject_methods[] = {

static PyMemberDef pyjobject_members[] = {
{"__dict__", T_OBJECT, offsetof(PyJObject, attr), READONLY},
{"__dictoffset__", T_PYSSIZET, offsetof(PyJObject, attr), READONLY}, // NOTE: Sets `PyTypeObject.tp_dictoffset` for heap types...
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting replacement for tp_dictoffset......
Looks like this is actually used in the internals somewhere...

Oddly, the heap-type API is mostly undocumented....

@Techcable
Copy link
Contributor Author

I'm closing this because I feel the Python "Stable" CAPI is too limited and is unlikely to provide any benefits for jep.

I think some of the refactorings that I've done here is still a good idea, even though I don't think this PR should be accepted.
The switch to using heap-allocated types is debatable, but it seems like a good idea with the ongoing encapsulation of the CAPI. The PEP specifically identifies PyTypeObject as a particularly problematic structure for its version and implementation dependence.......

Thank you so much for your time!
I appreciate your effort maintaining jep :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants