gh-117511: Make PyMutex public in the non-limited API #117731

colesbury · 2024-04-10T20:47:17Z

Issue: Make some PyMutex functions public #117511

📚 Direct link 📚: https://cpython-previews--117731.org.readthedocs.build/en/117731/c-api/init.html#synchronization-primitives
📚 Documentation preview 📚: https://cpython-previews--117731.org.readthedocs.build/

Doc/c-api/init.rst

Misc/NEWS.d/next/C API/2024-04-10-16-48-04.gh-issue-117511.RZtBRK.rst

Co-authored-by: Erlend E. Aasland <[email protected]>

Update code comments to further clarify that the `_bits` field is not part of the public C API.

colesbury · 2024-06-17T19:44:28Z

@encukou @vstinner - I've updated the docs based on capi-workgroup/decisions#22 (comment).

I'm not sure of the best way to make the functions also exported as regular functions, particularly with the same name. It's easy to export them with different names and _PyMutex_LockSlow and _PyMutex_UnlockSlow are effectively that.

vstinner · 2024-06-17T19:51:45Z

I'm not sure of the best way to make the functions also exported as regular functions, particularly with the same name.

You can look at my PR which does exactly that just for Py_TYPE(): https://github.com/python/cpython/pull/120601/files

In short, add an underscore prefix to the static inline function, then use a macro to "rename" the static inline function.

Include/cpython/lock.h

vstinner · 2024-06-18T06:40:24Z

Include/lock.h

+
+#ifndef Py_LIMITED_API
+#  define Py_CPYTHON_LOCK_H
+#  include "cpython/lock.h"


I don't see the value of such header file. Just include cpython/lock.h in Python.h, and check Py_LIMITED_API in cpython/lock.h.

@ericsnowcurrently expressed a preference for this style when pyatomic.h was added: #109344 (comment)

IMO, it's helpful to follow a consistent pattern when it comes to the Include/cpython header files. That means in some cases we end up with very minimal header files like this in Include/.

If you mention consistency, there are already many header files in Include/cpython/ which have no companion Include/ header file:

$ grep cpython/ Include/Python.h #include "cpython/pydebug.h" #include "cpython/longintrepr.h" #include "cpython/odictobject.h" #include "cpython/funcobject.h" #include "cpython/classobject.h" #include "cpython/code.h" #include "cpython/cellobject.h" #include "cpython/initconfig.h" #include "cpython/genobject.h" #include "cpython/picklebufobject.h" #include "cpython/pytime.h" #include "cpython/context.h" #include "cpython/pyctype.h" #include "cpython/pyfpe.h" #include "cpython/tracemalloc.h" #include "cpython/optimizer.h"

vstinner · 2024-06-18T06:42:44Z

Objects/object.c

@@ -2372,7 +2372,7 @@ new_reference(PyObject *op)
 #else
    op->ob_tid = _Py_ThreadId();
    op->_padding = 0;
-    op->ob_mutex = (struct _PyMutex){ 0 };
+    op->ob_mutex = (PyMutex){ 0 };


If there is no public PyMutex_STATIC_INIT, can you maybe add a private one in pycore_lock.h?

Let's consider that in a separate PR:

It's not directly related to making the API public

It would make sense to consider that for multiple types (e.g., PyEvent, _PyOnceFlag), not just PyMutex

vstinner · 2024-06-18T06:43:57Z

Python/lock.c

 void
-_PyMutex_LockSlow(PyMutex *m)
+PyMutex_Lock(PyMutex *m)


You might move it to the end to still use the static inline macro in the following lines.

Include/cpython/lock.h

Doc/c-api/init.rst

vstinner · 2024-06-19T07:39:37Z

Include/lock.h

+
+#ifndef Py_LIMITED_API
+#  define Py_CPYTHON_LOCK_H
+#  include "cpython/lock.h"


If you mention consistency, there are already many header files in Include/cpython/ which have no companion Include/ header file:

$ grep cpython/ Include/Python.h #include "cpython/pydebug.h" #include "cpython/longintrepr.h" #include "cpython/odictobject.h" #include "cpython/funcobject.h" #include "cpython/classobject.h" #include "cpython/code.h" #include "cpython/cellobject.h" #include "cpython/initconfig.h" #include "cpython/genobject.h" #include "cpython/picklebufobject.h" #include "cpython/pytime.h" #include "cpython/context.h" #include "cpython/pyctype.h" #include "cpython/pyfpe.h" #include "cpython/tracemalloc.h" #include "cpython/optimizer.h"

Doc/c-api/init.rst

Python/lock.c

vstinner

LGTM. Thanks for the different update and the doc.

@colesbury got an exception from @Yhg1s (3.13 release manager) to add this API to Python 3.13, god.

miss-islington-app · 2024-06-20T15:29:11Z

Thanks @colesbury for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington-app · 2024-06-20T15:29:16Z

Sorry, @colesbury, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 3af7263037de1d0ef63b070fc7bfc2cf042eaebe 3.13

…17731) (cherry picked from commit 3af7263)

…ythonGH-117731) (cherry picked from commit 3af7263) Co-authored-by: Sam Gross <[email protected]>

bedevere-app · 2024-06-20T15:35:51Z

GH-120800 is a backport of this pull request to the 3.13 branch.

…) (#120800) (cherry picked from commit 3af7263)

vstinner · 2024-06-20T16:17:11Z

Yeah, congrats!

erlend-aasland · 2024-06-27T22:01:33Z

@colesbury got an exception from @Yhg1s (3.13 release manager) to add this API to Python 3.13, god.

That is very good news! Glad to see this land.

…17731)

pythongh-117511: Make PyMutex public in the non-limited API

3c743f8

bedevere-app bot mentioned this pull request Apr 10, 2024

Make some PyMutex functions public #117511

Closed

colesbury added 3 commits April 10, 2024 16:48

Add blurb

d340d28

Add missing newline

e59e0f7

Replace struct _PyMutex with PyMutex

1cef880

colesbury mentioned this pull request Apr 10, 2024

Make PyMutex public in the non-limited API capi-workgroup/decisions#22

Closed

erlend-aasland reviewed Apr 11, 2024

View reviewed changes

Doc/c-api/init.rst Outdated Show resolved Hide resolved

Doc/c-api/init.rst Outdated Show resolved Hide resolved

Doc/c-api/init.rst Outdated Show resolved Hide resolved

Misc/NEWS.d/next/C API/2024-04-10-16-48-04.gh-issue-117511.RZtBRK.rst Outdated Show resolved Hide resolved

colesbury and others added 7 commits April 11, 2024 11:53

Apply suggestions from code review

28c90bc

Co-authored-by: Erlend E. Aasland <[email protected]>

Update Doc/c-api/init.rst

e321817

Co-authored-by: Erlend E. Aasland <[email protected]>

Use underscore prefix for PyMutex field.

6533089

Update code comments to further clarify that the `_bits` field is not part of the public C API.

Fix comment

215020a

PyMutex should not be copied or moved

c0f75f9

Merge branch 'main' into pythongh-117511-pymutex

4ebc5e5

Update documentation

19a3492

colesbury marked this pull request as ready for review June 17, 2024 19:38

colesbury requested a review from a team as a code owner June 17, 2024 19:38

bedevere-app bot added the awaiting core review label Jun 17, 2024

colesbury requested review from vstinner and encukou June 17, 2024 19:38

Export PyMutex_Lock and PyMutex_Unlock as regular functions

7230134

vstinner reviewed Jun 17, 2024

View reviewed changes

Include/cpython/lock.h Show resolved Hide resolved

vstinner reviewed Jun 18, 2024

View reviewed changes

Move PyMutex_Lock/Unlock definitions to end of file

6b2d377

colesbury added the needs backport to 3.13 bugs and security fixes label Jun 18, 2024

vstinner reviewed Jun 19, 2024

View reviewed changes

Doc/c-api/init.rst Outdated Show resolved Hide resolved

Update Doc/c-api/init.rst

476e493

vstinner reviewed Jun 19, 2024

View reviewed changes

vstinner approved these changes Jun 20, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Jun 20, 2024

colesbury merged commit 3af7263 into python:main Jun 20, 2024
38 checks passed

bedevere-app bot removed the awaiting merge label Jun 20, 2024

miss-islington-app bot assigned colesbury Jun 20, 2024

colesbury deleted the gh-117511-pymutex branch June 20, 2024 15:29

colesbury added a commit to colesbury/cpython that referenced this pull request Jun 20, 2024

pythongh-117511: Make PyMutex public in the non-limited API (python#1…

b75dff5

…17731) (cherry picked from commit 3af7263)

colesbury added a commit to colesbury/cpython that referenced this pull request Jun 20, 2024

[3.13] pythongh-117511: Make PyMutex public in the non-limited API (p…

74c5b83

…ythonGH-117731) (cherry picked from commit 3af7263) Co-authored-by: Sam Gross <[email protected]>

colesbury added a commit to colesbury/cpython that referenced this pull request Jun 20, 2024

[3.13] pythongh-117511: Make PyMutex public in the non-limited API (p…

84a639b

…ythonGH-117731) (cherry picked from commit 3af7263) Co-authored-by: Sam Gross <[email protected]>

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 20, 2024

colesbury added a commit that referenced this pull request Jun 20, 2024

[3.13] gh-117511: Make PyMutex public in the non-limited API (GH-117731…

3cb6c4c

…) (#120800) (cherry picked from commit 3af7263)

mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024

pythongh-117511: Make PyMutex public in the non-limited API (python#1…

7c7eec3

…17731)

noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024

pythongh-117511: Make PyMutex public in the non-limited API (python#1…

81adac2

…17731)

estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024

pythongh-117511: Make PyMutex public in the non-limited API (python#1…

f85752b

…17731)

colesbury removed their assignment Jul 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-117511: Make PyMutex public in the non-limited API #117731

gh-117511: Make PyMutex public in the non-limited API #117731

colesbury commented Apr 10, 2024 •

edited

Loading

colesbury commented Jun 17, 2024

vstinner commented Jun 17, 2024

vstinner Jun 18, 2024

colesbury Jun 18, 2024

ericsnowcurrently Jun 18, 2024

vstinner Jun 19, 2024

vstinner Jun 18, 2024

colesbury Jun 18, 2024

vstinner Jun 18, 2024

vstinner Jun 19, 2024

vstinner left a comment

miss-islington-app bot commented Jun 20, 2024

miss-islington-app bot commented Jun 20, 2024

bedevere-app bot commented Jun 20, 2024

vstinner commented Jun 20, 2024

erlend-aasland commented Jun 27, 2024

gh-117511: Make PyMutex public in the non-limited API #117731

gh-117511: Make PyMutex public in the non-limited API #117731

Conversation

colesbury commented Apr 10, 2024 • edited Loading

colesbury commented Jun 17, 2024

vstinner commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

miss-islington-app bot commented Jun 20, 2024

miss-islington-app bot commented Jun 20, 2024

bedevere-app bot commented Jun 20, 2024

vstinner commented Jun 20, 2024

erlend-aasland commented Jun 27, 2024

colesbury commented Apr 10, 2024 •

edited

Loading