From a7e785b46b14bdd6975e0fa52f35c5e1d7574434 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Thu, 22 Oct 2026 16:35:06 +0100 Subject: [PATCH 1/4] gh-124697: avoid unnecessary deduplication in frameobject API functions --- Include/cpython/code.h | 4 + Include/internal/pycore_code.h | 5 + Lib/test/test_listcomps.py | 17 +++ Objects/codeobject.c | 1 + Objects/frameobject.c | 187 +++++++++++++++++++++------------ 5 files changed, 144 insertions(+), 70 deletions(-) diff --git a/Include/cpython/code.h b/Include/cpython/code.h index 5238447070ee54..a56aceaa7c42dd 100644 --- a/Include/cpython/code.h +++ b/Include/cpython/code.h @@ -107,6 +107,10 @@ typedef struct { struct _PyCoMonitoringData *_co_monitoring; /* Monitoring data */ \ _PyCode_DEF_UNIQUE_ID() \ int _co_firsttraceable; /* index of first traceable instruction */ \ + /* 0 = not yet known, 1 = co_localsplusnames are unique, 2 = a name is \ + repeated. FrameLocalsProxy learns this from the seen map it builds \ + while collecting locals, and skips deduplication when this is 1. */ \ + char _co_localsplus_dedup; \ /* Scratch space for extra data relating to the code object. \ Type is a void* to keep the format private in codeobject.c to force \ people to go through the proper APIs. */ \ diff --git a/Include/internal/pycore_code.h b/Include/internal/pycore_code.h index aeb2ad06ad07cb..bf618c187823e9 100644 --- a/Include/internal/pycore_code.h +++ b/Include/internal/pycore_code.h @@ -273,6 +273,11 @@ extern PyObject* _PyCode_GetCellvars(PyCodeObject *); extern PyObject* _PyCode_GetFreevars(PyCodeObject *); extern PyObject* _PyCode_GetCode(PyCodeObject *); +/* co->_co_localsplus_dedup. Learned by FrameLocalsProxy from its seen map. */ +#define _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN 0 +#define _PYCODE_LOCALSPLUS_DEDUP_UNIQUE 1 +#define _PYCODE_LOCALSPLUS_DEDUP_NEEDED 2 + /** API for initializing the line number tables. */ PyAPI_FUNC(int) _PyCode_InitAddressRange(PyCodeObject* co, PyCodeAddressRange *bounds); diff --git a/Lib/test/test_listcomps.py b/Lib/test/test_listcomps.py index f02f3223a513d2..1f561010fc4cdd 100644 --- a/Lib/test/test_listcomps.py +++ b/Lib/test/test_listcomps.py @@ -944,6 +944,23 @@ def inner(): {"snaps": [1, 2], "vals": [2, 2], "consistent": [True, True]}, ns={"sys": sys}, scopes=["module", "function"]) + def test_frame_locals_dedup_learned_before_cell_is_bound(self): + # Reading f_locals before the comprehension cell is bound must not + # cache "names are unique": the repeated name is still on the code + # object, and dict(**f_locals) inside the comprehension has to work. + code = """ + def outer(x): + def inner(): + list(sys._getframe().f_locals) + return [dict(**sys._getframe().f_locals)['x'] for x in x] + return inner() + result = outer([7, 8]) + """ + import sys + self._check_in_scopes( + code, {"result": [7, 8]}, + ns={"sys": sys}, scopes=["module", "function"]) + def test_frame_locals_nested_comp_cell_and_enclosing_free(self): # Stress a nested inlined shape where a comp cell and enclosing free # share a name; all f_locals views must stay consistent. diff --git a/Objects/codeobject.c b/Objects/codeobject.c index 724243d2b8cfae..0dad125ccffe20 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -561,6 +561,7 @@ init_code(PyCodeObject *co, struct _PyCodeConstructor *con) /* not set */ co->co_weakreflist = NULL; co->co_extra = NULL; + co->_co_localsplus_dedup = _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN; co->_co_cached = NULL; co->co_executors = NULL; diff --git a/Objects/frameobject.c b/Objects/frameobject.c index a4cc14a6eaad45..c6ddb8b5a1d4f6 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -94,20 +94,62 @@ framelocalsproxy_hasval(_PyInterpreterFrame *frame, PyCodeObject *co, int i) return true; } +/* *seen is NULL when dedup is known to be unnecessary. + Otherwise it maps each localsplus name to Py_True if a value was already + kept, or Py_False if the name was only seen on an empty slot. An empty + slot must not count as the kept value, but it still proves the name is + repeated, so it is recorded while _co_localsplus_dedup is unknown. + Returns -1 if the map cannot be created. */ static int -framelocalsproxy_is_first_occurrence(PyObject *seen, PyObject *name) +framelocalsproxy_prepare_dedup(PyCodeObject *co, PyObject **seen) { - int found = PySet_Contains(seen, name); - if (found < 0) { + *seen = NULL; + if (co->_co_localsplus_dedup == _PYCODE_LOCALSPLUS_DEDUP_UNIQUE) { + return 0; + } + *seen = PyDict_New(); + if (*seen == NULL) { return -1; } - if (found) { + return 0; +} + +/* Returns 1 if this slot's value should be kept, 0 if not, -1 on error. + An empty slot is skipped once _co_localsplus_dedup is known. A repeated + name stores _PYCODE_LOCALSPLUS_DEDUP_NEEDED. UNIQUE is stored by the + caller only after the walk finishes without one. */ +static int +framelocalsproxy_visit_name(PyCodeObject *co, int i, PyObject *seen, + int has_value) +{ + if (seen == NULL) { + return has_value; + } + if (!has_value && + co->_co_localsplus_dedup != _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { return 0; } - if (PySet_Add(seen, name) < 0) { - return -1; + PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); + PyObject *prev = PyDict_GetItemWithError(seen, name); + if (prev == NULL) { + if (PyErr_Occurred()) { + return -1; + } + if (PyDict_SetItem(seen, name, has_value ? Py_True : Py_False) < 0) { + return -1; + } + return has_value; + } + else { + co->_co_localsplus_dedup = _PYCODE_LOCALSPLUS_DEDUP_NEEDED; + if (!has_value || prev == Py_True) { + return 0; + } + if (PyDict_SetItem(seen, name, Py_True) < 0) { + return -1; + } + return 1; } - return 1; } static int @@ -397,27 +439,30 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) return NULL; } // An inlined comprehension cell can share a name with a free var. - PyObject *seen = PySet_New(NULL); - if (seen == NULL) { + // Assume dedup may be needed until a walk of seen shows otherwise. + PyObject *seen = NULL; + if (framelocalsproxy_prepare_dedup(co, &seen) < 0) { Py_DECREF(names); return NULL; } for (int i = 0; i < co->co_nlocalsplus; i++) { - if (framelocalsproxy_hasval(frame->f_frame, co, i)) { + int has = framelocalsproxy_hasval(frame->f_frame, co, i); + int emit = framelocalsproxy_visit_name(co, i, seen, has); + if (emit < 0) { + goto error; + } + if (emit) { PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); - int first = framelocalsproxy_is_first_occurrence(seen, name); - if (first < 0) { + if (PyList_Append(names, name) < 0) { goto error; } - if (first) { - if (PyList_Append(names, name) < 0) { - goto error; - } - } } } - Py_DECREF(seen); + if (co->_co_localsplus_dedup == _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { + co->_co_localsplus_dedup = _PYCODE_LOCALSPLUS_DEDUP_UNIQUE; + } + Py_XDECREF(seen); // Iterate through the extra locals if (frame->f_extra_locals) { @@ -438,7 +483,7 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) return names; error: - Py_DECREF(seen); + Py_XDECREF(seen); Py_DECREF(names); return NULL; } @@ -622,30 +667,30 @@ framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored)) if (values == NULL) { return NULL; } - PyObject *seen = PySet_New(NULL); - if (seen == NULL) { + PyObject *seen = NULL; + if (framelocalsproxy_prepare_dedup(co, &seen) < 0) { Py_DECREF(values); return NULL; } for (int i = 0; i < co->co_nlocalsplus; i++) { PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i); - if (value) { - PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); - int first = framelocalsproxy_is_first_occurrence(seen, name); - if (first == 1) { - if (PyList_Append(values, value) < 0) { - Py_DECREF(value); - goto error; - } - } + int has = value != NULL; + int emit = framelocalsproxy_visit_name(co, i, seen, has); + if (emit < 0) { + Py_XDECREF(value); + goto error; + } + if (emit && PyList_Append(values, value) < 0) { Py_DECREF(value); - if (first < 0) { - goto error; - } + goto error; } + Py_XDECREF(value); + } + if (co->_co_localsplus_dedup == _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { + co->_co_localsplus_dedup = _PYCODE_LOCALSPLUS_DEDUP_UNIQUE; } - Py_DECREF(seen); + Py_XDECREF(seen); // Iterate through the extra locals if (frame->f_extra_locals) { @@ -663,7 +708,7 @@ framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored)) return values; error: - Py_DECREF(seen); + Py_XDECREF(seen); Py_DECREF(values); return NULL; } @@ -677,36 +722,37 @@ framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored)) if (items == NULL) { return NULL; } - PyObject *seen = PySet_New(NULL); - if (seen == NULL) { + PyObject *seen = NULL; + if (framelocalsproxy_prepare_dedup(co, &seen) < 0) { Py_DECREF(items); return NULL; } for (int i = 0; i < co->co_nlocalsplus; i++) { - PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i); - - if (value) { - int first = framelocalsproxy_is_first_occurrence(seen, name); - if (first == 1) { - PyObject *pair = _PyTuple_FromPairSteal(Py_NewRef(name), value); - if (pair == NULL) { - goto error; - } - if (_PyList_AppendTakeRef((PyListObject *)items, pair) < 0) { - goto error; - } - } - else { - Py_DECREF(value); - if (first < 0) { - goto error; - } - } + int has = value != NULL; + int emit = framelocalsproxy_visit_name(co, i, seen, has); + if (emit < 0) { + Py_XDECREF(value); + goto error; } + if (!emit) { + Py_XDECREF(value); + continue; + } + PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); + PyObject *pair = _PyTuple_FromPairSteal(Py_NewRef(name), value); + if (pair == NULL) { + goto error; + } + if (_PyList_AppendTakeRef((PyListObject *)items, pair) < 0) { + goto error; + } + } + if (co->_co_localsplus_dedup == _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { + co->_co_localsplus_dedup = _PYCODE_LOCALSPLUS_DEDUP_UNIQUE; } - Py_DECREF(seen); + Py_XDECREF(seen); seen = NULL; // Iterate through the extra locals @@ -746,24 +792,25 @@ framelocalsproxy_length(PyObject *self) size += PyDict_Size(frame->f_extra_locals); } - PyObject *seen = PySet_New(NULL); - if (seen == NULL) { + PyObject *seen = NULL; + if (framelocalsproxy_prepare_dedup(co, &seen) < 0) { return -1; } for (int i = 0; i < co->co_nlocalsplus; i++) { - if (framelocalsproxy_hasval(frame->f_frame, co, i)) { - PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); - int first = framelocalsproxy_is_first_occurrence(seen, name); - if (first < 0) { - Py_DECREF(seen); - return -1; - } - else if (first) { - size++; - } + int has = framelocalsproxy_hasval(frame->f_frame, co, i); + int emit = framelocalsproxy_visit_name(co, i, seen, has); + if (emit < 0) { + Py_XDECREF(seen); + return -1; } + if (emit) { + size++; + } + } + if (co->_co_localsplus_dedup == _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { + co->_co_localsplus_dedup = _PYCODE_LOCALSPLUS_DEDUP_UNIQUE; } - Py_DECREF(seen); + Py_XDECREF(seen); return size; } From 4caa22224b60e6db343ce722d0fe147a765a6786 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Fri, 23 Oct 2026 00:01:56 +0100 Subject: [PATCH 2/4] address review: thread safe, parameterize test --- Lib/test/test_listcomps.py | 33 ++++++++++++++++++++++----------- Objects/frameobject.c | 33 ++++++++++++++++++++++----------- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/Lib/test/test_listcomps.py b/Lib/test/test_listcomps.py index 1f561010fc4cdd..ae2c418071b3a6 100644 --- a/Lib/test/test_listcomps.py +++ b/Lib/test/test_listcomps.py @@ -948,18 +948,29 @@ def test_frame_locals_dedup_learned_before_cell_is_bound(self): # Reading f_locals before the comprehension cell is bound must not # cache "names are unique": the repeated name is still on the code # object, and dict(**f_locals) inside the comprehension has to work. - code = """ - def outer(x): - def inner(): - list(sys._getframe().f_locals) - return [dict(**sys._getframe().f_locals)['x'] for x in x] - return inner() - result = outer([7, 8]) - """ + # Each API initializes the cache on its own code object. list(proxy) + # goes through keys() first, so it does not cover the others. + probes = { + "keys": "list(sys._getframe().f_locals.keys())", + "values": "list(sys._getframe().f_locals.values())", + "items": "list(sys._getframe().f_locals.items())", + "len": "len(sys._getframe().f_locals)", + } import sys - self._check_in_scopes( - code, {"result": [7, 8]}, - ns={"sys": sys}, scopes=["module", "function"]) + for name, probe in probes.items(): + with self.subTest(probe=name): + code = f""" + def outer(x): + def inner(): + {probe} + return [dict(**sys._getframe().f_locals)['x'] + for x in x] + return inner() + result = outer([7, 8]) + """ + self._check_in_scopes( + code, {"result": [7, 8]}, + ns={"sys": sys}, scopes=["module", "function"]) def test_frame_locals_nested_comp_cell_and_enclosing_free(self): # Stress a nested inlined shape where a comp cell and enclosing free diff --git a/Objects/frameobject.c b/Objects/frameobject.c index c6ddb8b5a1d4f6..d58776f1f4bb4e 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -104,7 +104,8 @@ static int framelocalsproxy_prepare_dedup(PyCodeObject *co, PyObject **seen) { *seen = NULL; - if (co->_co_localsplus_dedup == _PYCODE_LOCALSPLUS_DEDUP_UNIQUE) { + if (FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) == + _PYCODE_LOCALSPLUS_DEDUP_UNIQUE) { return 0; } *seen = PyDict_New(); @@ -126,7 +127,8 @@ framelocalsproxy_visit_name(PyCodeObject *co, int i, PyObject *seen, return has_value; } if (!has_value && - co->_co_localsplus_dedup != _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { + FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) != + _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { return 0; } PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); @@ -141,7 +143,8 @@ framelocalsproxy_visit_name(PyCodeObject *co, int i, PyObject *seen, return has_value; } else { - co->_co_localsplus_dedup = _PYCODE_LOCALSPLUS_DEDUP_NEEDED; + FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, + _PYCODE_LOCALSPLUS_DEDUP_NEEDED); if (!has_value || prev == Py_True) { return 0; } @@ -459,8 +462,10 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) } } } - if (co->_co_localsplus_dedup == _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { - co->_co_localsplus_dedup = _PYCODE_LOCALSPLUS_DEDUP_UNIQUE; + if (FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) == + _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { + FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, + _PYCODE_LOCALSPLUS_DEDUP_UNIQUE); } Py_XDECREF(seen); @@ -687,8 +692,10 @@ framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored)) } Py_XDECREF(value); } - if (co->_co_localsplus_dedup == _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { - co->_co_localsplus_dedup = _PYCODE_LOCALSPLUS_DEDUP_UNIQUE; + if (FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) == + _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { + FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, + _PYCODE_LOCALSPLUS_DEDUP_UNIQUE); } Py_XDECREF(seen); @@ -749,8 +756,10 @@ framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored)) goto error; } } - if (co->_co_localsplus_dedup == _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { - co->_co_localsplus_dedup = _PYCODE_LOCALSPLUS_DEDUP_UNIQUE; + if (FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) == + _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { + FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, + _PYCODE_LOCALSPLUS_DEDUP_UNIQUE); } Py_XDECREF(seen); seen = NULL; @@ -807,8 +816,10 @@ framelocalsproxy_length(PyObject *self) size++; } } - if (co->_co_localsplus_dedup == _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { - co->_co_localsplus_dedup = _PYCODE_LOCALSPLUS_DEDUP_UNIQUE; + if (FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) == + _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { + FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, + _PYCODE_LOCALSPLUS_DEDUP_UNIQUE); } Py_XDECREF(seen); return size; From cb971c0f2ae9412d458291eb8e455918b3b5df1c Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Sat, 24 Oct 2026 13:04:51 +0100 Subject: [PATCH 3/4] separate scan to decide dedup from the names --- Include/cpython/code.h | 4 +- Include/internal/pycore_code.h | 2 +- Lib/test/test_listcomps.py | 29 ++++----- Objects/frameobject.c | 107 +++++++++++++++------------------ 4 files changed, 66 insertions(+), 76 deletions(-) diff --git a/Include/cpython/code.h b/Include/cpython/code.h index a56aceaa7c42dd..ba5102c05b5c82 100644 --- a/Include/cpython/code.h +++ b/Include/cpython/code.h @@ -108,8 +108,8 @@ typedef struct { _PyCode_DEF_UNIQUE_ID() \ int _co_firsttraceable; /* index of first traceable instruction */ \ /* 0 = not yet known, 1 = co_localsplusnames are unique, 2 = a name is \ - repeated. FrameLocalsProxy learns this from the seen map it builds \ - while collecting locals, and skips deduplication when this is 1. */ \ + repeated. FrameLocalsProxy scans the names once and skips \ + deduplication when this is 1. */ \ char _co_localsplus_dedup; \ /* Scratch space for extra data relating to the code object. \ Type is a void* to keep the format private in codeobject.c to force \ diff --git a/Include/internal/pycore_code.h b/Include/internal/pycore_code.h index bf618c187823e9..32659784a47793 100644 --- a/Include/internal/pycore_code.h +++ b/Include/internal/pycore_code.h @@ -273,7 +273,7 @@ extern PyObject* _PyCode_GetCellvars(PyCodeObject *); extern PyObject* _PyCode_GetFreevars(PyCodeObject *); extern PyObject* _PyCode_GetCode(PyCodeObject *); -/* co->_co_localsplus_dedup. Learned by FrameLocalsProxy from its seen map. */ +/* co->_co_localsplus_dedup. Computed once from co_localsplusnames. */ #define _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN 0 #define _PYCODE_LOCALSPLUS_DEDUP_UNIQUE 1 #define _PYCODE_LOCALSPLUS_DEDUP_NEEDED 2 diff --git a/Lib/test/test_listcomps.py b/Lib/test/test_listcomps.py index ae2c418071b3a6..047e3a3cef6fef 100644 --- a/Lib/test/test_listcomps.py +++ b/Lib/test/test_listcomps.py @@ -944,32 +944,33 @@ def inner(): {"snaps": [1, 2], "vals": [2, 2], "consistent": [True, True]}, ns={"sys": sys}, scopes=["module", "function"]) - def test_frame_locals_dedup_learned_before_cell_is_bound(self): - # Reading f_locals before the comprehension cell is bound must not + def test_frame_locals_dedup_learned_before_slot_is_bound(self): + # Reading f_locals before the comprehension slot is bound must not # cache "names are unique": the repeated name is still on the code # object, and dict(**f_locals) inside the comprehension has to work. - # Each API initializes the cache on its own code object. list(proxy) - # goes through keys() first, so it does not cover the others. + # The enclosing x is already bound, so the first read shows that + # value. Each API runs on its own code object. probes = { - "keys": "list(sys._getframe().f_locals.keys())", - "values": "list(sys._getframe().f_locals.values())", - "items": "list(sys._getframe().f_locals.items())", - "len": "len(sys._getframe().f_locals)", + "keys": ("list(sys._getframe().f_locals.keys())", ["x"]), + "values": ("list(sys._getframe().f_locals.values())", [[7, 8]]), + "items": ("list(sys._getframe().f_locals.items())", + [("x", [7, 8])]), + "len": ("len(sys._getframe().f_locals)", 1), } import sys - for name, probe in probes.items(): + for name, (probe, expected) in probes.items(): with self.subTest(probe=name): code = f""" def outer(x): def inner(): - {probe} - return [dict(**sys._getframe().f_locals)['x'] - for x in x] + probe = {probe} + return probe, [dict(**sys._getframe().f_locals)['x'] + for x in x] return inner() - result = outer([7, 8]) + probe, result = outer([7, 8]) """ self._check_in_scopes( - code, {"result": [7, 8]}, + code, {"probe": expected, "result": [7, 8]}, ns={"sys": sys}, scopes=["module", "function"]) def test_frame_locals_nested_comp_cell_and_enclosing_free(self): diff --git a/Objects/frameobject.c b/Objects/frameobject.c index d58776f1f4bb4e..b85fbfa127dffa 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -94,21 +94,44 @@ framelocalsproxy_hasval(_PyInterpreterFrame *frame, PyCodeObject *co, int i) return true; } -/* *seen is NULL when dedup is known to be unnecessary. - Otherwise it maps each localsplus name to Py_True if a value was already - kept, or Py_False if the name was only seen on an empty slot. An empty - slot must not count as the kept value, but it still proves the name is - repeated, so it is recorded while _co_localsplus_dedup is unknown. - Returns -1 if the map cannot be created. */ +/* Scan co_localsplusnames once. A repeated name is on the code object + whether or not its slot is bound. Returns -1 on error. */ +static int +framelocalsproxy_learn_dedup(PyCodeObject *co) +{ + char dedup = _PYCODE_LOCALSPLUS_DEDUP_UNIQUE; + if (co->co_nlocalsplus > 1) { + assert(PyTuple_GET_SIZE(co->co_localsplusnames) == co->co_nlocalsplus); + PyObject *names = PySet_New(co->co_localsplusnames); + if (names == NULL) { + return -1; + } + if (PySet_GET_SIZE(names) != co->co_nlocalsplus) { + dedup = _PYCODE_LOCALSPLUS_DEDUP_NEEDED; + } + Py_DECREF(names); + } + FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, dedup); + return 0; +} + +/* *seen is NULL when dedup is unnecessary. Otherwise it is the set of bound + names already kept. Returns -1 if the set cannot be created. */ static int framelocalsproxy_prepare_dedup(PyCodeObject *co, PyObject **seen) { *seen = NULL; - if (FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) == - _PYCODE_LOCALSPLUS_DEDUP_UNIQUE) { + char state = FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup); + if (state == _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { + if (framelocalsproxy_learn_dedup(co) < 0) { + return -1; + } + state = FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup); + } + if (state == _PYCODE_LOCALSPLUS_DEDUP_UNIQUE) { return 0; } - *seen = PyDict_New(); + *seen = PySet_New(NULL); if (*seen == NULL) { return -1; } @@ -116,43 +139,30 @@ framelocalsproxy_prepare_dedup(PyCodeObject *co, PyObject **seen) } /* Returns 1 if this slot's value should be kept, 0 if not, -1 on error. - An empty slot is skipped once _co_localsplus_dedup is known. A repeated - name stores _PYCODE_LOCALSPLUS_DEDUP_NEEDED. UNIQUE is stored by the - caller only after the walk finishes without one. */ + Empty slots are not kept. When dedup is required, the first bound value + for a name wins. */ static int framelocalsproxy_visit_name(PyCodeObject *co, int i, PyObject *seen, int has_value) { - if (seen == NULL) { - return has_value; - } - if (!has_value && - FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) != - _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { + if (!has_value) { return 0; } + if (seen == NULL) { + return 1; + } PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); - PyObject *prev = PyDict_GetItemWithError(seen, name); - if (prev == NULL) { - if (PyErr_Occurred()) { - return -1; - } - if (PyDict_SetItem(seen, name, has_value ? Py_True : Py_False) < 0) { - return -1; - } - return has_value; + int found = PySet_Contains(seen, name); + if (found < 0) { + return -1; } - else { - FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, - _PYCODE_LOCALSPLUS_DEDUP_NEEDED); - if (!has_value || prev == Py_True) { - return 0; - } - if (PyDict_SetItem(seen, name, Py_True) < 0) { - return -1; - } - return 1; + if (found) { + return 0; } + if (PySet_Add(seen, name) < 0) { + return -1; + } + return 1; } static int @@ -441,8 +451,7 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) if (names == NULL) { return NULL; } - // An inlined comprehension cell can share a name with a free var. - // Assume dedup may be needed until a walk of seen shows otherwise. + // An inlined comprehension local can share a name with a free var. PyObject *seen = NULL; if (framelocalsproxy_prepare_dedup(co, &seen) < 0) { Py_DECREF(names); @@ -462,11 +471,6 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) } } } - if (FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) == - _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { - FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, - _PYCODE_LOCALSPLUS_DEDUP_UNIQUE); - } Py_XDECREF(seen); // Iterate through the extra locals @@ -692,11 +696,6 @@ framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored)) } Py_XDECREF(value); } - if (FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) == - _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { - FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, - _PYCODE_LOCALSPLUS_DEDUP_UNIQUE); - } Py_XDECREF(seen); // Iterate through the extra locals @@ -756,11 +755,6 @@ framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored)) goto error; } } - if (FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) == - _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { - FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, - _PYCODE_LOCALSPLUS_DEDUP_UNIQUE); - } Py_XDECREF(seen); seen = NULL; @@ -816,11 +810,6 @@ framelocalsproxy_length(PyObject *self) size++; } } - if (FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) == - _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { - FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, - _PYCODE_LOCALSPLUS_DEDUP_UNIQUE); - } Py_XDECREF(seen); return size; } From 74ed861681464aee66dbcf6e9cafcc26b6a291bb Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Sat, 24 Oct 2026 13:38:18 +0100 Subject: [PATCH 4/4] scan only bound names. Unknown until all are bound. --- Include/cpython/code.h | 4 +- Include/internal/pycore_code.h | 3 +- Objects/frameobject.c | 96 ++++++++++++++++++++-------------- 3 files changed, 61 insertions(+), 42 deletions(-) diff --git a/Include/cpython/code.h b/Include/cpython/code.h index ba5102c05b5c82..c958f4d05a6cc5 100644 --- a/Include/cpython/code.h +++ b/Include/cpython/code.h @@ -108,8 +108,8 @@ typedef struct { _PyCode_DEF_UNIQUE_ID() \ int _co_firsttraceable; /* index of first traceable instruction */ \ /* 0 = not yet known, 1 = co_localsplusnames are unique, 2 = a name is \ - repeated. FrameLocalsProxy scans the names once and skips \ - deduplication when this is 1. */ \ + repeated. Learned from bound locals; deduplication is skipped when \ + this is 1. */ \ char _co_localsplus_dedup; \ /* Scratch space for extra data relating to the code object. \ Type is a void* to keep the format private in codeobject.c to force \ diff --git a/Include/internal/pycore_code.h b/Include/internal/pycore_code.h index 32659784a47793..16e2ccf513bc7f 100644 --- a/Include/internal/pycore_code.h +++ b/Include/internal/pycore_code.h @@ -273,7 +273,8 @@ extern PyObject* _PyCode_GetCellvars(PyCodeObject *); extern PyObject* _PyCode_GetFreevars(PyCodeObject *); extern PyObject* _PyCode_GetCode(PyCodeObject *); -/* co->_co_localsplus_dedup. Computed once from co_localsplusnames. */ +/* co->_co_localsplus_dedup. Learned from bound locals. UNIQUE is stored + only after a walk sees every slot bound without a repeated name. */ #define _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN 0 #define _PYCODE_LOCALSPLUS_DEDUP_UNIQUE 1 #define _PYCODE_LOCALSPLUS_DEDUP_NEEDED 2 diff --git a/Objects/frameobject.c b/Objects/frameobject.c index b85fbfa127dffa..7187465a175214 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -94,27 +94,6 @@ framelocalsproxy_hasval(_PyInterpreterFrame *frame, PyCodeObject *co, int i) return true; } -/* Scan co_localsplusnames once. A repeated name is on the code object - whether or not its slot is bound. Returns -1 on error. */ -static int -framelocalsproxy_learn_dedup(PyCodeObject *co) -{ - char dedup = _PYCODE_LOCALSPLUS_DEDUP_UNIQUE; - if (co->co_nlocalsplus > 1) { - assert(PyTuple_GET_SIZE(co->co_localsplusnames) == co->co_nlocalsplus); - PyObject *names = PySet_New(co->co_localsplusnames); - if (names == NULL) { - return -1; - } - if (PySet_GET_SIZE(names) != co->co_nlocalsplus) { - dedup = _PYCODE_LOCALSPLUS_DEDUP_NEEDED; - } - Py_DECREF(names); - } - FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, dedup); - return 0; -} - /* *seen is NULL when dedup is unnecessary. Otherwise it is the set of bound names already kept. Returns -1 if the set cannot be created. */ static int @@ -122,15 +101,16 @@ framelocalsproxy_prepare_dedup(PyCodeObject *co, PyObject **seen) { *seen = NULL; char state = FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup); - if (state == _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { - if (framelocalsproxy_learn_dedup(co) < 0) { - return -1; - } - state = FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup); - } if (state == _PYCODE_LOCALSPLUS_DEDUP_UNIQUE) { return 0; } + /* A single name cannot be repeated. More names stay UNKNOWN until a + walk sees every slot bound without a repeat. */ + if (state == _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN && co->co_nlocalsplus <= 1) { + FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, + _PYCODE_LOCALSPLUS_DEDUP_UNIQUE); + return 0; + } *seen = PySet_New(NULL); if (*seen == NULL) { return -1; @@ -138,14 +118,31 @@ framelocalsproxy_prepare_dedup(PyCodeObject *co, PyObject **seen) return 0; } +/* A walk that saw every slot bound, and no repeated bound name, has shown + that co_localsplusnames are unique. A sparse walk leaves the cache + UNKNOWN: an unbound slot may still repeat a name. */ +static void +framelocalsproxy_finish_dedup(PyCodeObject *co, int all_bound) +{ + if (all_bound && + FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) == + _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) + { + FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, + _PYCODE_LOCALSPLUS_DEDUP_UNIQUE); + } +} + /* Returns 1 if this slot's value should be kept, 0 if not, -1 on error. - Empty slots are not kept. When dedup is required, the first bound value - for a name wins. */ + Empty slots are not kept. The first bound value for a name wins. + *all_bound is cleared when a slot is empty. A repeated bound name + stores NEEDED. */ static int framelocalsproxy_visit_name(PyCodeObject *co, int i, PyObject *seen, - int has_value) + int has_value, int *all_bound) { if (!has_value) { + *all_bound = 0; return 0; } if (seen == NULL) { @@ -157,6 +154,11 @@ framelocalsproxy_visit_name(PyCodeObject *co, int i, PyObject *seen, return -1; } if (found) { + if (FT_ATOMIC_LOAD_CHAR_RELAXED(co->_co_localsplus_dedup) == + _PYCODE_LOCALSPLUS_DEDUP_UNKNOWN) { + FT_ATOMIC_STORE_CHAR_RELAXED(co->_co_localsplus_dedup, + _PYCODE_LOCALSPLUS_DEDUP_NEEDED); + } return 0; } if (PySet_Add(seen, name) < 0) { @@ -458,9 +460,10 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) return NULL; } + int all_bound = 1; for (int i = 0; i < co->co_nlocalsplus; i++) { int has = framelocalsproxy_hasval(frame->f_frame, co, i); - int emit = framelocalsproxy_visit_name(co, i, seen, has); + int emit = framelocalsproxy_visit_name(co, i, seen, has, &all_bound); if (emit < 0) { goto error; } @@ -471,7 +474,10 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) } } } - Py_XDECREF(seen); + if (seen != NULL) { + framelocalsproxy_finish_dedup(co, all_bound); + Py_DECREF(seen); + } // Iterate through the extra locals if (frame->f_extra_locals) { @@ -682,10 +688,11 @@ framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored)) return NULL; } + int all_bound = 1; for (int i = 0; i < co->co_nlocalsplus; i++) { PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i); int has = value != NULL; - int emit = framelocalsproxy_visit_name(co, i, seen, has); + int emit = framelocalsproxy_visit_name(co, i, seen, has, &all_bound); if (emit < 0) { Py_XDECREF(value); goto error; @@ -696,7 +703,10 @@ framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored)) } Py_XDECREF(value); } - Py_XDECREF(seen); + if (seen != NULL) { + framelocalsproxy_finish_dedup(co, all_bound); + Py_DECREF(seen); + } // Iterate through the extra locals if (frame->f_extra_locals) { @@ -734,10 +744,11 @@ framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored)) return NULL; } + int all_bound = 1; for (int i = 0; i < co->co_nlocalsplus; i++) { PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i); int has = value != NULL; - int emit = framelocalsproxy_visit_name(co, i, seen, has); + int emit = framelocalsproxy_visit_name(co, i, seen, has, &all_bound); if (emit < 0) { Py_XDECREF(value); goto error; @@ -755,8 +766,11 @@ framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored)) goto error; } } - Py_XDECREF(seen); - seen = NULL; + if (seen != NULL) { + framelocalsproxy_finish_dedup(co, all_bound); + Py_DECREF(seen); + seen = NULL; + } // Iterate through the extra locals if (frame->f_extra_locals) { @@ -799,9 +813,10 @@ framelocalsproxy_length(PyObject *self) if (framelocalsproxy_prepare_dedup(co, &seen) < 0) { return -1; } + int all_bound = 1; for (int i = 0; i < co->co_nlocalsplus; i++) { int has = framelocalsproxy_hasval(frame->f_frame, co, i); - int emit = framelocalsproxy_visit_name(co, i, seen, has); + int emit = framelocalsproxy_visit_name(co, i, seen, has, &all_bound); if (emit < 0) { Py_XDECREF(seen); return -1; @@ -810,7 +825,10 @@ framelocalsproxy_length(PyObject *self) size++; } } - Py_XDECREF(seen); + if (seen != NULL) { + framelocalsproxy_finish_dedup(co, all_bound); + Py_DECREF(seen); + } return size; }