Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-111926: Update _weakref to be threadsafe in --disable-gil build #112189

Merged
merged 6 commits into from
Nov 18, 2023

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Nov 17, 2023

@corona10 corona10 changed the title gh-111926 Update _weakref to be threadsafe in --disable-gil build gh-111926: Update _weakref to be threadsafe in --disable-gil build Nov 17, 2023
@corona10 corona10 changed the title gh-111926: Update _weakref to be threadsafe in --disable-gil build [WIP] gh-111926: Update _weakref to be threadsafe in --disable-gil build Nov 17, 2023
@corona10 corona10 changed the title [WIP] gh-111926: Update _weakref to be threadsafe in --disable-gil build gh-111926: Update _weakref to be threadsafe in --disable-gil build Nov 17, 2023
@corona10 corona10 marked this pull request as ready for review November 17, 2023 06:04
@corona10 corona10 requested a review from colesbury November 17, 2023 06:04
@corona10
Copy link
Member Author

I am going to work on weakref object separately.

@corona10
Copy link
Member Author

There was some misunderstanding with my PR, I will re-send a patch for this :)

@corona10 corona10 closed this Nov 17, 2023
@corona10 corona10 reopened this Nov 17, 2023
@corona10
Copy link
Member Author

corona10 commented Nov 17, 2023

@colesbury Hmm, I am not sure this was what you intended from #111926.

IIUC, There might be two kinds of task that should be done

  • From the side of the modules: This PR might be an example.
  • From the weakreference object side: For example, PyWeakref_GetRef or _PyWeakref_GET_REF should be updated to be thread-safe?

Please let me know if there is my misunderstanding.
The reason why I don't use AC for this PR is that AC generate the critical section based on module object rather than target object.

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.

This looks good, other than the issue I mention below.

We will want to make both the module-level code (like this) and the operations on the weakref object thread-safe. I think it makes sense to split them up as separate PRs as you are doing.

Regarding Argument Clinic: maybe the @critical_section directive should take an optional argument specifying which parameter name to lock. For now, I think writing the critical sections manually is fine.

The tricky bit will be making _PyWeakref_GET_REF thread-safe because the lock is in the pointed-to object (ref->wr_object), but that might change or even get deallocated. I think that will need an operation _Py_TryXFetchRef from nogil-3.12 that isn't upstreamed yet. It may be worth doing the other bits and leaving _PyWeakref_GET_REF for last

Modules/_weakref.c Outdated Show resolved Hide resolved
@corona10 corona10 requested a review from colesbury November 18, 2023 01:32
@corona10 corona10 merged commit 0566ab9 into python:main Nov 18, 2023
@corona10 corona10 deleted the gh-111926-weakref branch November 18, 2023 07:09
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
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