Peeephole optimizer does not optimize functions with multiline expressions

[serhiy-storchaka]

BPO	37213
Nosy	@vstinner, @benjaminp, @ned-deily, @serhiy-storchaka, @pablogsal, @miss-islington
PRs	bpo-37213: Handle negative line deltas correctly in the peephole optimizer #13969 [3.8] bpo-37213: Handle negative line deltas correctly in the peephole optimizer (GH-13969) #14063
Files	test_peepholer.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/pablogsal'
closed_at = <Date 2019-06-13.19:01:58.014>
created_at = <Date 2019-06-10.08:24:23.404>
labels = ['interpreter-core', 'type-bug', '3.8', '3.9']
title = 'Peeephole optimizer does not optimize functions with multiline expressions'
updated_at = <Date 2019-06-14.09:49:54.774>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2019-06-14.09:49:54.774>
actor = 'vstinner'
assignee = 'pablogsal'
closed = True
closed_date = <Date 2019-06-13.19:01:58.014>
closer = 'pablogsal'
components = ['Interpreter Core']
creation = <Date 2019-06-10.08:24:23.404>
creator = 'serhiy.storchaka'
dependencies = []
files = ['48412']
hgrepos = []
issue_num = 37213
keywords = ['patch', '3.8regression']
message_count = 13.0
messages = ['345108', '345117', '345120', '345268', '345309', '345529', '345530', '345531', '345532', '345534', '345535', '345562', '345566']
nosy_count = 6.0
nosy_names = ['vstinner', 'benjamin.peterson', 'ned.deily', 'serhiy.storchaka', 'pablogsal', 'miss-islington']
pr_nums = ['13969', '14063']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue37213'
versions = ['Python 3.8', 'Python 3.9']

[serhiy-storchaka]

The optimization is skipped if lnotab contains 255. It was very uncommon in older versions (only when the function contains very large expressions, larger than hundreds of lines or bytecode instructions), but in 3.8 this situation is common.

For example:

[x
for x in a if x]

1 0 BUILD_LIST 0
2 LOAD_FAST 0 (.0)
>> 4 FOR_ITER 12 (to 18)

2 6 STORE_FAST 1 (x)
8 LOAD_FAST 1 (x)
10 POP_JUMP_IF_FALSE 16

1 12 LOAD_FAST 1 (x)
14 LIST_APPEND 2
>> 16 JUMP_ABSOLUTE 4
>> 18 RETURN_VALUE

if x:
    if (y and
        z):
        foo()
else:
    bar()

1 0 LOAD_NAME 0 (x)
2 POP_JUMP_IF_FALSE 20

2 4 LOAD_NAME 1 (y)
6 POP_JUMP_IF_FALSE 18

3 8 LOAD_NAME 2 (z)

2 10 POP_JUMP_IF_FALSE 18

4 12 LOAD_NAME 3 (foo)
14 CALL_FUNCTION 0
16 POP_TOP
>> 18 JUMP_FORWARD 6 (to 26)

6 >> 20 LOAD_NAME 4 (bar)
22 CALL_FUNCTION 0
24 POP_TOP
>> 26 LOAD_CONST 0 (None)
28 RETURN_VALUE

You can see non-optimized jumps to jumps (from 10 to 16 and from 6 and 10 to 16 correspondingly).

This is a consequence of two features: ability to encode negative line differences in lnotab and setting lines for both outer and inner expressions.

Two ways to solve this issue:

1. Move optimizations from Python/peephole.c to Python/compile.c (see bpo-32477 and bpo-33318). This is a new feature and it is too late for 3.8.

2. Make the peepholer to work with lnotab containing 255.

Pablo, are you interesting?

[pablogsal]

Thank you very much, Serhiy!

I am interested, I will try to look at the problem and try to get a PR soon.

What of the two possible solutions that you mention you think is better? I assume if we make the peephole optimizer work with lnotab containing 255 we could backport to 3.8 as a bugfix, right?

[serhiy-storchaka]

Yes, we should backport the fix to 3.8. There is a bug in 3.8.

[vstinner]

I removed the memchr(255) to see which tests fail:

test_extended_opargs (test.test_modulefinder.ModuleFinderTest) ... python: Python/peephole.c:469: PyCode_Optimize: Assertion `cum_orig_offset % sizeof(_Py_CODEUNIT) == 0' failed.
Fatal Python error: Aborted

test_extended_arg (test.test_compile.TestSpecifics) ... python: Python/peephole.c:469: PyCode_Optimize: Assertion `cum_orig_offset % sizeof(_Py_CODEUNIT) == 0' failed.
Fatal Python error: Aborted

test_field_named_like_builtin (test.test_dataclasses.TestCase) ... python: Python/peephole.c:469: PyCode_Optimize: Assertion `cum_orig_offset % sizeof(_Py_CODEUNIT) == 0' failed.
Fatal Python error: Aborted

test_field_named_like_builtin_frozen (test.test_dataclasses.TestCase) ... python: Python/peephole.c:469: PyCode_Optimize: Assertion `cum_orig_offset % sizeof(_Py_CODEUNIT) == 0' failed.
Fatal Python error: Aborted

Does test_compile have unit tests test_modulefinder and test_dataclasses cases?

[serhiy-storchaka]

test_peepholer.diff adds tests which cover various peepholer optimizations not tested before. All of them are failed now and should be passed after merging PR 13969. Pablo, you can include these tests in PR 13969.

[pablogsal]

New changeset 3498c64 by Pablo Galindo in branch 'master':
bpo-37213: Handle negative line deltas correctly in the peephole optimizer (GH-13969)
3498c64

[vstinner]

The optimization is skipped if lnotab contains 255. It was very uncommon in older versions (only when the function contains very large expressions, larger than hundreds of lines or bytecode instructions), but in 3.8 this situation is common.

Do you know why 255 became more common? Is it the side effect if an AST optimization?

[miss-islington]

New changeset 5282b3b by Miss Islington (bot) in branch '3.8':
bpo-37213: Handle negative line deltas correctly in the peephole optimizer (GH-13969)
5282b3b

[pablogsal]

Should we backport this to 3.7 as well?

[ned-deily]

Should we backport this to 3.7 as well?

Not unless someone can show how this is a major problem in 3.7 and then only if the changes will not introduce any 3.7.x compatibility problems.

[pablogsal]

Not unless someone can show how this is a major problem in 3.7

I would say is not a major problem in 3.7

I will close the issue then. Thanks everyone who participated!

[serhiy-storchaka]

Do you know why 255 became more common?

Because the line number is now correctly set for every bytecode instruction.

Compare the output in msg345108 for 3.8 with the corresponding output in 3.7:

1 0 BUILD_LIST 0
2 LOAD_FAST 0 (.0)
>> 4 FOR_ITER 12 (to 18)

2 6 STORE_FAST 1 (x)
8 LOAD_FAST 1 (x)
10 POP_JUMP_IF_FALSE 4
12 LOAD_FAST 1 (x)
14 LIST_APPEND 2
16 JUMP_ABSOLUTE 4
>> 18 RETURN_VALUE

[vstinner]

Because the line number is now correctly set for every bytecode instruction.

That's a great enhancement! Should it be documented in https://docs.python.org/3.8/whatsnew/3.8.html ?