FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-149965: rename `ptr_wise_atomic_memmove` to `_PyObject_ptr_wise_atomic_memmove` by clin1234 · Pull Request #151271 · python/cpython · GitHub

/ cpython Public

gh-149965: rename ptr_wise_atomic_memmove to _PyObject_ptr_wise_atomic_memmove - #151271

Open
clin1234 wants to merge 21 commits into
python:mainfrom
clin1234:patch-4
Open

gh-149965: rename ptr_wise_atomic_memmove to _PyObject_ptr_wise_atomic_memmove#151271
clin1234 wants to merge 21 commits into
python:mainfrom
clin1234:patch-4

Conversation

clin1234 commented Jun 10, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

clin1234 and others added 15 commits June 10, 2026 09:55
…FW3ZD.rst

Co-authored-by: Sergey Miryanov <sergey.miryanov@gmail.com>
…ternal helper and add unit tests

Extract the duplicated static `ptr_wise_atomic_memmove` from
`Modules/_elementtree.c` and `Objects/listobject.c` into a shared
`static inline _Py_ptr_wise_atomic_memmove(PyObject *a, ...)` in
`Include/internal/pycore_object.h`.  The first parameter is generalised
from a type-specific pointer to `PyObject *` since only `PyObject *`-
level operations (`_Py_IsOwnedByCurrentThread`, `_PyObject_GC_IS_SHARED`)
were ever performed on it.

`listobject.c` previously embedded `_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED`
inside the local helper.  That assertion is preserved by moving it
explicitly to each of the four call sites in `listobject.c` (which are
all called under a critical section).  `_elementtree.c`'s open question
about whether a critical section is needed remains unanswered, so no
assertion is added there.

Unit tests are added in `Modules/_testinternalcapi/test_ptr_wise_memmove.c`
covering: dest < src (forward copy), dest > src (backward copy),
dest == src (no-op), overlapping ranges, and the single-owner fast path.
The single-owner test explicitly clears the GC SHARED bit to guard against
freelist reuse leaving the bit set from a sibling test.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Commented out critical section assertion for ElementObject.
clin1234 changed the title gh-149965: ptr_wise_atomic_memmove use within element_ass_subscr gh-149965: use ptr_wise_atomic_memmove within element_ass_subscr Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

@ZeroIntensity, I've reverted the header change and unit test additions, so my current PR is focused on only one file.

Copy link
Copy Markdown
Member

I think you misunderstood what I meant. You can move the function to a header (and in fact, you should, because otherwise we're duplicating code), but it was weird to do it in its own PR. Just combine this PR with #151255 and it'll be fine.

clin1234 marked this pull request as draft June 11, 2026 00:58
clin1234 marked this pull request as ready for review June 11, 2026 00:58

Copy link
Copy Markdown
Contributor Author

@ZeroIntensity, mind taking a look again?

ZeroIntensity left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you add a multithreaded test stressing this path?

Comment thread Include/internal/pycore_object.h Outdated
// Pointer-by-pointer memmove for PyObject** arrays that is safe for shared
// objects in Py_GIL_DISABLED builds. Locking is the caller's responsibility.
static inline void
_Py_ptr_wise_atomic_memmove(PyObject *a, PyObject **dest, PyObject **src,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We should probably prefix this with _PyObject instead of plain _Py.

Comment thread Modules/_elementtree.c Outdated
Comment on lines +1943 to +1944
_Py_ptr_wise_atomic_memmove(
(PyObject *)self,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

To avoid needing to cast on every use, we can turn this into a macro that uses _PyObject_CAST. Here's an outline:

#define _Py_ptr_wise_atomic_memmove(a, dest, src, n) _Py_ptr_wise_atomic_memmove(_PyObject_CAST(a), dest, src, n)

Copy link
Copy Markdown
Contributor Author

Can you add a multithreaded test stressing this path?

Where would the multithreaded test lie within the source tree, considering that it's an internal function not exported to the public headers?

clin1234 changed the title gh-149965: use ptr_wise_atomic_memmove within element_ass_subscr gh-149965: rename ptr_wise_atomic_memmove to _PyObject_ptr_wise_atomic_memmove Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Do you think a News fragment is needed here?

clin1234 requested a review from ZeroIntensity June 11, 2026 21:04

Copy link
Copy Markdown
Member

Where would the multithreaded test lie within the source tree, considering that it's an internal function not exported to the public headers?

I meant for the _elementtree code that you're fixing.

Do you think a News fragment is needed here?

Yes; xml.etree.ElementTree is a user-facing module.

Copy link
Copy Markdown
Contributor Author

Where would the multithreaded test lie within the source tree, considering that it's an internal function not exported to the public headers?

I meant for the _elementtree code that you're fixing.

Do you think a News fragment is needed here?

Yes; xml.etree.ElementTree is a user-facing module.

Any pointers on creating such a multi-threaded test in the native _elementtree module?

Copy link
Copy Markdown
Member

Add something to test_xml_etree.py or test_xml_etree_c.py.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL