From c8ccd02bbcec0ae12409efcd7beececaa436281a Mon Sep 17 00:00:00 2001 From: Bogdan Romanyuk Date: Thu, 26 Dec 2024 23:36:38 +0300 Subject: [PATCH 1/4] Implement atomic load and critical section in C code --- Objects/object.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Objects/object.c b/Objects/object.c index d584414c559b9d..b1baceb4ebe490 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1715,10 +1715,16 @@ _PyObject_GenericGetAttrWithDict(PyObject *obj, PyObject *name, dict = (PyObject *)_PyObject_GetManagedDict(obj); } else { + Py_BEGIN_CRITICAL_SECTION(obj); PyObject **dictptr = _PyObject_ComputedDictPointer(obj); if (dictptr) { +#ifdef DISABLE_GIL + dict = (PyObject *)_Py_atomic_load_ptr(dictptr); +#else dict = *dictptr; +#endif } + Py_END_CRITICAL_SECTION(); } } if (dict != NULL) { From 3d0d584d1f388af0bfdb97136329ef173259f7a2 Mon Sep 17 00:00:00 2001 From: Bogdan Romanyuk Date: Fri, 27 Dec 2024 02:17:22 +0300 Subject: [PATCH 2/4] Add tests --- Lib/test/test_free_threading/test_races.py | 19 +++++++++++++++++++ Objects/object.c | 8 ++++---- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_free_threading/test_races.py b/Lib/test/test_free_threading/test_races.py index 69982558a067a5..0e7be74006134b 100644 --- a/Lib/test/test_free_threading/test_races.py +++ b/Lib/test/test_free_threading/test_races.py @@ -270,6 +270,25 @@ def mutate(): do_race(set_value, mutate) + def test_generic_getattr(self): + """Test generic attribute load""" + import concurrent.futures + + num_threads = 100 + + + def closure(b, o): + b.wait() + getattr(o, "foo", None) + o.foo = 42 + + with concurrent.futures.ThreadPoolExecutor(max_workers=num_threads) as executor: + for _ in range(100): + b = threading.Barrier(num_threads) + o = functools.partial(lambda x: x, 42) + for _ in range(num_threads): + executor.submit(functools.partial(closure, b, o)) + if __name__ == "__main__": unittest.main() diff --git a/Objects/object.c b/Objects/object.c index b1baceb4ebe490..6c0aefd281c44b 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1716,13 +1716,13 @@ _PyObject_GenericGetAttrWithDict(PyObject *obj, PyObject *name, } else { Py_BEGIN_CRITICAL_SECTION(obj); - PyObject **dictptr = _PyObject_ComputedDictPointer(obj); - if (dictptr) { #ifdef DISABLE_GIL - dict = (PyObject *)_Py_atomic_load_ptr(dictptr); + PyObject **dictptr = _Py_atomic_load_ptr(_PyObject_ComputedDictPointer(obj)); #else - dict = *dictptr; + PyObject **dictptr = _PyObject_ComputedDictPointer(obj); #endif + if (dictptr) { + dict = *dictptr; } Py_END_CRITICAL_SECTION(); } From 7227e3ed103e10f13251f60a1c75ec69def704e9 Mon Sep 17 00:00:00 2001 From: Bogdan Romanyuk Date: Sat, 28 Dec 2024 02:10:07 +0300 Subject: [PATCH 3/4] Address the review --- Objects/object.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/Objects/object.c b/Objects/object.c index 6c0aefd281c44b..4c30257ca26938 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1715,16 +1715,14 @@ _PyObject_GenericGetAttrWithDict(PyObject *obj, PyObject *name, dict = (PyObject *)_PyObject_GetManagedDict(obj); } else { - Py_BEGIN_CRITICAL_SECTION(obj); -#ifdef DISABLE_GIL - PyObject **dictptr = _Py_atomic_load_ptr(_PyObject_ComputedDictPointer(obj)); -#else PyObject **dictptr = _PyObject_ComputedDictPointer(obj); -#endif if (dictptr) { +#ifdef Py_GIL_DISABLED + dict = _Py_atomic_load_ptr_acquire(dictptr); +#else dict = *dictptr; +#endif } - Py_END_CRITICAL_SECTION(); } } if (dict != NULL) { From b92759a07bf89fa67381d6140a7445f7a44f4d6a Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 30 Dec 2024 20:14:32 +0000 Subject: [PATCH 4/4] Remove test case. It wasn't reporting any TSAN failures before the PR. --- Lib/test/test_free_threading/test_races.py | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/Lib/test/test_free_threading/test_races.py b/Lib/test/test_free_threading/test_races.py index 0e7be74006134b..69982558a067a5 100644 --- a/Lib/test/test_free_threading/test_races.py +++ b/Lib/test/test_free_threading/test_races.py @@ -270,25 +270,6 @@ def mutate(): do_race(set_value, mutate) - def test_generic_getattr(self): - """Test generic attribute load""" - import concurrent.futures - - num_threads = 100 - - - def closure(b, o): - b.wait() - getattr(o, "foo", None) - o.foo = 42 - - with concurrent.futures.ThreadPoolExecutor(max_workers=num_threads) as executor: - for _ in range(100): - b = threading.Barrier(num_threads) - o = functools.partial(lambda x: x, 42) - for _ in range(num_threads): - executor.submit(functools.partial(closure, b, o)) - if __name__ == "__main__": unittest.main()