From af304c8ea8b9a02219185cabb7de95fd0c175ed2 Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Mon, 4 Nov 2024 09:07:00 -0500 Subject: [PATCH 1/7] gh-126298: Don't deduplicated slice constants based on equality --- Lib/test/test_compile.py | 77 +++++++++++++++++++++++++++++----------- Objects/codeobject.c | 26 +++++++++++++- 2 files changed, 82 insertions(+), 21 deletions(-) diff --git a/Lib/test/test_compile.py b/Lib/test/test_compile.py index 85ae71c1f77b28..fde71228f44224 100644 --- a/Lib/test/test_compile.py +++ b/Lib/test/test_compile.py @@ -1384,53 +1384,90 @@ def check_op_count(func, op, expected): actual += 1 self.assertEqual(actual, expected) + def check_num_consts(func, typ, expected): + num_consts = 0 + consts = func.__code__.co_consts + for instr in dis.Bytecode(func): + if instr.opname == "LOAD_CONST" and isinstance(consts[instr.oparg], typ): + num_consts += 1 + self.assertEqual(num_consts, expected) + def check_consts(func, typ, expected): - slice_consts = 0 + all_consts = set() consts = func.__code__.co_consts for instr in dis.Bytecode(func): if instr.opname == "LOAD_CONST" and isinstance(consts[instr.oparg], typ): - slice_consts += 1 - self.assertEqual(slice_consts, expected) + all_consts.add(consts[instr.oparg]) + self.assertEqual(all_consts, expected) def load(): return x[a:b] + x [a:] + x[:b] + x[:] + check_op_count(load, "BINARY_SLICE", 3) + check_op_count(load, "BUILD_SLICE", 0) + check_consts(load, slice, {slice(None, None, None)}) + def store(): x[a:b] = y - x [a:] = y + x[a:] = y x[:b] = y x[:] = y + check_op_count(store, "STORE_SLICE", 3) + check_op_count(store, "BUILD_SLICE", 0) + check_consts(store, slice, {slice(None, None, None)}) + def long_slice(): return x[a:b:c] + check_op_count(long_slice, "BUILD_SLICE", 1) + check_op_count(long_slice, "BINARY_SLICE", 0) + check_num_consts(long_slice, slice, 0) + def aug(): x[a:b] += y + check_op_count(aug, "BINARY_SLICE", 1) + check_op_count(aug, "STORE_SLICE", 1) + check_op_count(aug, "BUILD_SLICE", 0) + check_num_consts(long_slice, slice, 0) + def aug_const(): x[1:2] += y + check_op_count(aug_const, "BINARY_SLICE", 0) + check_op_count(aug_const, "STORE_SLICE", 0) + check_consts(aug_const, slice, {slice(1, 2)}) + def compound_const_slice(): x[1:2:3, 4:5:6] = y - check_op_count(load, "BINARY_SLICE", 3) - check_op_count(load, "BUILD_SLICE", 0) - check_consts(load, slice, 1) - check_op_count(store, "STORE_SLICE", 3) - check_op_count(store, "BUILD_SLICE", 0) - check_consts(store, slice, 1) - check_op_count(long_slice, "BUILD_SLICE", 1) - check_op_count(long_slice, "BINARY_SLICE", 0) - check_op_count(aug, "BINARY_SLICE", 1) - check_op_count(aug, "STORE_SLICE", 1) - check_op_count(aug, "BUILD_SLICE", 0) - check_op_count(aug_const, "BINARY_SLICE", 0) - check_op_count(aug_const, "STORE_SLICE", 0) - check_consts(aug_const, slice, 1) check_op_count(compound_const_slice, "BINARY_SLICE", 0) check_op_count(compound_const_slice, "BUILD_SLICE", 0) - check_consts(compound_const_slice, slice, 0) - check_consts(compound_const_slice, tuple, 1) + check_num_consts(compound_const_slice, slice, 0) + check_consts(compound_const_slice, tuple, {(slice(1, 2, 3), slice(4, 5, 6))}) + + def mutable_slice(): + x[[]:] = y + + check_num_consts(mutable_slice, slice, 0) + + def different_but_equal(): + x[:0] = y + x[:0.0] = y + x[:False] = y + x[:None] = y + + check_consts( + different_but_equal, + slice, + { + slice(None, 0, None), + slice(None, 0.0, None), + slice(None, False, None), + slice(None, None, None) + } + ) def test_compare_positions(self): for opname_prefix, op in [ diff --git a/Objects/codeobject.c b/Objects/codeobject.c index 775ea7aca824c4..4e7febc781198d 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -2348,7 +2348,6 @@ _PyCode_ConstantKey(PyObject *op) if (op == Py_None || op == Py_Ellipsis || PyLong_CheckExact(op) || PyUnicode_CheckExact(op) - || PySlice_Check(op) /* code_richcompare() uses _PyCode_ConstantKey() internally */ || PyCode_Check(op)) { @@ -2456,6 +2455,31 @@ _PyCode_ConstantKey(PyObject *op) Py_DECREF(set); return key; } + else if (PySlice_Check(op)) { + PySliceObject *slice = (PySliceObject *)op; + + PyObject *start = slice->start; + PyObject *start_key = _PyCode_ConstantKey(start); + if (start_key == NULL) { + return NULL; + } + + PyObject *stop = slice->stop; + PyObject *stop_key = _PyCode_ConstantKey(stop); + if (stop_key == NULL) { + return NULL; + } + + PyObject *step = slice->step; + PyObject *step_key = _PyCode_ConstantKey(step); + if (step_key == NULL) { + return NULL; + } + + PyObject *slice_key = PySlice_New(start_key, stop_key, step_key); + key = PyTuple_Pack(2, slice_key, op); + Py_DECREF(slice_key); + } else { /* for other types, use the object identifier as a unique identifier * to ensure that they are seen as unequal. */ From 1d55e6e79c9e1296f89f844aaea87f06b71e6a2e Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Mon, 4 Nov 2024 09:13:54 -0500 Subject: [PATCH 2/7] NULL check for PySlice_New --- Objects/codeobject.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Objects/codeobject.c b/Objects/codeobject.c index 4e7febc781198d..95584f17cc0af6 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -2477,6 +2477,9 @@ _PyCode_ConstantKey(PyObject *op) } PyObject *slice_key = PySlice_New(start_key, stop_key, step_key); + if (slice_key == NULL) { + return NULL; + } key = PyTuple_Pack(2, slice_key, op); Py_DECREF(slice_key); } From 8d70094e65b4175a5e872f77f225bb875a6b4e38 Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Mon, 4 Nov 2024 09:15:23 -0500 Subject: [PATCH 3/7] Fix refcounting --- Objects/codeobject.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Objects/codeobject.c b/Objects/codeobject.c index 95584f17cc0af6..8fd17b49cdd996 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -2478,8 +2478,12 @@ _PyCode_ConstantKey(PyObject *op) PyObject *slice_key = PySlice_New(start_key, stop_key, step_key); if (slice_key == NULL) { + Py_DECREF(start_key); + Py_DECREF(stop_key); + Py_DECREF(step_key); return NULL; } + key = PyTuple_Pack(2, slice_key, op); Py_DECREF(slice_key); } From 1fc93c78d8b6e22d26bbef628d289767c0919e04 Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Mon, 4 Nov 2024 09:16:13 -0500 Subject: [PATCH 4/7] Fix refcounting some more --- Objects/codeobject.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Objects/codeobject.c b/Objects/codeobject.c index 8fd17b49cdd996..41ed7d46e651cc 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -2467,12 +2467,15 @@ _PyCode_ConstantKey(PyObject *op) PyObject *stop = slice->stop; PyObject *stop_key = _PyCode_ConstantKey(stop); if (stop_key == NULL) { + Py_DECREF(start_key); return NULL; } PyObject *step = slice->step; PyObject *step_key = _PyCode_ConstantKey(step); if (step_key == NULL) { + Py_DECREF(start_key); + Py_DECREF(stop_key); return NULL; } From 3cd03e0b4551f597ce43f5b793a37f3e24762c04 Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Mon, 4 Nov 2024 09:21:08 -0500 Subject: [PATCH 5/7] Fix refcounting --- Objects/codeobject.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/Objects/codeobject.c b/Objects/codeobject.c index 41ed7d46e651cc..1c3f188bf5c086 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -2457,38 +2457,37 @@ _PyCode_ConstantKey(PyObject *op) } else if (PySlice_Check(op)) { PySliceObject *slice = (PySliceObject *)op; + PyObject *start_key = NULL; + PyObject *stop_key = NULL; + PyObject *step_key = NULL; + key = NULL; - PyObject *start = slice->start; - PyObject *start_key = _PyCode_ConstantKey(start); + start_key = _PyCode_ConstantKey(slice->start); if (start_key == NULL) { - return NULL; + goto slice_exit; } - PyObject *stop = slice->stop; - PyObject *stop_key = _PyCode_ConstantKey(stop); + stop_key = _PyCode_ConstantKey(slice->stop); if (stop_key == NULL) { - Py_DECREF(start_key); - return NULL; + goto slice_exit; } - PyObject *step = slice->step; - PyObject *step_key = _PyCode_ConstantKey(step); + step_key = _PyCode_ConstantKey(slice->step); if (step_key == NULL) { - Py_DECREF(start_key); - Py_DECREF(stop_key); - return NULL; + goto slice_exit; } PyObject *slice_key = PySlice_New(start_key, stop_key, step_key); if (slice_key == NULL) { - Py_DECREF(start_key); - Py_DECREF(stop_key); - Py_DECREF(step_key); - return NULL; + goto slice_exit; } key = PyTuple_Pack(2, slice_key, op); Py_DECREF(slice_key); + slice_exit: + Py_XDECREF(start_key); + Py_XDECREF(stop_key); + Py_XDECREF(step_key); } else { /* for other types, use the object identifier as a unique identifier From b2b6149bdb9ac5fbecc389690aaea1d62fe6e6e9 Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Mon, 4 Nov 2024 15:27:40 -0500 Subject: [PATCH 6/7] Make tests more complete --- Lib/test/test_compile.py | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/Lib/test/test_compile.py b/Lib/test/test_compile.py index fde71228f44224..87d735bd33c6e4 100644 --- a/Lib/test/test_compile.py +++ b/Lib/test/test_compile.py @@ -1384,14 +1384,6 @@ def check_op_count(func, op, expected): actual += 1 self.assertEqual(actual, expected) - def check_num_consts(func, typ, expected): - num_consts = 0 - consts = func.__code__.co_consts - for instr in dis.Bytecode(func): - if instr.opname == "LOAD_CONST" and isinstance(consts[instr.oparg], typ): - num_consts += 1 - self.assertEqual(num_consts, expected) - def check_consts(func, typ, expected): all_consts = set() consts = func.__code__.co_consts @@ -1406,15 +1398,17 @@ def load(): check_op_count(load, "BINARY_SLICE", 3) check_op_count(load, "BUILD_SLICE", 0) check_consts(load, slice, {slice(None, None, None)}) + check_op_count(load, "BINARY_SUBSCR", 1) def store(): x[a:b] = y - x[a:] = y + x [a:] = y x[:b] = y x[:] = y check_op_count(store, "STORE_SLICE", 3) check_op_count(store, "BUILD_SLICE", 0) + check_op_count(store, "STORE_SUBSCR", 1) check_consts(store, slice, {slice(None, None, None)}) def long_slice(): @@ -1422,7 +1416,8 @@ def long_slice(): check_op_count(long_slice, "BUILD_SLICE", 1) check_op_count(long_slice, "BINARY_SLICE", 0) - check_num_consts(long_slice, slice, 0) + check_consts(long_slice, slice, set()) + check_op_count(long_slice, "BINARY_SUBSCR", 1) def aug(): x[a:b] += y @@ -1430,13 +1425,17 @@ def aug(): check_op_count(aug, "BINARY_SLICE", 1) check_op_count(aug, "STORE_SLICE", 1) check_op_count(aug, "BUILD_SLICE", 0) - check_num_consts(long_slice, slice, 0) + check_op_count(aug, "BINARY_SUBSCR", 0) + check_op_count(aug, "STORE_SUBSCR", 0) + check_consts(aug, slice, set()) def aug_const(): x[1:2] += y check_op_count(aug_const, "BINARY_SLICE", 0) check_op_count(aug_const, "STORE_SLICE", 0) + check_op_count(aug_const, "BINARY_SUBSCR", 1) + check_op_count(aug_const, "STORE_SUBSCR", 1) check_consts(aug_const, slice, {slice(1, 2)}) def compound_const_slice(): @@ -1444,13 +1443,15 @@ def compound_const_slice(): check_op_count(compound_const_slice, "BINARY_SLICE", 0) check_op_count(compound_const_slice, "BUILD_SLICE", 0) - check_num_consts(compound_const_slice, slice, 0) + check_op_count(compound_const_slice, "STORE_SLICE", 0) + check_op_count(compound_const_slice, "BINARY_SUBSCR", 1) + check_consts(compound_const_slice, slice, {}) check_consts(compound_const_slice, tuple, {(slice(1, 2, 3), slice(4, 5, 6))}) def mutable_slice(): x[[]:] = y - check_num_consts(mutable_slice, slice, 0) + check_consts(mutable_slice, slice, {}) def different_but_equal(): x[:0] = y From 3a089290ac0f22c186b2f64c90eabf4a57eb423a Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Thu, 7 Nov 2024 11:09:39 -0500 Subject: [PATCH 7/7] Fix tests --- Lib/test/test_compile.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/Lib/test/test_compile.py b/Lib/test/test_compile.py index 87d735bd33c6e4..519a1207afb1fc 100644 --- a/Lib/test/test_compile.py +++ b/Lib/test/test_compile.py @@ -2,6 +2,7 @@ import dis import io import itertools +import marshal import math import opcode import os @@ -1385,11 +1386,12 @@ def check_op_count(func, op, expected): self.assertEqual(actual, expected) def check_consts(func, typ, expected): + expected = set([repr(x) for x in expected]) all_consts = set() consts = func.__code__.co_consts for instr in dis.Bytecode(func): if instr.opname == "LOAD_CONST" and isinstance(consts[instr.oparg], typ): - all_consts.add(consts[instr.oparg]) + all_consts.add(repr(consts[instr.oparg])) self.assertEqual(all_consts, expected) def load(): @@ -1397,7 +1399,7 @@ def load(): check_op_count(load, "BINARY_SLICE", 3) check_op_count(load, "BUILD_SLICE", 0) - check_consts(load, slice, {slice(None, None, None)}) + check_consts(load, slice, [slice(None, None, None)]) check_op_count(load, "BINARY_SUBSCR", 1) def store(): @@ -1409,14 +1411,14 @@ def store(): check_op_count(store, "STORE_SLICE", 3) check_op_count(store, "BUILD_SLICE", 0) check_op_count(store, "STORE_SUBSCR", 1) - check_consts(store, slice, {slice(None, None, None)}) + check_consts(store, slice, [slice(None, None, None)]) def long_slice(): return x[a:b:c] check_op_count(long_slice, "BUILD_SLICE", 1) check_op_count(long_slice, "BINARY_SLICE", 0) - check_consts(long_slice, slice, set()) + check_consts(long_slice, slice, []) check_op_count(long_slice, "BINARY_SUBSCR", 1) def aug(): @@ -1427,7 +1429,7 @@ def aug(): check_op_count(aug, "BUILD_SLICE", 0) check_op_count(aug, "BINARY_SUBSCR", 0) check_op_count(aug, "STORE_SUBSCR", 0) - check_consts(aug, slice, set()) + check_consts(aug, slice, []) def aug_const(): x[1:2] += y @@ -1436,7 +1438,7 @@ def aug_const(): check_op_count(aug_const, "STORE_SLICE", 0) check_op_count(aug_const, "BINARY_SUBSCR", 1) check_op_count(aug_const, "STORE_SUBSCR", 1) - check_consts(aug_const, slice, {slice(1, 2)}) + check_consts(aug_const, slice, [slice(1, 2)]) def compound_const_slice(): x[1:2:3, 4:5:6] = y @@ -1444,9 +1446,9 @@ def compound_const_slice(): check_op_count(compound_const_slice, "BINARY_SLICE", 0) check_op_count(compound_const_slice, "BUILD_SLICE", 0) check_op_count(compound_const_slice, "STORE_SLICE", 0) - check_op_count(compound_const_slice, "BINARY_SUBSCR", 1) - check_consts(compound_const_slice, slice, {}) - check_consts(compound_const_slice, tuple, {(slice(1, 2, 3), slice(4, 5, 6))}) + check_op_count(compound_const_slice, "STORE_SUBSCR", 1) + check_consts(compound_const_slice, slice, []) + check_consts(compound_const_slice, tuple, [(slice(1, 2, 3), slice(4, 5, 6))]) def mutable_slice(): x[[]:] = y @@ -1462,12 +1464,12 @@ def different_but_equal(): check_consts( different_but_equal, slice, - { + [ slice(None, 0, None), slice(None, 0.0, None), slice(None, False, None), slice(None, None, None) - } + ] ) def test_compare_positions(self):