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 16 commits into
base: main
Choose a base branch
from
+34 −13
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
16 commits
Select commit Hold shift + click to select a range
b834dc5
gh-123358: Update LOAD_DEREF to use stackref and atomic incref
corona10 Sep 24, 2024
ab34c92
fix TSAN
corona10 Sep 25, 2024
536b6fc
Update TSAN
corona10 Sep 25, 2024
05f0cf9
Revert "fix TSAN"
corona10 Sep 25, 2024
617c759
Reapply "fix TSAN"
corona10 Sep 25, 2024
1b2b618
Address code review
corona10 Sep 25, 2024
cea6813
Address code review
corona10 Sep 25, 2024
a3a89f0
Remove whole inline
corona10 Sep 26, 2024
4ad83c1
Revert "Remove whole inline"
corona10 Sep 26, 2024
ee3113d
Address code review from Mark
corona10 Sep 27, 2024
a7c7181
[WIP] Comment out _Py_TryIncrefCompare
corona10 Sep 27, 2024
90ef0d8
test SwitchToThread()
corona10 Sep 27, 2024
f16009c
test fence
corona10 Sep 28, 2024
9edf409
Comment out immortal and deferred case
corona10 Sep 28, 2024
1c8e923
test
corona10 Sep 28, 2024
2ec1278
Test _Py_TryIncrefFast only
corona10 Oct 3, 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
25 changes: 24 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,28 @@ PyCell_GetRef(PyCellObject *cell)
return res;
}

static inline
_PyStackRef _PyCell_GetStackRef(PyCellObject *cell)
{
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)) {
return (_PyStackRef){ .bits = (uintptr_t)value | Py_TAG_DEFERRED };
}
if (_Py_TryIncrefFast(value)) {
return _PyStackRef_FromPyObjectSteal(value);
}
}
#endif
value = PyCell_GetRef(cell);
if (value == NULL) {
return PyStackRef_NULL;
}
return PyStackRef_FromPyObjectSteal(value);
}

#ifdef __cplusplus
}
#endif
Expand Down
1 change: 0 additions & 1 deletion Include/internal/pycore_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ extern "C" {
#include "pycore_pystate.h" // _PyInterpreterState_GET()
#include "pycore_uniqueid.h" // _PyType_IncrefSlow


Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if in _Py_TryIncRefShared we could just do an atomic add to the ob_ref_shared if the object is already marked as weakref'd? E.g:

Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&op->ob_ref_shared);
for (;;) {
    if ((shared & _Py_REF_SHARED_FLAG_MASK) == _Py_REF_MAYBE_WEAKREF) {
        _Py_atomic_add_ssize(&op->ob_ref_shared, (1 << _Py_REF_SHARED_SHIFT));
       return 1;
    }
}

That might avoid the threads spinning on updating the value when the value is in heavy contention and instead all of the atomic operations should just slowly proceed.

#define _Py_IMMORTAL_REFCNT_LOOSE ((_Py_IMMORTAL_REFCNT >> 1) + 1)

// This value is added to `ob_ref_shared` for objects that use deferred
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.

5 changes: 2 additions & 3 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1649,12 +1649,11 @@ dummy_func(

inst(LOAD_DEREF, ( -- value)) {
corona10 marked this conversation as resolved.
Show resolved Hide resolved
PyCellObject *cell = (PyCellObject *)PyStackRef_AsPyObjectBorrow(GETLOCAL(oparg));
PyObject *value_o = PyCell_GetRef(cell);
if (value_o == NULL) {
value = _PyCell_GetStackRef(cell);
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
5 changes: 2 additions & 3 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.

5 changes: 2 additions & 3 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.

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