bpo-2506: Experiment with adding a "-X noopt" flag by 1st1 · Pull Request #9693 · python/cpython (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation17 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

1st1

I've created this PR to evaluate if we can add the -X noopt (or similar) option. So far I've done the following basic things:

Partial list of ToDo's

I don't have time to implement everything myself right now, but it might be a good idea to ask @elprans to work on this one in the next couple of months (@vstinner currently mentors Elvis.)

https://bugs.python.org/issue2506

(See also https://bugs.python.org/issue34888)

@1st1

@nedbat

@warsaw

I'm in favor of being able to disable the peephole optimizer, but I wonder if we need to rethink how we do -O and -OO to make it all more consistent. I don't like that we have those two options and -X noopt. Also, this is just a one-off option; what if we want to get more control over other types of optimization in the future? Maybe we should repurpose -O so you can do -O noopt or -Onoopt? The we might in the future say -Onoopt,nojit,slowonpurpose kind of thing?

@1st1

Test file

a = 1 + 2 while 3: print(a) b = 4

With -X noopt

$ python -X noopt -m dis < test.py
  1           0 LOAD_CONST               0 (1)
              2 LOAD_CONST               1 (2)
              4 BINARY_ADD
              6 STORE_NAME               0 (a)

  2     >>    8 LOAD_CONST               2 (3)
             10 POP_JUMP_IF_FALSE       22

  3          12 LOAD_NAME                1 (print)
             14 LOAD_NAME                0 (a)
             16 CALL_FUNCTION            1
             18 POP_TOP
             20 JUMP_ABSOLUTE            8

  4     >>   22 LOAD_CONST               3 (4)
             24 STORE_NAME               2 (b)
             26 LOAD_CONST               4 (None)
             28 RETURN_VALUE

Without -X noopt (current default Python 3.8 behaviour)

(note that line 2 was optimized away)

$ python -m dis < test.py
  1           0 LOAD_CONST               0 (3)
              2 STORE_NAME               0 (a)

  3     >>    4 LOAD_NAME                1 (print)
              6 LOAD_NAME                0 (a)
              8 CALL_FUNCTION            1
             10 POP_TOP
             12 JUMP_ABSOLUTE            4

  4          14 LOAD_CONST               1 (4)
             16 STORE_NAME               2 (b)
             18 LOAD_CONST               2 (None)
             20 RETURN_VALUE

@1st1

@warsaw I'd be OK with -O noopt. FWIW I'm also not a big fan of cramming all possible runtime flags into the new -X option.

@elprans

The we might in the future say -Onoopt,nojit,slowonpurpose kind of thing?

We already have three switchable compilation behaviors: peephole, NDEBUG (assert and __debug__ removal) and the __doc__ stripping, so, roughly:

-O[no-]peephole, -O[no-]strip-debug, and -O[no-]strip-doc.

Arguably, it might be useful to be able to enable docstring stripping without stripping assert as well, so granularity is good.

What's not clear at all is how this would interact with the bytecode cache.

@1st1

We already have three switchable compilation behaviors: peephole, NDEBUG (assert and debug removal) and the doc stripping, so, roughly:

We also have an AST optimizer (and Inada-san is working on making it possible to have that optimizer in Python). To simplify things we can make -O[no-]peephole disable the AST optimizer as well.

Arguably, it might be useful to be able to enable docstring stripping without stripping assert as well, so granularity is good.

I agree. Some packages [ab]use docstrings for meta programming and it's not possible to use those packages with -O to disable asserts.

What's not clear at all is how this would interact with the bytecode cache.

One option is to assign a bit flag to every -O variant. Then we can have the resulting integer (as hex?) as a suffix to pyc file: python_module.opt-{OPTHEX}.cpython-38.pyc

@1st1

@elprans

One option is to assign a bit flag to every -O variant.

Once the number of flags grows, this would quickly become unmaintainable, as system-installed packages will have to generate a .pyc for every flag combination. A solution for this is to allow writing the bytecode cache to a user-writable directory, which, together with PycInvalidationMode.CHECKED_HASH, might actually be doable now.

@1st1

Once the number of flags grows, this would quickly become unmaintainable, as system-installed packages will have to generate a .pyc for every flag combination.

Ah, got it.

@serhiy-storchaka

The main problem is interaction with the bytecode cache. See previous discussion. This option should either blindly set sys.dont_write_bytecode to True or set sys.flags.optimize to -1 (or both).

There are other optimizations in bytecode generation. No bytecode is generated for else: and pass lines. We could add NOPs with specific line numbers for tracing these lines.

@nedbat

I appreciate all of the activity! For my purposes, emitting lines for "else:" and "pass" isn't necessary, but I understand the philosophy of doing it.

@brettcannon

@elprans E.g. "-O[no-]peephole, -O[no-]strip-debug, and -O[no-]strip-doc" idea, there should be an open issue for this because @ambv has asked for the ability to turn off everything but assert removals. @vstinner and I talked about this when he was doing all of his AST manipulation work for FAT Python a while back.

And with .pyc files now storing their optimization levels this can be made to not impact .pyc files in terms of wiping out other .pyc files. And there is support for writing .pyc files to parallel directory location that got added after PyCon US 2018 thanks to Carl Meyer.

vstinner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .pyc filename should be different. I suggest ".noopt.pyc" suffix.

At least cache_from_source() and source_from_cache() of importlib._bootstrap_external have to be modified.

It's a little bit surprising to have sys.flags.optimize and sys.flags.noopt_mode. It seems redundant and a source of inconsistency, no? Would it be possible to modify sys.flags.optimize and _PyCoreConfig.optimization_level instead? For example, change the default optimization to level to 1, -X noopt would use 0.

Problem: doctest is hardcoded to skip tests if the optimization level is greater or equal to 2. doctest can easily be fixed, but maybe other 3rd party modules are hardcoded to rely on an exact optimization level.

@bedevere-bot

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

pfalcon added a commit to pfalcon/cpython that referenced this pull request

Jan 8, 2019

@pfalcon

…ytecode.

The patch (well, Py_INCREF) is based on python#9693 . Unlike that patch, this just forcibly skips optimizer, no command-line params, etc.

The purpose if producing reference bytecode output for initial stages of https://github.com/pfalcon/python-compiler upgrade to produce CPython3.5 compatible bytecode (later it's expected to implement peephole optimizer in Python, to use unmodified CPython code).

@nedbat

@1st1 What are your thoughts about pushing this forward?

@nedbat

@1st1 I would love to see this progress. Is there something I can do to help?

@vstinner

It seems like Yury @1st1 was too busy to finish this PR, so I created a new PR to finish it: PR #13600.

@terryjreedy

Victor's patch has been closed and followed by Pablo Salgado's #22027.