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-112213: Update _weakref module to use new AC feature #112250

Merged
merged 4 commits into from
Nov 19, 2023

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Nov 18, 2023

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.

Great! Much nicer than manually writing the Py_BEGIN/END_CRITICAL_SECTION

@corona10
Copy link
Member Author

corona10 commented Nov 19, 2023

@colesbury
One thing needs to be discussed:
Do we have to care about the thread-safe for is_dead_weakref?

is_dead_weakref(PyObject *value)
{
if (!PyWeakref_Check(value)) {
PyErr_SetString(PyExc_TypeError, "not a weakref");
return -1;
}
int is_dead;
Py_BEGIN_CRITICAL_SECTION(value);
is_dead = _PyWeakref_IS_DEAD(value);
Py_END_CRITICAL_SECTION();
return is_dead;
}

Or should it be handled from _PyDict_DelItemIf since it is only used for _PyDict_DelItemIf?
if (_PyDict_DelItemIf(dct, key, is_dead_weakref) < 0) {

cpython/Objects/dictobject.c

Lines 2062 to 2069 in 1a969b4

/* This function promises that the predicate -> deletion sequence is atomic
* (i.e. protected by the GIL), assuming the predicate itself doesn't
* release the GIL.
*/
int
_PyDict_DelItemIf(PyObject *op, PyObject *key,
int (*predicate)(PyObject *value))
{

If we decide not to handle from is_dead_weakref, we can remove critical section related code fully.

@corona10 corona10 merged commit 2bcc0f7 into python:main Nov 19, 2023
25 checks passed
@corona10 corona10 deleted the gh-112213-weakref branch November 19, 2023 02:07
@colesbury
Copy link
Contributor

@corona10, we'll need to handle the thread-safety issues in _PyWeakref_IS_DEAD().

@corona10
Copy link
Member Author

@corona10, we'll need to handle the thread-safety issues in _PyWeakref_IS_DEAD().

Okay let's handle it with upcoming PR :)

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