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.

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

[WIP] gh-123358: Update LOAD_DEREF to use stackref and atomic incref #124463

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

[WIP] gh-123358: Update LOAD_DEREF to use stackref and atomic incref #124463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d9e018c
gh-123358: Update LOAD_DEREF to use stackref and atomic incref
corona10 Sep 24, 2024
c293ba5
fix TSAN
corona10 Sep 25, 2024
1523a2d
Update TSAN
corona10 Sep 25, 2024
05392cb
Revert "fix TSAN"
corona10 Sep 25, 2024
66eec43
Reapply "fix TSAN"
corona10 Sep 25, 2024
4eab926
Address code review
corona10 Sep 25, 2024
bc751d7
Address code review
corona10 Sep 25, 2024
6dbbaca
Remove whole inline
corona10 Sep 26, 2024
d527e0a
Revert "Remove whole inline"
corona10 Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion Include/internal/pycore_cell.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#define Py_INTERNAL_CELL_H

#include "pycore_critical_section.h"
#include "pycore_object.h"

#ifdef __cplusplus
extern "C" {
Expand All @@ -19,7 +20,7 @@ PyCell_SwapTakeRef(PyCellObject *cell, PyObject *value)
PyObject *old_value;
Py_BEGIN_CRITICAL_SECTION(cell);
old_value = cell->ob_ref;
cell->ob_ref = value;
FT_ATOMIC_STORE_PTR_RELEASE(cell->ob_ref, value);
Py_END_CRITICAL_SECTION();
return old_value;
}
Expand All @@ -42,6 +43,31 @@ PyCell_GetRef(PyCellObject *cell)
return res;
}

static inline void
_PyCell_GetStackRef(PyCellObject *cell, _PyStackRef *value_addr)
{
PyObject *value;
#ifdef Py_GIL_DISABLED
value = _Py_atomic_load_ptr(&cell->ob_ref);
Copy link
Member Author

Choose a reason for hiding this comment

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

@colesbury Let's extract the utility function with separated PRs.

if (value != NULL) {
if (_Py_IsImmortal(value) || _PyObject_HasDeferredRefcount(value)) {
*value_addr = (_PyStackRef){ .bits = (uintptr_t)value | Py_TAG_DEFERRED };
return;
}
if (_Py_TryIncrefCompare(&cell->ob_ref, value)) {
*value_addr = _PyStackRef_FromPyObjectSteal(value);
return;
}
}
#endif
value = PyCell_GetRef(cell);
if (value == NULL) {
*value_addr = PyStackRef_NULL;
return;
}
*value_addr = PyStackRef_FromPyObjectSteal(value);
}

#ifdef __cplusplus
}
#endif
Expand Down
2 changes: 1 addition & 1 deletion Include/internal/pycore_opcode_metadata.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Include/internal/pycore_uop_metadata.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 3 additions & 4 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1647,14 +1647,13 @@ dummy_func(
value = PyStackRef_FromPyObjectSteal(value_o);
}

inst(LOAD_DEREF, ( -- value)) {
inst(LOAD_DEREF, ( -- value[1])) {
PyCellObject *cell = (PyCellObject *)PyStackRef_AsPyObjectBorrow(GETLOCAL(oparg));
PyObject *value_o = PyCell_GetRef(cell);
if (value_o == NULL) {
_PyCell_GetStackRef(cell, value);
if (PyStackRef_IsNull(*value)) {
_PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg);
ERROR_IF(true, error);
}
value = PyStackRef_FromPyObjectSteal(value_o);
}

inst(STORE_DEREF, (v --)) {
Expand Down
9 changes: 4 additions & 5 deletions Python/executor_cases.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 4 additions & 5 deletions Python/generated_cases.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 5 additions & 3 deletions Python/optimizer_cases.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Tools/tsan/suppressions_free_threading.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ race_top:tstate_is_freed
race_top:type_modified_unlocked
race_top:write_thread_id
race_top:PyThreadState_Clear
# see: https://github.com/python/cpython/issues/117721
race_top:lock_PyThread_release_lock
Copy link
Member Author

Choose a reason for hiding this comment

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

@colesbury found that there is a possible issue from

cpython/Lib/test/lock_tests.py

Lines 117 to 121 in 5e7eba0

def wait_phase(self, phase, expected):
for _ in support.sleeping_retry(support.SHORT_TIMEOUT):
if len(phase) >= expected:
break
self.assertEqual(len(phase), expected)
which cause this TSAN error. We should fix it with a separate PR.

# Only seen on macOS, sample: https://gist.github.com/aisk/dda53f5d494a4556c35dde1fce03259c
race_top:set_default_allocator_unlocked

Expand Down
Loading