Skip to content

gh-123358: Use _PyStackRef in LOAD_DEREF #130064

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

Merged
merged 5 commits into from
Mar 26, 2025
Merged

gh-123358: Use _PyStackRef in LOAD_DEREF #130064

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57526e8
gh-123358: Update LOAD_DEREF to use stackref and atomic incref
corona10 Sep 24, 2024
e4c7608
Use _Py_TryIncrefCompareStackRef and minor formatting
colesbury Feb 13, 2025
70a09df
Merge branch 'main' into gh-123358-load-deref
colesbury Mar 24, 2025
4b75c14
Use _Py_XNewRefWithLock
colesbury Mar 24, 2025
c5c689b
Avoid slow downs in test_free_threading
colesbury Mar 25, 2025
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
24 changes: 23 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);
Copy link
Member

Choose a reason for hiding this comment

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

Naiive question: out of curiosity, why does this need release? I don't see the acquire anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We generally want to use at least release when storing pointers that may be loaded concurrently. This ensures that previously written data is visible before the store of value to cell->ob_ref.

For example, we probably initialize value's type earlier in the program execution:

value->ob_type = &PyFloat_Type;  // for example
...
FT_ATOMIC_STORE_PTR_RELEASE(cell->ob_ref, value);

It's really important that value's ob_type field is visible before the write of value to cell->ob_ref or a reader might see some previous, garbage data for ob_type.

The load below uses seq-cst, which is at least as strong as acquire. The minimum in the C11/C++11 memory model would be consume for the load (for data dependencies), but no compiler implements that -- they all just upgrade it to "acquire", so it's kind of a mess.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the thorough explanation!

Py_END_CRITICAL_SECTION();
return old_value;
}
Expand All @@ -42,6 +43,27 @@ 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);
if (value == NULL) {
return PyStackRef_NULL;
}
_PyStackRef ref;
if (_Py_TryIncrefCompareStackRef(&cell->ob_ref, value, &ref)) {
return ref;
}
#endif
value = PyCell_GetRef(cell);
if (value == NULL) {
return PyStackRef_NULL;
}
return 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.

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 @@ -1811,12 +1811,11 @@ dummy_func(

inst(LOAD_DEREF, ( -- value)) {
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
10 changes: 7 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.

10 changes: 7 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.

Loading