GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-149965: rename `ptr_wise_atomic_memmove` to `_PyObject_ptr_wise_atomic_memmove` by clin1234 · Pull Request #151271 · python/cpython

Conversation

@clin1234

@clin1234 clin1234 commented Jun 10, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

See also capi-workgroup/decisions#106

  • Issue: Improve thread-safety in ElementObjects #149965

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added the awaiting review label Jun 10, 2026
@bedevere-app bedevere-app Bot mentioned this pull request Jun 10, 2026
Improve thread-safety in ElementObjects #149965
Open
@clin1234 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
@clin1234

clin1234 commented 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ZeroIntensity

ZeroIntensity commented Jun 10, 2026

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@clin1234
clin1234 requested review from a team, AA-Turner, emmatyping, erlend-aasland and itamaro as code owners June 10, 2026 20:20
@clin1234
clin1234 marked this pull request as draft June 11, 2026 00:58
@clin1234
clin1234 marked this pull request as ready for review June 11, 2026 00:58
@bedevere-app bedevere-app Bot added awaiting review and removed awaiting review labels Jun 11, 2026
@clin1234

clin1234 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

@ZeroIntensity, mind taking a look again?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ZeroIntensity
ZeroIntensity reviewed Jun 11, 2026
View reviewed changes

@ZeroIntensity ZeroIntensity left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you add a multithreaded test stressing this path?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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,

@ZeroIntensity ZeroIntensity Jun 11, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_elementtree.c Outdated
Comment on lines +1943 to +1944
_Py_ptr_wise_atomic_memmove(
(PyObject *)self,

@ZeroIntensity ZeroIntensity Jun 11, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@clin1234

clin1234 commented Jun 11, 2026

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@clin1234 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
@clin1234

clin1234 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Do you think a News fragment is needed here?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@clin1234
clin1234 requested a review from ZeroIntensity June 11, 2026 21:04
@ZeroIntensity

ZeroIntensity commented Jun 11, 2026

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@clin1234

clin1234 commented Jun 12, 2026

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ZeroIntensity

ZeroIntensity commented Jun 12, 2026

Copy link
Copy Markdown
Member

Add something to test_xml_etree.py or test_xml_etree_c.py.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 90 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Sep 17, 2026
@clin1234
clin1234 force-pushed the patch-4 branch from fbb7cfb to 8bf4470 Compare October 11, 2026 09:51
@python-cla-bot

python-cla-bot Bot commented Oct 11, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@clin1234
clin1234 force-pushed the patch-4 branch 3 times, most recently from a3a98b2 to 6c15025 Compare October 11, 2026 11:47
clin1234 and others added 14 commits October 11, 2026 09:56
@clin1234
Make test shut up check-c-globals.py
0b804bb
@clin1234
Drop const in test_objs
d3fc80b
@clin1234
Revert previous changes
e2d17d2
@blurb-it @clin1234
📜🤖 Added by blurb_it.
50d71cf
@clin1234
Delete Misc/NEWS.d/next/Core_and_Builtins/2026-06-10-14-01-11.gh-issu…
ab0750e 
…e-149965.edk2ut.rst
@clin1234
Drop unit tests for now
44c5f6c
@clin1234
Don't exclude weakref for testing
c4db785
@clin1234
Actually drop unit test
cf79ca0
@clin1234
Rename to _PyObject_ptr_wise_atomic_memmove
98a544b
@blurb-it @clin1234
📜🤖 Added by blurb_it.
d079b0d
@clin1234
Backtick
5944e34
@clin1234
Rebase mishaps
f526c60
@clin1234
Backtick
0daec9c
@clin1234
Finally use func declaration in internal header
1535db6
@clin1234
clin1234 force-pushed the patch-4 branch from 4669823 to 1535db6 Compare October 11, 2026 13:56
@clin1234

clin1234 commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Ping @ZeroIntensity @erlend-aasland @AA-Turner @emmatyping @itamaro
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ZeroIntensity
ZeroIntensity reviewed Oct 13, 2026
View reviewed changes

@ZeroIntensity ZeroIntensity left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Per my previous comments, please add tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Core_and_Builtins/2026-06-12-19-38-28.gh-issue-149965.w5eiS4.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/Setup.stdlib.in Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_elementtree.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
clin1234 added 11 commits October 13, 2026 08:39
@clin1234
Update 2026-06-12-19-38-28.gh-issue-149965.w5eiS4.rst
31298e0
@clin1234
Update comments and formatting in Setup.stdlib.in
4d1ca96
@clin1234
Comment out lines in Setup.stdlib.in for clarity
8545496
@clin1234
Fix comment formatting in Setup.stdlib.in
a2f13f1
@clin1234
Update _elementtree.c
1e8267c
@clin1234
Remove lock assertions in listobject functions
bdb231c 
Removed assertions for object lock in list operations.
@clin1234
Add assertion for object lock in memory move
4c75be2
@clin1234
Update pycore_object.h
50fa059
@clin1234
Unit tests for etree
4f8427f
@clin1234
Update _elementtree.c
1f3422f
@clin1234
Update _elementtree.c
3efd47c
@clin1234
clin1234 requested a review from ZeroIntensity October 13, 2026 18:15
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Oct 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland
@AA-Turner AA-Turner Awaiting requested review from AA-Turner
@emmatyping emmatyping Awaiting requested review from emmatyping
@itamaro itamaro Awaiting requested review from itamaro
@ZeroIntensity ZeroIntensity Awaiting requested review from ZeroIntensity

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@clin1234 @ZeroIntensity
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.