Make PyMutex public in the non-limited API
#22
Comments
|
+1 from me. This does introduce a non-opaque struct, which is a red flag worth focusing on. The practical issue with such a struct is that it's part of the ABI, so
There's some mind-blowingly genius engineering behind the "one-byte struct and two functions" API surface. But the idea is proven; it's been used in WebKit since 2015. I don't think it'll need to change any time soon. The struct member name should not appear in user code, so please mark it as private using a leading underscore: Note that this requires atomics -- the C11 optional feature, or the MSVC flavour. (The GCC flavour is an optional speedup.) |
|
Yeah, makes sense. |
|
The static inline function calls the private I would prefer to make the whole big Before
Later, once the static inline will be added, if Py_LIMITED_API macro is not defined, you would use the same API, but use the faster static inline functions. |
|
Current C compilers support C11/C++ atomics; other languages have their own locking. I don't think we want to add the wrappers as any kind long-term-support API; definitely not as dozens of type-specific functions.
The point here is to avoid memory allocation overhead (both time and space) for the one byte. If it helps: I see static inline void PyMutex_Lock(uint8_t *v) { ... }
static inline void PyMutex_Unlock(uint8_t *v) { ... }except we want to prevent users from doing arithmetic on v. |
How does someone use PyMutex in C++, Rust or any other language if _Py_atomic_compare_exchange_uint8() is not available for them? Knowning that it's implemented as a static inline function.
I don't think that I'm not sure that I understand. Do you suggest that people don't use the PyMutex API if
This is something new. I would prefer to not go this way. It looks very complicated. |
Unless there's a case where Rust code needs to lock / unlock a |
|
For C++, you just call the same code as C: I think the Rust situation is pretty different. In addition to what @davidhewitt wrote, mutexes in Rust protect specific data, so a mutex with no associated data is an awkward fit. If a mutex that releases the GIL when blocking makes sense, then I think PyO3 could implement it without too much difficulty. |
If Rust access an object which has a lock implemented as PyMutex, it should use the PyMutex_Lock() API while accessing the object, no? For example, |
|
Is that weakref access implemented entirely in |
|
We can additionally provide If we do expect people to re-implement these in other languages, rather than call them, then their body needs to be documented and stable. That's a can of worms, but, if people will do it anyway we might want to set reasonable rules and stability expectations.
No, there I wasn't talking about the |
|
I would really like for a decision to be made on this proposal |
|
I'm okay with whatever @encoku says. |
|
And the others?
|
|
Main change I'd like to see is replacing the explicit initialization ( And unless there's an important reason otherwise, specify the public Footnotes
|
|
I am not in favor of any of these suggestions. The pointer size means a completely different type and different API. In other words, a different proposal. Regarding Practically, most mutexes are either global (i.e., statically initialized to zero) or instances of some struct. For subtypes of PyObject, the typical initialization in extensions already zero initialize fields so any We should not make the API more complicated just for some unlikely hypothetical changes. Note that |
|
Okay, I'll concede zero initialization (and no destruction), but I still worry about having a public, non-opaque API based around a single byte. The API doesn't change in that case - we only have to use a single byte in the struct for what we currently want - in fact, I'd settle for the struct remaining at one byte but the specification saying to ensure you have Can you expand a little on why Footnotes
|
So there is a API for that? Can you add an API for static init? |
To be clear, I mean that it would not be suitable for some of the current uses of
I'm not opposed to providing extra guidance for something like Rust bindings, but I don't know what concretely we can say or what actions we expect them to take. PyO3 could make their I wrote this in the other issue, but the primary ways of extending
Yes, that's true -- the bigger problem is that having more than one size or definition of |
No, I was not suggesting an API for static init. I was trying to point out that if you want an API for initialization you need at least two functions/macros. |
This is an important point to highlight in the API - the address has to be stable (in effect, the mutex is the address of the mutex, not merely the value).
What would be wrong with a definition like this: We still get our single byte, it's going to be as stably in the same position as anything else we could define, and the ABI is now immune to us adding up to three additional bytes for every supported platform, or changing the value of the mutex to a pointer (e.g. pointing directly into our own table in order to make the mutex value-based rather than address-based). |
This is the same as other locking APIs, the type is just smaller: the functions take a The
It's incompatible with the one-byte internal definition and doesn't adhere to the "one definition rule." I'd expect things to probably work out okay, but it doesn't seem to me like a good enough reason deviate from standard compliant C. I don't think we will want a pointer/table based mutex, but if, hypothetically we wanted that, we should just add a new API instead of trying to repurpose existing APIs. EDIT: The "one definition rule" might not be the relevant rule, but trying to use the same functions on differently defined types seems fishy even if it works in practice. |
The point is that it's different from most of our APIs, so it should be highlighted (or avoided, but I assume making it an explicit part of the API is preferable).
Sure, whatever. I'm getting sick of arguing in favour of API stability on every issue. If nobody else from the WG wants to work for it at the early stages, then consider me abstained from this vote. |
_PyObject_GC_New() does not initialize allocated memory to zeros. Many types use _PyObject_GC_New(). I would prefer to have a macro and a function to initialize a mutex. Something like: #define PyMutex_STATIC_INIT (PyMutex){0}
void PyMutex_Init(PyMutex *mutex)
{
*mutex = PyMutex_STATIC_INIT;
}
static PyMutex mutex = PyMutex_STATIC_INIT;
PyObject* new_object(void)
{
MyObject *obj = PyObject_GC_New(MyObject, MyType);
// obj->mutex = PyMutex_STATIC_INIT;
PyMutex_Init(&obj->mutex);
return (PyObject*)obj;
} |
typedef struct PyMutex { ... } PyMutex;
static inline void PyMutex_Lock(PyMutex *m) { ... }
static inline void PyMutex_Unlock(PyMutex *m) { ... }Can you please expand the |
|
Please see the PR linked above for the actual code: |
|
The below suggested pattern will not compile in MSVC. It relies on a GCC extension. #define PyMutex_STATIC_INIT (PyMutex){0}
static PyMutex mutex = PyMutex_STATIC_INIT; |
|
I don't think API stability is an issue. Changing the AFAIK, the dynamic-allocation API for locking is Perhaps we'll get another non-C-language-friendly, ABI-stable mutex as free-threading matures. (Maybe as a VM-managed part of a PyObject to make dynamic allocation worthwhile, with lock and unlock API on the object.) This proposal is exposing the low-level machinery, for cases where you're counting bits and instructions. |
|
Note also that the size of the mutex is included in the specification of the accepted PEP 703. |
|
Yes, the design comes from the WebKit lock linked to by @zooba. It was added in Aug 2015. The major change since then was in July 2016 when it became "eventually fair", which entailed going from three states (two bits in one byte) to four states (still two bits in one byte); the major implementation changes happen at a lower levels of the API. I'm still not exactly sure how the C API WG is intended to operate. I think it'd be helpful if the WG as a whole decided what it wanted from this API. |
We go for consensus, and when we give up on that we vote. In general, when I'm pushing for future-proof APIs like this, I eventually get outvoted 😉 So if you insist on sticking to the current design, you probably just have to hold on until the other members give up and call for a vote. This discussion has gone on pretty long (and I've repeated my position 3-4 times already), so I expect that won't be long, if only because nobody wants to re-read the whole thing anymore. |
|
Let's say that the struct can grow to up to AFAICS, Docs for “both the address and the value” can be added. Some non-C languages call this “pinning” (rust, golang). |
|
I'm fine with the 10 year old, battle tested one-byte design of WTF locks. The size of Quoting Petr, from early in the discussion:
Consider me +1 for adding the API. Perhaps there is a naming issue? Would |
Agreed. |
The If this API is required for users to directly use the mutex embedded in the object structure, then they need to be the same. I asked earlier whether this was the case and was told no: they are independent, but making the private API public is just convenient. So PEP 703 doesn't force us into anything here.
I'm okay with this. |
This is not how I interpret any of Sam's reponses; I won't reiterate them, as they are all easily found in this discussion, and I think each of them is a good (re)read. I'm 100% aligned with Sam's argumentation, and I think it would be a really bad idea to change the API in any way, including making it dynamic, and/or changing the size of the mutex. I'm fine with:
|
|
I'm fine with adding a |
I don't think any of these are necessary. I have no problem committing to support the API over multiple releases, provided it gets a bit of buffer space, adding "Static" to names doesn't make anything clearer in my opinion, and saying "initialize to zero" is sufficiently future-proof (provided we can detect an uninitialised mutex on first use, which it sounds like we can, because our internal hash set won't have an entry).
I think there's a valuable use for a |
|
If I understood correctly, the proposed API would look like: typedef struct PyMutex { ... } PyMutex;
#define PyMutex_STATIC_INIT ...
static inline void PyMutex_Init(PyMutex *m) { ... }
static inline void PyMutex_Lock(PyMutex *m) { ... }
static inline void PyMutex_Unlock(PyMutex *m) { ... }
static inline void PyMutex_Destroy(PyMutex *m) { ... }Did I miss anything? I'm fine with this API. I don't worry much about the |
|
If you want to let users of the API assert that no one is waiting on the mutex, we should make I think that
|
Yeah, I realised this as well.
I'm sure we get that right for our internal uses, but it's far more likely that users will use this in different ways and likely get it wrong. A
I'd like to see error results as well, not just |
I dislike adding an error code path when it's possible to avoid it. I prefer to return void if the function "cannot fail". I'm not sure which errors you're referring to. Which functions can/should be able to fail? |
It's not useful for making control flow decisions, but it's useful for assertions. Both "assert that the mutex is not locked" and "assert that this mutex is already locked" (i.e., by a caller higher up the stack), which is particularly helpful for catching bugs when adding locking to make existing code thread-safe.
Adding error codes complicates the use of the API to the point where callers are best served by just ignoring them. How are you supposed to handle a failure of The only possible failures are due to incorrect usage, like unlocking a mutex that is not locked, but returning an error code in that case isn't a good design because they are likely to be ignored.
There's still a runtime cost if callers feel the need to write something like: https://github.com/python/cpython/blob/153b118b78588209850cc2a4cbc977f193a3ab6e/Python/ceval_gil.c#L122-L124 |
Yes, we had that discussion. I guess in our next in-person we'll argue this one out some more, but I'm very much with @encukou on this. APIs that can have error results, should have error results.
The fact that some situations cannot be handled doesn't invalidate all the ones where they can be handled. A public API is going to be used in a lot of ways we can't predict - we should leave it up to users to know whether they can or can't handle it in whatever context they're in. All we can really predict is that people will indeed use it incorrectly, and so failures are possible, and it will help people if we report them. Also, handling doesn't necessarily mean recovering. It might mean asserting with some application-specific assert functionality. How do you intend for users to find bugs in their
Any optimiser that can't figure out that a literal |
There is feedback -- it fails with a
That only works if the slow path functions return |
Users cannot handle errors if you do not describe the error conditions. What are the error conditions for We should not try to make the APIs that are as general as possible. We should aim for APIs that are easy to use and hard to misuse. Vaguely described error conditions that never happen in practice make the API harder to use correctly. |
If there's genuinely no errors possible (as claimed for the current implementation), then these can continue to return void. My concern is about what we tell consumers to write, so that if/when the implementations change, their code still works without them having to rewrite it (but possibly they have to recompile).
"Not a mutex" or "not locked", though I appreciate that the first is not distinguishable with this implementation. But errors are not difficult to identify.
Clearly we just disagree on how to interpret this. There's a lot of generality I'm not asking for, and what I am asking is not expanding the scope of the APIs at all. I just want to ensure that users who use our public APIs won't feel screwed over if we later change the implementation. We've jumped through so many hoops to avoid changing APIs like |
We're talking about a dead simple static one-byte mutex API; comparing it with something as complex as All participants in this thread, including Steve, has acknowledged there is a vanishing probability that this API will ever need to change. Why weigh it heavier than the rest of the API design factors? To quote the infamous zen, just for fun: practicality beats purity, simple is better than complex. |
|
Before the C API Working Group was created, many APIs were added without any design for many years. Some were more lucky and tried to avoid some issues and were reviewed by a few people. At the end, the C API exists, has a few flaws but it's not the end of the world. Now I see the C API Working Group as a gatekeeper preventing other people from changing the C API. It's good to have more eyes on changes, to prevent known issues. But there it seems like the discussion goes nowhere and Sam is just blocked by the Working Group. I don't think that the discussion is still constructive but I don't know how to move on. We are not going to solve all issues suddenly at once. We can try to avoid the most issues pitfals. But here I feel like we are exploring an hypothetical future of an API which doesn't already exist. Let's allow us to make mistakes. If the API looks fragile, we can use the "provisional" statuscin the doc. The API targets free threading which is already experimental, no? Early adopters would not be surprised if the API changes. |
|
Thanks Victor. I suggest we go for the unstable API tier and Sam's design. That is probably the best way to move forward in order to please everyone. |
Yup. The rules are very useful for everyday API design, but each comes with reasoning and trade-offs we need to consider if we want to break them.
Both are a problem for the stable ABI, which means that if we add
IMO,
I don't share that optimism; I think we will want to change or remove it in the future. When that happens, it'll be messy for us and mean extra work for users.
Sam, sorry that you got caught in the cross-fire. (There's a reason Steering Council meetings are private...) Steve, thanks for brainstorming; your ideas are valuable even if (if!) they don't make it. |
|
The C API Working Group met to discuss this proposal and has decided to accept the API as shown in the original post with the following clarifications:
There is no need to artificially increase the size of We would like to see the inline functions also exported as regular functions, in line with our desire to see the API be usable without relying on macros. This would be required for Limited API inclusion, but is nice to have regardless. In our view, exported functions do not constrain future API changes any more than inline functions. A fresh review by the WG will be required before these may be added to the Limited API. — Petr, on befalf of the WG |
|
(Clarification: by "major releases" we mean "feature releases", IOW the size might change in 3.15.) |
|
I will work on the PR for PyMutex this week (hopefully today). |
@colesbury is now working on updating his PR. IMO we can now close the issue. |
Links
CPython Issue: python/cpython#117511
PR draft: python/cpython#117731
Additional discussion: https://discuss.python.org/t/make-some-pymutex-apis-public/50203
Overview
The
PyMutexAPIs are currently internal-only inpycore_lock.h. This proposes making the type and two functions public as part of the general, non-limited API inInclude/cpython/lock.h.The APIs to be included in the public header are:
Additionally, the out-of line of "slow path" functions are exposed in the header, but not part of the public API. (For context, these are called when
PyMutex_Lockneeds to block and whenPyMutex_Unlockneeds to wake up another waiting thread).The text was updated successfully, but these errors were encountered: