classification
Title: Peeephole optimizer does not optimize functions with multiline expressions
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pablogsal Nosy List: benjamin.peterson, miss-islington, ned.deily, pablogsal, serhiy.storchaka, vstinner
Priority: normal Keywords: 3.8regression, patch

Created on 2019-06-10 08:24 by serhiy.storchaka, last changed 2019-06-14 09:49 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
test_peepholer.diff serhiy.storchaka, 2019-06-12 06:00
Pull Requests
URL Status Linked Edit
PR 13969 merged pablogsal, 2019-06-11 10:14
PR 14063 merged miss-islington, 2019-06-13 18:17
Messages (13)
msg345108 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-06-10 08:24
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 issue32477 and issue33318). 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?
msg345117 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-06-10 10:12
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?
msg345120 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-06-10 10:39
Yes, we should backport the fix to 3.8. There is a bug in 3.8.
msg345268 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-11 20:53
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?
msg345309 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-06-12 06:00
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.
msg345529 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-06-13 18:16
New changeset 3498c642f4e83f3d8e2214654c0fa8e0d51cebe5 by Pablo Galindo in branch 'master':
bpo-37213: Handle negative line deltas correctly in the peephole optimizer (GH-13969)
https://github.com/python/cpython/commit/3498c642f4e83f3d8e2214654c0fa8e0d51cebe5
msg345530 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-13 18:30
> 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?
msg345531 - (view) Author: miss-islington (miss-islington) Date: 2019-06-13 18:36
New changeset 5282b3b1d2e0bdf13899b1616aea20a6e3c4e13e by Miss Islington (bot) in branch '3.8':
bpo-37213: Handle negative line deltas correctly in the peephole optimizer (GH-13969)
https://github.com/python/cpython/commit/5282b3b1d2e0bdf13899b1616aea20a6e3c4e13e
msg345532 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-06-13 18:51
Should we backport this to 3.7 as well?
msg345534 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2019-06-13 19:00
> 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.
msg345535 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-06-13 19:01
> 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!
msg345562 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-06-14 07:10
> 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
msg345566 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-14 09:49
> 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 ?
History
Date User Action Args
2019-06-14 09:49:54vstinnersetmessages: + msg345566
2019-06-14 07:10:34serhiy.storchakasetmessages: + msg345562
2019-06-13 19:01:58pablogsalsetstatus: open -> closed
resolution: fixed
messages: + msg345535

stage: patch review -> resolved
2019-06-13 19:00:03ned.deilysetnosy: + ned.deily
messages: + msg345534
2019-06-13 18:51:17pablogsalsetmessages: + msg345532
2019-06-13 18:36:03miss-islingtonsetnosy: + miss-islington
messages: + msg345531
2019-06-13 18:30:07vstinnersetmessages: + msg345530
2019-06-13 18:17:04miss-islingtonsetpull_requests: + pull_request13923
2019-06-13 18:16:26pablogsalsetmessages: + msg345529
2019-06-12 06:00:27serhiy.storchakasetfiles: + test_peepholer.diff

messages: + msg345309
2019-06-11 20:54:52yselivanovsetnosy: - yselivanov
2019-06-11 20:53:44vstinnersetmessages: + msg345268
2019-06-11 10:14:31pablogsalsetkeywords: + patch
stage: patch review
pull_requests: + pull_request13836
2019-06-10 10:39:04serhiy.storchakasetmessages: + msg345120
2019-06-10 10:12:40pablogsalsetassignee: pablogsal
messages: + msg345117
2019-06-10 08:32:54serhiy.storchakasetkeywords: + 3.8regression
2019-06-10 08:24:23serhiy.storchakacreate