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

Evolving the Stable ABI to use functions for INCREF, DECREF, TYPE #45

Closed
gvanrossum opened this issue Jun 5, 2023 · 17 comments
Closed

Comments

@gvanrossum
Copy link

The specific problem here is that there are macros in the Stable ABI that access the ob_refcnt and ob_type directly. This makes it impossible to move those fields or change the meaning of some bits in them.

At some point in the future it would be nice if the macros that access these (mainly Py_INCREF, Py_DECREF and Py_TYPE) were changed into real functions (not inline functions!), so that we can use some bits of these fields for flags. Possible uses include immortality, some GC state, and the nogil branch.

I am creating this issue in order to request feedback on how problematic this would be for current heavy users of the Stable ABI. There's a tradeoff: we'd be able to offer more stability for a one-time change. The cost of the one-time change would be twofold:

  • Wheels need to be rebuilt in order to be compatible with the new Stable ABI version.
  • Wheels using the Stable ABI would incur extra function call overhead for every INCREF and DECREF call (and for Py_TYPE, but I assume that's less of a concern).

This ties in with #39 -- we could arrange things so that 3.13 and several later versions still support the macros (or static inline functions -- for Stable ABI purposes that's pretty much the same thing), but any wheel built with 3.13 using the Stable ABI will use the functions. (You can still build wheels that use the macros, but they won't be compatible with future Python versions. I think this comes down to similar choices you have with HPy.)

My main question is whether this is ever going to be acceptable given the performance degradation due to the function calls. If it isn't, we have to do something else. If the performance hit is acceptable, we can talk about how we would evolve the Stable ABI to switch to function calls with minimal disruption for projects that use it. There would have to be a PEP in that case, I suspect.

@erlend-aasland
Copy link

FWIW, Victor already added Py_IncRef and Py_DecRef as functions in the Stable ABI1:

https://github.com/python/cpython/blob/f04c16875b649e2c2b420eb146308d0206c7e527/Objects/object.c#L261-L271

Footnotes

  1. Not sure why they're not in the Limited API; IMO, it would make sense to expose them there.

@gvanrossum
Copy link
Author

gvanrossum commented Jun 5, 2023

FWIW, Victor already added Py_IncRef and Py_DecRef as functions in the Stable ABI1:

Nice. I had hoped that was the case (but didn't have the time to research it). Is it in all versions of the Stable ABI? (The functions themselves look like they were added 19 years ago.) It would be nice if we could somehow recommend a best practice for building future-proof wheels that uses this whenever user code calls Py_INCREF or Py_DECREF. But I don't know how we would do that without tweaking the source code of old CPython versions.

@erlend-aasland
Copy link

Yes it looks like they've been there for quite a while; maybe I'm misremembering about Victor adding them. Anyways, since they were added in 3.2, I guess that means they're in all the versions of the Stable ABI.

@gvanrossum
Copy link
Author

But what we'd need is a way to cause all stable-abi-using wheels compiled going forward to use these instead of accessing ob_refcnt via the macros, even when compiled (as they likely will be) with older CPython versions. How could we do that?

@encukou
Copy link
Contributor

encukou commented Jun 6, 2023

Py_IncRef and Py_DecRef always were in the stable ABI, but they aren't actually called by Py_INCREF/Py_DECREF. They're there as exported symbols, so e.g. alternate implementations can use them.

So, first step is to change Python 3.13: make the macros call the functions in limited API. I don't think there's a better way to evaluate the performance impact than to just do it and test with alphas.
We can revert if necessary -- but I get the impression that if the performance impact is too big, the best course of acttion is to compile version-specific wheels -- and leave stable ABI builds for "tier 2" Python versions -- ones that are too old or too new (alphas/betas).

#15 (Array export and mass incref/decref) could help bring down overhead of multiple function calls.

The way to cause all stable-abi-using wheels to avoid macros is to switch to a new version of stable ABI, and deprecate abi3. The original plan from 2009 was to do it in Python 4.0. If the assumption was that 4.0 would follow 3.9, we're 3 years overdue :)
There are a bunch of issues to be solved in abi4, the main one being more precise versioning to allow rolling deprecations: for example we could say Python 3.30 supports abi4.20--abi4.30. (That would affect importlib & packaging, so it's a bit outside my comfort zone.)
If we do this I'd love abi3->abi4 transition to be API compatible, i.e. no functions are going away, you only need to recompile.


But what we'd need is a way to cause all stable-abi-using wheels compiled going forward to use these instead of accessing ob_refcnt via the macros, even when compiled (as they likely will be) with older CPython versions. How could we do that?

You can't. Even if we could you'd still need to recompile -- wheels that are already uploaded definitely won't magically switch. And if you need to recompile, why not use a newer Python?
To make this easy we probably want to avoid removing stuff from old Py_LIMITED_API, which AFAIK Victor started doing for 3.13.

@vstinner
Copy link
Contributor

vstinner commented Jun 7, 2023

Py_IncRef() checks for NULL and so might be a little bit less efficient: it's Py_XINCREF() as a function. So I added _Py_IncRef() which doesn't check for NULL: it's Py_INCREF().

@gvanrossum
Copy link
Author

But the latter aren’t in the Stable ABI yet, are they?

@vstinner
Copy link
Contributor

vstinner commented Jun 7, 2023

_Py_IncRef() was added to the stable ABI in Python 3.10. You can check Misc/stable_abi.toml:

[function._Py_IncRef]
    added = '3.10'
    abi_only = true

@vstinner
Copy link
Contributor

vstinner commented Jun 7, 2023

Since 2020 (Python 3.10 and 3.11), I'm working on enforcing usage of function calls to get and set PyObject and PyVarObject members.

My plan:

  • DONE: Fix the API (part 1): Convert macros to functions PEP 670
  • DONE: Fix the API (part 2): Disallow using macros as l-value PEP 674
  • [optional] Disallow a direct access to PyObject members. Maybe put PyObject members before PyObject* pointer and make PyObject structure empty (move members in a new internal _PyObject structure).
  • ON GOING: Fix the ABI (part 1): Only in the limited C API (stable ABI): Convert static inline functions to opaque function calls (only part with a potential impact on performance)
  • Fix the ABI (part 2): Same change, but in the regular API

I already made multiple changes:

@gvanrossum
Copy link
Author

  • ON GOING: Fix the ABI (part 1): Only in the limited C API (stable ABI): Convert static inline functions to opaque function calls (only part with a potential impact on performance)

This is the part that we need to be able to repurpose part of the ob_refcnt field.

  • Fix the ABI (part 2): Same change, but in the regular API

Why is this needed? There is no "regular ABI", is there?

@vstinner
Copy link
Contributor

vstinner commented Jun 8, 2023

Why is this needed? There is no "regular ABI", is there?

As soon as you give access to structure members, people will do weird things and may crash Python. For example, PySide was accessing directly PyObject.ob_refcnt to implement its own flavor of "static strings: https://bugreports.qt.io/browse/PYSIDE-1436 We cannot repurpose a structure member, remove it, move it, rename it, or whatever.

The less things are exposed, the more easy it is to evolve Python internals. See the current complexity of evolving PyObjcect.ob_refnct in Python 3.12: python/cpython#105059 There are new C compiler warnings/errors related to the new "unnamed union".

Touching anything in a structure is always causing a lot of pain. What Python core devs do? Not change structures anymore, and teach other core devs to do the same.

Well, that's basically the rationale of my withdrawn PEP 620: Hide implementation details from the C API.

If Py_INCREF() remains implemented as a static inline function accessing directly PyObject.ob_refcnt, it means that the member remains accessible in the public C API, and so people will abuse it, and we are no longer able to change it.

@vstinner
Copy link
Contributor

vstinner commented Jun 8, 2023

Well, see also issue #22 which is related.

@gvanrossum
Copy link
Author

As soon as you give access to structure members, people will do weird things and may crash Python. For example, PySide was accessing directly PyObject.ob_refcnt to implement its own flavor of "static strings: https://bugreports.qt.io/browse/PYSIDE-1436 We cannot repurpose a structure member, remove it, move it, rename it, or whatever.

That's debatable. Yes, there will be complaints, because sometimes the person complaining doesn't understand that the code they are using is doing something unsupported. But if they get an error during build, it's working as designed: they will have to fix their hack before they can continue.

In the specific case of the Stable ABI, the whole point is that they are not recompiling for each version, and that's what blocks us (the CPython maintainers) from evolving the code. We explicitly promise that if you only use the Stable ABI, your binary wheels will keep working with newer versions of CPython.

But we don't make such promises to users of the full API, which requires recompilation for each feature release (though generally not for bugfix releases).

Nevertheless, in general I agree that we should minimize access to struct members. But some APIs need those for performance. And I think that non-stable-ABI use of INCREF/DECREF falls in that category.

I also don't want to end up in a future where nobody can understand our headers any more because they are too full of hacks (like the C standard headers, which are very frustrating if you are trying to understand why something goes wrong).

PS. I didn't try to understand what happened in the PySide2 issue you linked to -- the conclusion seems that it was their fault though? So again, things work as promised.

@vstinner
Copy link
Contributor

vstinner commented Jun 9, 2023

I opened a discussion at: https://discuss.python.org/t/limited-c-api-implement-py-incref-and-py-decref-as-function-calls/27592

My implementation is now ready for review: python/cpython#105388

@vstinner
Copy link
Contributor

vstinner commented Jun 9, 2023

But we don't make such promises to users of the full API, which requires recompilation for each feature release (though generally not for bugfix releases).

It can be fixed, it's mostly a matter of tradeoff between compatibility and performance.

If all APIs are implemented as opaque function calls, as HPy and most of the limited C API do, there is no such ABI compatibility issue anymore.

I'm tracking the quantity of "inlined" code via macros and static inline functions at: https://pythoncapi.readthedocs.io/stats.html#functions-defined-as-macros-and-static-inline-functions

PEP 670: Convert macros to functions is a first step: fix the API to later consider converting static inline functions (as fast as macros) to opaque functions (unknown impact on performance, maybe not significant). Previously, many macros were abused for good or bad reasons in various ways, especially when functions implemented as macros had a return value whereas they "should not". I added _Py_RVALUE() macros for functions still implemente as macros which must not have a return value (void).

@gvanrossum
Copy link
Author

IIUC the stable ABI version 3.12 now makes Py_INCREF and Py_DECREF real functions.

@vstinner
Copy link
Contributor

python/cpython#105387 the stable ABI version 3.12 now makes Py_INCREF and Py_DECREF real functions.

I close the issue.

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

No branches or pull requests

5 participants