8000 gh-112069: Add _PySet_NextEntryRef to be thread-safe. by corona10 · Pull Request #117990 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

8000 gh-112069: Add _PySet_NextEntryRef to be thread-safe. #117990

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Apr 18, 2024
Merged

gh-112069: Add _PySet_NextEntryRef to be thread-safe. #117990

merged 11 commits into from
Apr 18, 2024

Conversation

corona10
Copy link
Member
@corona10 corona10 commented Apr 17, 2024 •
edited
Loading
  • Add _PySet_NextEntryRef to be thread-safe.
  • Use FrozenSet if possible to use _PySet_NextEntry without locking.

@corona10 corona10 requested a review from colesbury April 17, 2024 14:29
@bedevere-app bedevere-app bot mentioned this pull request Apr 17, 2024
Closed
3 tasks
@corona10
Copy link
Member Author

Tests / Thread sanitizer (free-threading) / Thread sanitizer (pull_request)

OKay, it's too slow.

@corona10 corona10 marked this pull request as draft April 17, 2024 15:17
colesbury
colesbury reviewed Apr 17, 2024
View reviewed changes
Copy link
Contributor
@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

I'm not sure what the right approach here is, but as written _PySet_NextEntry still returns a borrowed reference, so the cases that were not thread-safe are probably still not thread-safe.

I don't think it makes sense to try to make individual calls to _PySet_NextEntry thread-safe. The whole loop of while(_PySet_NextEntry(...)) is generally what needs to be thread-safe. For example, the comment in set_next says:

cpython/Objects/setobject.c

Lines 477 to 478 in fccedbd

* CAUTION: In general, it isn't safe to use set_next in a loop that
* mutates the table.

I think we should add locking in the use sites of _PySet_NextEntry as necessary. Most look like they're already okay to me: either they already do locking (listobject.c and dictobject.c) or operate on private or immutable data (pylifecycle.c, codeobject.c).

The two cases that concern me and may need locking are:

  • _pickle.c
  • marshal.c

@corona10 corona10 changed the title gh-112069: Make _PySet_NextEntry to be thread-safe. [WIP] gh-112069: Make _PySet_NextEntry to be thread-safe. Apr 17, 2024
@corona10 corona10 changed the title [WIP] gh-112069: Make _PySet_NextEntry to be thread-safe. gh-112069: Make _PySet_NextEntry to be thread-safe. Apr 17, 2024
@corona10 corona10 marked this pull request as ready for review April 17, 2024 17:55
@corona10 corona10 requested a review from colesbury April 17, 2024 17:55
@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 17, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 5d57a56 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 17, 2024
@corona10
Copy link
Member Author

buildbot/AMD64 RHEL7 Refleaks PR: Unrelated

In file included from ./Include/internal/mimalloc/mimalloc/types.h:24:0,
                 from ./Include/internal/pycore_mimalloc.h:40,
                 from ./Include/internal/pycore_interp.h:30,
                 from ./Include/internal/pycore_runtime.h:17,
                 from ./Include/internal/pycore_pystate.h:12,
                 from Parser/myreadline.c:14:
./Include/internal/mimalloc/mimalloc/atomic.h:39:23: fatal error: stdatomic.h: No such file or directory
 #include <stdatomic.h>
                       ^
compilation terminated.
In file included from ./Include/internal/mimalloc/mimalloc/types.h:24:0,
                 from ./Include/internal/pycore_mimalloc.h:40,
                 from ./Include/internal/pycore_interp.h:30,
                 from ./Include/internal/pycore_runtime.h:17,
                 from ./Include/internal/pycore_long.h:13,
                 from Objects/boolobject.c:4:
./Include/internal/mimalloc/mimalloc/atomic.h:39:23: fatal error: stdatomic.h: No such file or directory
 #include <stdatomic.h>
                       ^

colesbury
colesbury reviewed Apr 17, 2024
View reviewed changes
Copy link
Contributor
@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

  • We should not change the reference count behavior of _PySet_NextEntry as it's exposed publicly and used outside of CPython (like in Cython). If needed, add a new function _PySet_NextEntryRef.
  • I don't think _PyFrozenSet_NextEntry is needed. _PySet_NextEntry (or _PySet_NextEntryRef) looks sufficient.

@corona10 corona10 changed the title gh-112069: Make _PySet_NextEntry to be thread-safe. [WIP] gh-112069: Make _PySet_NextEntry to be thread-safe. Apr 18, 2024
@corona10 corona10 changed the title [WIP] gh-112069: Make _PySet_NextEntry to be thread-safe. gh-112069: Make _PySet_NextEntry to be thread-safe. Apr 18, 2024
@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 18, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 99fb7e6 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 18, 2024
@corona10 corona10 changed the title gh-112069: Make _PySet_NextEntry to be thread-safe. [WIP] gh-112069: Make _PySet_NextEntry to be thread-safe. Apr 18, 2024
@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 18, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 53f949a 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 18, 2024
@corona10 corona10 force-pushed the gh-112069-PySet_NextEntry branch from 53f949a to 99fb7e6 Compare April 18, 2024 03:50
@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 18, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 99fb7e6 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 18, 2024
@corona10
Copy link
Member Author 
test_compile leaked [2, 2, 2] references, sum=6
test_compile leaked [4, 4, 4] memory blocks, sum=12

Need to check.

@corona10 corona10 changed the title [WIP] gh-112069: Make _PySet_NextEntry to be thread-safe. gh-112069: Make _PySet_NextEntry to be thread-safe. Apr 18, 2024
@corona10
Copy link
8000
Member Author
corona10 commented Apr 18, 2024 •
edited
Loading

test_compile: #118023
test_logging: #102412 (comment)

So leaks are not related to this PR.

@corona10 corona10 requested a review from colesbury April 18, 2024 05:47
@corona10 corona10 changed the title gh-112069: Make _PySet_NextEntry to be thread-safe. gh-112069: Add _PySet_NextEntryRef to be thread-safe. Apr 18, 2024
@corona10 corona10 merged commit 94444ea into python:main Apr 18, 2024
@corona10 corona10 deleted the gh-112069-PySet_NextEntry branch April 18, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colesbury colesbury colesbury approved these changes

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

@iritkatriel iritkatriel Awaiting requested review from iritkatriel

@methane methane Awaiting requested review from methane methane is a code owner

@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

Assignees
No one assigned
Labels
skip news topic-free-threading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@corona10 @bedevere-bot @colesbury
0