gh-126298: Don't deduplicate slice constants based on equality #126398

mdboom · 2024-11-04T14:07:42Z

Issue: Constant slices are deduplicated too aggressively #126298

brandtbucher

Looks good! Just a typo in one test, and some further suggestions for completeness:

brandtbucher · 2024-11-04T20:05:05Z

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):


I think everywhere this is used it's called with 0 expected. Maybe just replace those calls with check_consts(func, typ, set()) since both functions are so similar?

brandtbucher · 2024-11-04T20:06:20Z

Lib/test/test_compile.py

        def store():
            x[a:b] = y
-            x [a:] = y
+            x[a:] = y


This whitespace might be intentional... load has the exact same whitespace above. We are testing the compiler, after all.

I'm inclined to just leave it.

brandtbucher · 2024-11-04T20:08:23Z

Lib/test/test_compile.py


        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)


Suggested change

check_op_count(load, "BUILD_SLICE", 0)

check_op_count(load, "BUILD_SLICE", 0)

check_op_count(load, "BINARY_SUBSCR", 1)

brandtbucher · 2024-11-04T20:08:33Z

Lib/test/test_compile.py

            x[:b] = y
            x[:] = y

+        check_op_count(store, "STORE_SLICE", 3)
+        check_op_count(store, "BUILD_SLICE", 0)


Suggested change

check_op_count(store, "BUILD_SLICE", 0)

check_op_count(store, "BUILD_SLICE", 0)

check_op_count(store, "STORE_SUBSCR", 1)

brandtbucher · 2024-11-04T20:08:47Z

Lib/test/test_compile.py

        def long_slice():
            return x[a:b:c]

+        check_op_count(long_slice, "BUILD_SLICE", 1)
+        check_op_count(long_slice, "BINARY_SLICE", 0)


Suggested change

check_op_count(long_slice, "BINARY_SLICE", 0)

check_op_count(long_slice, "BINARY_SLICE", 0)

check_op_count(long_slice, "BINARY_SUBSCR", 1)

brandtbucher · 2024-11-04T20:10:27Z

Lib/test/test_compile.py

+        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)


Suggested change

check_num_consts(long_slice, slice, 0)

check_num_consts(aug, slice, 0)

brandtbucher · 2024-11-04T20:11:58Z

Lib/test/test_compile.py

        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)


Suggested change

check_op_count(aug, "BUILD_SLICE", 0)

check_op_count(aug, "BUILD_SLICE", 0)

check_op_count(aug, "BINARY_SUBSCR", 0)

check_op_count(aug, "STORE_SUBSCR", 0)

brandtbucher · 2024-11-04T20:12:36Z

Lib/test/test_compile.py

        def aug_const():
            x[1:2] += y

+        check_op_count(aug_const, "BINARY_SLICE", 0)
+        check_op_count(aug_const, "STORE_SLICE", 0)


Suggested change

check_op_count(aug_const, "STORE_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)

brandtbucher · 2024-11-04T20:13:44Z

Lib/test/test_compile.py

-        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)


Suggested change

check_op_count(compound_const_slice, "BUILD_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)

TeamSpen210 · 2024-11-04T20:46:38Z

Lib/test/test_compile.py

@@ -1385,52 +1385,90 @@ def check_op_count(func, op, expected):
            self.assertEqual(actual, expected)

        def check_consts(func, typ, expected):
-            slice_consts = 0
+            all_consts = set()


I don't think this is working correctly - slices just defer to their contents for hash/equality, so this will be deduplicating the slices in the same way this is trying to fix.

Ooh, nice catch!

…ython#126398) * pythongh-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

mdboom requested a review from brandtbucher November 4, 2024 14:07

mdboom requested a review from markshannon as a code owner November 4, 2024 14:07

bedevere-app bot added the awaiting core review label Nov 4, 2024

bedevere-app bot mentioned this pull request Nov 4, 2024

Constant slices are deduplicated too aggressively #126298

Closed

mdboom added skip news and removed awaiting core review labels Nov 4, 2024

pythongh-126298: Don't deduplicated slice constants based on equality

af304c8

mdboom force-pushed the slice-constant-fix branch from da12eed to af304c8 Compare November 4, 2024 14:08

mdboom added 4 commits November 4, 2024 09:13

NULL check for PySlice_New

1d55e6e

Fix refcounting

8d70094

Fix refcounting some more

1fc93c7

Fix refcounting

3cd03e0

mdboom changed the title ~~gh-126298: Don't deduplicated slice constants based on equality~~ gh-126298: Don't deduplicate slice constants based on equality Nov 4, 2024

brandtbucher approved these changes Nov 4, 2024

View reviewed changes

bedevere-app bot added the awaiting merge label Nov 4, 2024

Make tests more complete

b2b6149

TeamSpen210 reviewed Nov 4, 2024

View reviewed changes

mdboom added 2 commits November 7, 2024 11:09

Fix tests

3a08929

Merge branch 'main' into slice-constant-fix

6eb1ffa

mdboom enabled auto-merge (squash) November 7, 2024 16:13

mdboom merged commit a38e82b into python:main Nov 7, 2024
37 checks passed

bedevere-app bot removed the awaiting merge label Nov 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-126298: Don't deduplicate slice constants based on equality #126398

gh-126298: Don't deduplicate slice constants based on equality #126398

mdboom commented Nov 4, 2024 •

edited by bedevere-app bot

Loading

brandtbucher left a comment

brandtbucher Nov 4, 2024

brandtbucher Nov 4, 2024

brandtbucher Nov 4, 2024

brandtbucher Nov 4, 2024

brandtbucher Nov 4, 2024

brandtbucher Nov 4, 2024

brandtbucher Nov 4, 2024 •

edited

Loading

brandtbucher Nov 4, 2024

brandtbucher Nov 4, 2024

TeamSpen210 Nov 4, 2024

brandtbucher Nov 4, 2024

	check_op_count(load, "BUILD_SLICE", 0)
	check_op_count(load, "BUILD_SLICE", 0)
	check_op_count(load, "BINARY_SUBSCR", 1)

	check_op_count(store, "BUILD_SLICE", 0)
	check_op_count(store, "BUILD_SLICE", 0)
	check_op_count(store, "STORE_SUBSCR", 1)

	check_op_count(long_slice, "BINARY_SLICE", 0)
	check_op_count(long_slice, "BINARY_SLICE", 0)
	check_op_count(long_slice, "BINARY_SUBSCR", 1)

	check_num_consts(long_slice, slice, 0)
	check_num_consts(aug, slice, 0)

gh-126298: Don't deduplicate slice constants based on equality #126398

gh-126298: Don't deduplicate slice constants based on equality #126398

Conversation

mdboom commented Nov 4, 2024 • edited by bedevere-app bot Loading

brandtbucher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandtbucher Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdboom commented Nov 4, 2024 •

edited by bedevere-app bot

Loading

brandtbucher Nov 4, 2024 •

edited

Loading