Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
corona10 merged 11 commits intopython:mainfromcorona10:gh-112069-PySet_NextEntry
Apr 18, 2024

Conversation

@corona10
Copy link
Member

@corona10corona10 commentedApr 17, 2024•
edited
Loading

  • Add _PySet_NextEntryRef to be thread-safe.
  • Use FrozenSet if possible to use _PySet_NextEntry without locking.

@corona10
Copy link
MemberAuthor

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

OKay, it's too slow.

@corona10corona10 marked this pull request as draftApril 17, 2024 15:17
Copy link
Contributor

@colesburycolesbury 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 ofwhile(_PySet_NextEntry(...)) is generally what needs to be thread-safe. For example, the comment inset_next says:

* 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 anddictobject.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 reacted with thumbs up emoji
@corona10corona10 changed the titlegh-112069: Make _PySet_NextEntry to be thread-safe.[WIP] gh-112069: Make _PySet_NextEntry to be thread-safe.Apr 17, 2024
@corona10corona10 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
@corona10corona10 marked this pull request as ready for reviewApril 17, 2024 17:55
@corona10corona10 added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelApr 17, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@corona10 for commit5d57a56 🤖

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

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelApr 17, 2024
@corona10
Copy link
MemberAuthor

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>                       ^

Copy link
Contributor

@colesburycolesbury 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 reacted with thumbs up emoji
@corona10corona10 changed the titlegh-112069: Make _PySet_NextEntry to be thread-safe.[WIP] gh-112069: Make _PySet_NextEntry to be thread-safe.Apr 18, 2024
@corona10corona10 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
@corona10corona10 added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelApr 18, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@corona10 for commit99fb7e6 🤖

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

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

🤖 New build scheduled with the buildbot fleet by@corona10 for commit53f949a 🤖

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

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelApr 18, 2024
@corona10corona10force-pushed thegh-112069-PySet_NextEntry branch from53f949a to99fb7e6CompareApril 18, 2024 03:50
@corona10corona10 added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelApr 18, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@corona10 for commit99fb7e6 🤖

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

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelApr 18, 2024
@corona10
Copy link
MemberAuthor

test_compile leaked [2, 2, 2] references, sum=6test_compile leaked [4, 4, 4] memory blocks, sum=12

Need to check.

@corona10corona10 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
MemberAuthor

corona10 commentedApr 18, 2024•
edited
Loading

test_compile:#118023
test_logging:#102412 (comment)

So leaks are not related to this PR.

Eclips4 reacted with thumbs up emoji

@corona10corona10 changed the titlegh-112069: Make _PySet_NextEntry to be thread-safe.gh-112069: Add _PySet_NextEntryRef to be thread-safe.Apr 18, 2024
@corona10corona10 merged commit94444ea intopython:mainApr 18, 2024
@corona10corona10 deleted the gh-112069-PySet_NextEntry branchApril 18, 2024 15:18
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@colesburycolesburycolesbury approved these changes

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

@iritkatrieliritkatrielAwaiting requested review from iritkatriel

@methanemethaneAwaiting requested review from methanemethane is a code owner

@rhettingerrhettingerAwaiting requested review from rhettingerrhettinger is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@corona10@bedevere-bot@colesbury
[8]ページ先頭
©2009-2025 Movatter.jp