From f9d44d1796dc6c77caae986dc8749ec1aa507ea0 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Fri, 23 Aug 2024 22:17:30 +0200 Subject: [PATCH 01/14] Make builtin zip method safe under free-threading --- Lib/test/test_zip_threading.py | 34 ++++++++++++++++++++++++++++++++++ Python/bltinmodule.c | 7 ++++++- 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 Lib/test/test_zip_threading.py diff --git a/Lib/test/test_zip_threading.py b/Lib/test/test_zip_threading.py new file mode 100644 index 00000000000000..b569715f1f3155 --- /dev/null +++ b/Lib/test/test_zip_threading.py @@ -0,0 +1,34 @@ +import unittest +from threading import Thread + +from test.support import threading_helper + + +class ZipThreading(unittest.TestCase): + @staticmethod + def work(enum): + while True: + try: + next(enum) + except StopIteration: + break + + @threading_helper.reap_threads + @threading_helper.requires_working_threading() + def test_threading(self): + number_of_threads = 8 + number_of_iterations = 10 + n = 40_000 + enum = zip(range(n), range(n)) + for _ in range(number_of_iterations): + worker_threads = [] + for ii in range(number_of_threads): + worker_threads.append(Thread(target=self.work, args=[enum,])) + _ = [t.start() for t in worker_threads] + _ = [t.join() for t in worker_threads] + +if __name__=='__main__': + z=ZipThreading() + for kk in range(10): + print(kk) + z.test_threading() diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index 99ed06972be98e..2a72504b64879b 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -2971,7 +2971,12 @@ zip_next(zipobject *lz) if (tuplesize == 0) return NULL; - if (Py_REFCNT(result) == 1) { + #ifdef Py_GIL_DISABLED + int reuse_tuple = 0; + #else + int reuse_tuple = Py_REFCNT(result) == 1; + #endif + if (reuse_tuple) { Py_INCREF(result); for (i=0 ; i < tuplesize ; i++) { it = PyTuple_GET_ITEM(lz->ittuple, i); From 2530fbec798eddd585ae2503fbe9e37ed03304bf Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Fri, 23 Aug 2024 22:23:43 +0200 Subject: [PATCH 02/14] whitespace --- Lib/test/test_zip_threading.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_zip_threading.py b/Lib/test/test_zip_threading.py index b569715f1f3155..648dd217d0a2e7 100644 --- a/Lib/test/test_zip_threading.py +++ b/Lib/test/test_zip_threading.py @@ -23,12 +23,20 @@ def test_threading(self): for _ in range(number_of_iterations): worker_threads = [] for ii in range(number_of_threads): - worker_threads.append(Thread(target=self.work, args=[enum,])) + worker_threads.append( + Thread( + target=self.work, + args=[ + enum, + ], + ) + ) _ = [t.start() for t in worker_threads] _ = [t.join() for t in worker_threads] - -if __name__=='__main__': - z=ZipThreading() + + +if __name__ == "__main__": + z = ZipThreading() for kk in range(10): print(kk) z.test_threading() From 1b592d2429ec4de1ce618e2b6f4e9408333006d1 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Fri, 23 Aug 2024 21:20:38 +0000 Subject: [PATCH 03/14] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst new file mode 100644 index 00000000000000..b0c050b75eb855 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst @@ -0,0 +1 @@ +Make :meth:`zip` safe under free-threading. From bd9d9fb21788f7c2af6b1181504ab7b6b2bb2926 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Sat, 24 Aug 2024 17:29:06 +0200 Subject: [PATCH 04/14] Update Python/bltinmodule.c Co-authored-by: Kirill Podoprigora --- Python/bltinmodule.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index 2a72504b64879b..edd21d03b56b3c 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -2971,11 +2971,11 @@ zip_next(zipobject *lz) if (tuplesize == 0) return NULL; - #ifdef Py_GIL_DISABLED +#ifdef Py_GIL_DISABLED int reuse_tuple = 0; - #else + #else int reuse_tuple = Py_REFCNT(result) == 1; - #endif +#endif if (reuse_tuple) { Py_INCREF(result); for (i=0 ; i < tuplesize ; i++) { From d199600d6d4da64263109541cbd2025d05c1ae40 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Sat, 24 Aug 2024 17:32:03 +0200 Subject: [PATCH 05/14] cleanup test --- Lib/test/test_free_threading/test_zip.py | 39 ++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 Lib/test/test_free_threading/test_zip.py diff --git a/Lib/test/test_free_threading/test_zip.py b/Lib/test/test_free_threading/test_zip.py new file mode 100644 index 00000000000000..7e444a13f80b02 --- /dev/null +++ b/Lib/test/test_free_threading/test_zip.py @@ -0,0 +1,39 @@ +import unittest +from threading import Thread + +from test.support import threading_helper + + +class ZipThreading(unittest.TestCase): + @staticmethod + def work(enum): + while True: + try: + next(enum) + except StopIteration: + break + + @threading_helper.reap_threads + @threading_helper.requires_working_threading() + def test_threading(self): + number_of_threads = 8 + number_of_iterations = 40 + n = 40_000 + enum = zip(range(n), range(n)) + for _ in range(number_of_iterations): + worker_threads = [] + for ii in range(number_of_threads): + worker_threads.append( + Thread( + target=self.work, + args=[ + enum, + ], + ) + ) + _ = [t.start() for t in worker_threads] + _ = [t.join() for t in worker_threads] + + +if __name__ == "__main__": + unittest.main() From 54eb5094e303164c05f155ce8f7531d2bb672648 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Sat, 24 Aug 2024 17:38:42 +0200 Subject: [PATCH 06/14] review comments --- Lib/test/test_zip_threading.py | 42 ------------------- ...-08-23-21-20-34.gh-issue-123271.xeVViR.rst | 2 +- 2 files changed, 1 insertion(+), 43 deletions(-) delete mode 100644 Lib/test/test_zip_threading.py diff --git a/Lib/test/test_zip_threading.py b/Lib/test/test_zip_threading.py deleted file mode 100644 index 648dd217d0a2e7..00000000000000 --- a/Lib/test/test_zip_threading.py +++ /dev/null @@ -1,42 +0,0 @@ -import unittest -from threading import Thread - -from test.support import threading_helper - - -class ZipThreading(unittest.TestCase): - @staticmethod - def work(enum): - while True: - try: - next(enum) - except StopIteration: - break - - @threading_helper.reap_threads - @threading_helper.requires_working_threading() - def test_threading(self): - number_of_threads = 8 - number_of_iterations = 10 - n = 40_000 - enum = zip(range(n), range(n)) - for _ in range(number_of_iterations): - worker_threads = [] - for ii in range(number_of_threads): - worker_threads.append( - Thread( - target=self.work, - args=[ - enum, - ], - ) - ) - _ = [t.start() for t in worker_threads] - _ = [t.join() for t in worker_threads] - - -if __name__ == "__main__": - z = ZipThreading() - for kk in range(10): - print(kk) - z.test_threading() diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst index b0c050b75eb855..a1b00b641a7b27 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst @@ -1 +1 @@ -Make :meth:`zip` safe under free-threading. +Make :func:`zip` safe under free-threading. From 755a862c7e560b1b31c95a25e8bbe937389acc75 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Tue, 27 Aug 2024 17:04:09 +0200 Subject: [PATCH 07/14] enable re-use of tuples in the free-threading build --- Include/refcount.h | 20 ++++++++++++++++++++ Python/bltinmodule.c | 7 ++----- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/Include/refcount.h b/Include/refcount.h index a0bd2087fb1b57..edc47aed2dbea4 100644 --- a/Include/refcount.h +++ b/Include/refcount.h @@ -493,6 +493,26 @@ static inline PyObject* _Py_XNewRef(PyObject *obj) # define Py_XNewRef(obj) _Py_XNewRef(obj) #endif +#ifdef Py_GIL_DISABLED +static inline int _Py_Reuse_Immutable_Object(PyObject *ob) { + /* Function to check whether an immutable object such as a tuple can be + * modified inplace by the owning method + */ + uint32_t local = _Py_atomic_load_uint32_relaxed(&ob->ob_ref_local); + if (local != 1) + return 0; + // maybe we need to check on Py_ARITHMETIC_RIGHT_SHIFT(Py_ssize_t, shared, _Py_REF_SHARED_SHIFT) ? + Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&ob->ob_ref_shared); + if (shared != 0) { + return 0; + } + return ob->ob_tid == _Py_ThreadId(); +} +#else +static inline int _Py_Reuse_Immutable_Object(PyObject *ob) { + return Py_REFCNT(ob) == 1; +} +#endif #ifdef __cplusplus } diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index edd21d03b56b3c..96818cc3d49c33 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -2971,11 +2971,8 @@ zip_next(zipobject *lz) if (tuplesize == 0) return NULL; -#ifdef Py_GIL_DISABLED - int reuse_tuple = 0; - #else - int reuse_tuple = Py_REFCNT(result) == 1; -#endif + + int reuse_tuple = _Py_Reuse_Immutable_Object(result); if (reuse_tuple) { Py_INCREF(result); for (i=0 ; i < tuplesize ; i++) { From 8c0fe4032e20fc5e5c3a2c055914df4d0a26fe56 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Tue, 27 Aug 2024 17:04:37 +0200 Subject: [PATCH 08/14] Update Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst Co-authored-by: Sam Gross --- .../2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst index a1b00b641a7b27..51fdec452c1d41 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-08-23-21-20-34.gh-issue-123271.xeVViR.rst @@ -1 +1 @@ -Make :func:`zip` safe under free-threading. +Make concurrent iterations over the same :func:`zip` iterator safe under free-threading. From 1e8799f84c33ee3e3acd084004fb8b035d3ff424 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Tue, 27 Aug 2024 17:34:21 +0200 Subject: [PATCH 09/14] move to pycore_object.h --- Include/internal/pycore_object.h | 18 ++++++++++++++++++ Include/refcount.h | 21 --------------------- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 0f2de6fd28f56e..e11b2f8feb9130 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -159,6 +159,24 @@ static inline void _Py_RefcntAdd(PyObject* op, Py_ssize_t n) } #define _Py_RefcntAdd(op, n) _Py_RefcntAdd(_PyObject_CAST(op), n) +static inline int _Py_Reuse_Immutable_Object(PyObject *ob) { + /* Function to check whether an immutable object such as a tuple can be + * modified inplace by the owning method + */ +#if !defined(Py_GIL_DISABLED) + return Py_REFCNT(ob) == 1; +#else + uint32_t local = _Py_atomic_load_uint32_relaxed(&ob->ob_ref_local); + if (local != 1) + return 0; + Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&ob->ob_ref_shared); + if (shared != 0) { + return 0; + } + return ob->ob_tid == _Py_ThreadId(); +#endif +} + PyAPI_FUNC(void) _Py_SetImmortal(PyObject *op); PyAPI_FUNC(void) _Py_SetImmortalUntracked(PyObject *op); diff --git a/Include/refcount.h b/Include/refcount.h index edc47aed2dbea4..6fd7205ec93f02 100644 --- a/Include/refcount.h +++ b/Include/refcount.h @@ -493,27 +493,6 @@ static inline PyObject* _Py_XNewRef(PyObject *obj) # define Py_XNewRef(obj) _Py_XNewRef(obj) #endif -#ifdef Py_GIL_DISABLED -static inline int _Py_Reuse_Immutable_Object(PyObject *ob) { - /* Function to check whether an immutable object such as a tuple can be - * modified inplace by the owning method - */ - uint32_t local = _Py_atomic_load_uint32_relaxed(&ob->ob_ref_local); - if (local != 1) - return 0; - // maybe we need to check on Py_ARITHMETIC_RIGHT_SHIFT(Py_ssize_t, shared, _Py_REF_SHARED_SHIFT) ? - Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&ob->ob_ref_shared); - if (shared != 0) { - return 0; - } - return ob->ob_tid == _Py_ThreadId(); -} -#else -static inline int _Py_Reuse_Immutable_Object(PyObject *ob) { - return Py_REFCNT(ob) == 1; -} -#endif - #ifdef __cplusplus } #endif From 4fa48dd31bcf2d1d313d4954e80e1b2b3899c0fe Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Tue, 27 Aug 2024 17:53:55 +0200 Subject: [PATCH 10/14] whitespace --- Include/refcount.h | 1 + 1 file changed, 1 insertion(+) diff --git a/Include/refcount.h b/Include/refcount.h index 6fd7205ec93f02..a0bd2087fb1b57 100644 --- a/Include/refcount.h +++ b/Include/refcount.h @@ -493,6 +493,7 @@ static inline PyObject* _Py_XNewRef(PyObject *obj) # define Py_XNewRef(obj) _Py_XNewRef(obj) #endif + #ifdef __cplusplus } #endif From 0416f0b004313c25932ec4fcd974a11d39576c8a Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Tue, 27 Aug 2024 17:54:50 +0200 Subject: [PATCH 11/14] cleanup --- Python/bltinmodule.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index 96818cc3d49c33..96db33f3f70fcb 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -2972,8 +2972,7 @@ zip_next(zipobject *lz) if (tuplesize == 0) return NULL; - int reuse_tuple = _Py_Reuse_Immutable_Object(result); - if (reuse_tuple) { + if (_Py_Reuse_Immutable_Object(result)) { Py_INCREF(result); for (i=0 ; i < tuplesize ; i++) { it = PyTuple_GET_ITEM(lz->ittuple, i); From 4120b513e0a9b604d4911ca2c3d97972a681baa3 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Tue, 27 Aug 2024 20:41:53 +0200 Subject: [PATCH 12/14] Apply suggestions from code review Co-authored-by: Sam Gross --- Include/internal/pycore_object.h | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index e11b2f8feb9130..3b33516e0d71d3 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -159,21 +159,23 @@ static inline void _Py_RefcntAdd(PyObject* op, Py_ssize_t n) } #define _Py_RefcntAdd(op, n) _Py_RefcntAdd(_PyObject_CAST(op), n) -static inline int _Py_Reuse_Immutable_Object(PyObject *ob) { +// Checks if an object has a single, unique reference. If the caller holds a +// unique reference, it may be able to safely modify the object in-place. +static inline int +_PyObject_IsUniquelyReferenced(PyObject *ob) +{ /* Function to check whether an immutable object such as a tuple can be * modified inplace by the owning method */ #if !defined(Py_GIL_DISABLED) return Py_REFCNT(ob) == 1; #else - uint32_t local = _Py_atomic_load_uint32_relaxed(&ob->ob_ref_local); - if (local != 1) - return 0; - Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&ob->ob_ref_shared); - if (shared != 0) { - return 0; - } - return ob->ob_tid == _Py_ThreadId(); + // NOTE: the entire ob_ref_shared field must be zero, including flags, to + // ensure that other threads cannot concurrently create new references to + // this object. + return (_Py_IsOwnedByCurrentThread(ob) && + _Py_atomic_load_uint32_relaxed(&ob->ob_ref_local) == 1 && + _Py_atomic_load_ssize_relaxed(&ob->ob_ref_shared) == 0); #endif } From cc3e3cbbb11e76834ce309a980772bb50b5599e4 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Tue, 27 Aug 2024 20:47:24 +0200 Subject: [PATCH 13/14] review comments --- Lib/test/test_free_threading/test_zip.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_free_threading/test_zip.py b/Lib/test/test_free_threading/test_zip.py index 7e444a13f80b02..b4c5837b1d8936 100644 --- a/Lib/test/test_free_threading/test_zip.py +++ b/Lib/test/test_free_threading/test_zip.py @@ -17,7 +17,7 @@ def work(enum): @threading_helper.requires_working_threading() def test_threading(self): number_of_threads = 8 - number_of_iterations = 40 + number_of_iterations = 8 n = 40_000 enum = zip(range(n), range(n)) for _ in range(number_of_iterations): @@ -31,8 +31,10 @@ def test_threading(self): ], ) ) - _ = [t.start() for t in worker_threads] - _ = [t.join() for t in worker_threads] + for t in worker_threads: + t.start() + for t in worker_threads: + t.join() if __name__ == "__main__": From 29b39d28ccafc818901adb5fda8d26406899d870 Mon Sep 17 00:00:00 2001 From: Pieter Eendebak Date: Tue, 27 Aug 2024 20:49:49 +0200 Subject: [PATCH 14/14] resolve merge issue --- Include/internal/pycore_object.h | 5 +---- Python/bltinmodule.c | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 3b33516e0d71d3..2ef0aaee0f82b3 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -164,15 +164,12 @@ static inline void _Py_RefcntAdd(PyObject* op, Py_ssize_t n) static inline int _PyObject_IsUniquelyReferenced(PyObject *ob) { - /* Function to check whether an immutable object such as a tuple can be - * modified inplace by the owning method - */ #if !defined(Py_GIL_DISABLED) return Py_REFCNT(ob) == 1; #else // NOTE: the entire ob_ref_shared field must be zero, including flags, to // ensure that other threads cannot concurrently create new references to - // this object. + // this object. return (_Py_IsOwnedByCurrentThread(ob) && _Py_atomic_load_uint32_relaxed(&ob->ob_ref_local) == 1 && _Py_atomic_load_ssize_relaxed(&ob->ob_ref_shared) == 0); diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index 96db33f3f70fcb..f87f942cc76258 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -2972,7 +2972,7 @@ zip_next(zipobject *lz) if (tuplesize == 0) return NULL; - if (_Py_Reuse_Immutable_Object(result)) { + if (_PyObject_IsUniquelyReferenced(result)) { Py_INCREF(result); for (i=0 ; i < tuplesize ; i++) { it = PyTuple_GET_ITEM(lz->ittuple, i);