From a38e82bd8c249c126ab033c078170b6dea27a619 Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Thu, 7 Nov 2024 11:39:23 -0500 Subject: [PATCH] gh-126298: Don't deduplicate slice constants based on equality (#126398) * gh-126298: Don't deduplicated slice constants based on equality * NULL check for PySlice_New * Fix refcounting * Fix refcounting some more * Fix refcounting * Make tests more complete * Fix tests --- Lib/test/test_compile.py | 78 ++++++++++++++++++++++++++++++---------- Objects/codeobject.c | 35 +++++++++++++++++- 2 files changed, 93 insertions(+), 20 deletions(-) diff --git a/Lib/test/test_compile.py b/Lib/test/test_compile.py index 85ae71c1f77..519a1207afb 100644 --- a/Lib/test/test_compile.py +++ b/Lib/test/test_compile.py @@ -2,6 +2,7 @@ import contextlib import dis import io import itertools +import marshal import math import opcode import os @@ -1385,52 +1386,91 @@ class TestSpecifics(unittest.TestCase): self.assertEqual(actual, expected) def check_consts(func, typ, expected): - slice_consts = 0 + 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): - slice_consts += 1 - self.assertEqual(slice_consts, expected) + all_consts.add(repr(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)]) + check_op_count(load, "BINARY_SUBSCR", 1) + def store(): x[a:b] = 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(): 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, []) + check_op_count(long_slice, "BINARY_SUBSCR", 1) + 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_op_count(aug, "BINARY_SUBSCR", 0) + check_op_count(aug, "STORE_SUBSCR", 0) + check_consts(aug, slice, []) + 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(): 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_op_count(compound_const_slice, "STORE_SLICE", 0) + 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 + + check_consts(mutable_slice, slice, {}) + + 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 1cf9740af9a..dba43d5911d 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -2388,7 +2388,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)) { @@ -2496,6 +2495,40 @@ _PyCode_ConstantKey(PyObject *op) Py_DECREF(set); return key; } + else if (PySlice_Check(op)) { + PySliceObject *slice = (PySliceObject *)op; + PyObject *start_key = NULL; + PyObject *stop_key = NULL; + PyObject *step_key = NULL; + key = NULL; + + start_key = _PyCode_ConstantKey(slice->start); + if (start_key == NULL) { + goto slice_exit; + } + + stop_key = _PyCode_ConstantKey(slice->stop); + if (stop_key == NULL) { + goto slice_exit; + } + + step_key = _PyCode_ConstantKey(slice->step); + if (step_key == NULL) { + goto slice_exit; + } + + PyObject *slice_key = PySlice_New(start_key, stop_key, step_key); + if (slice_key == 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 * to ensure that they are seen as unequal. */