Skip to content

Conversation

@corona10
Copy link
Member

@corona10corona10 commented Nov 17, 2023

@corona10corona10 changed the title gh-111926 Update _weakref to be threadsafe in --disable-gil buildgh-111926: Update _weakref to be threadsafe in --disable-gil buildNov 17, 2023
@corona10corona10 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 buildNov 17, 2023
@corona10corona10 changed the title [WIP] gh-111926: Update _weakref to be threadsafe in --disable-gil buildgh-111926: Update _weakref to be threadsafe in --disable-gil buildNov 17, 2023
@corona10corona10 marked this pull request as ready for review November 17, 2023 06:04
@corona10
Copy link
MemberAuthor

I am going to work on weakref object separately.

@corona10
Copy link
MemberAuthor

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

@corona10corona10 reopened this Nov 17, 2023
@corona10
Copy link
MemberAuthor

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

@colesburycolesbury 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

@corona10corona10 merged commit 0566ab9 into python:mainNov 18, 2023
@corona10corona10 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 freeto 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

@corona10@colesbury