bpo-2506: Add mechanism to disable optimizations #22027

pablogsal · 2020-08-31T13:11:59Z

https://bugs.python.org/issue2506

pablogsal · 2020-08-31T13:23:08Z

Here we go again....:)

vstinner

My brain always fail to understand a double negation: noopt=False. What about:

use_optimization=True
with_optimization=True
enable_optimization=True
disable_optimization=False (still double negation)
optimization=True ... the annoying part with this one is that compile() already has optimize parameter and there is already sys.flags.optimize, whereas cache_from_source() already has optimization :-(

To me, with_optimization=True makes sense (optimize), as well as with_optimization=False (don't optimize).

with fits well with an action like "compile". But it doesn't fit well with the global sys.flags. Maybe use_optimization=True is better.

Doc/c-api/init_config.rst

Doc/library/py_compile.rst

Include/compile.h

Include/cpython/initconfig.h

Add -X noopt command line option to disable compiler optimizations. distutils ignores noopt: always enable compiler optimizations. * Add sys.flags.noopt flag. * Add 'noopt' keyword-only parameter to builtin compile(), importlib.util.cache_from_source() and py_compile.compile(). * importlib: SourceLoader gets an additional keyword-only '_noopt' parameter. It is used by py_compile.compile(). * importlib uses ``.noopt.pyc`` suffix for .pyc files if sys.flags.noopt is true. * Add PyConfig.optimize parameter. * Update subprocess._optim_args_from_interpreter_flags(). * Add support.requires_compiler_optimizations() * Update unit tests which rely on compiler optimizations for noopt: add @support.requires_compiler_optimizations. * Rename c_optimize to c_optimization_level in compiler.c Co-Authored-By: Yury Selivanov <yury@magic.io> Co-Authored-By: Victor Stinner <vstinner@python.org>

pablogsal · 2020-08-31T14:06:46Z

My brain always fail to understand a double negation: noopt=False. What about:

use_optimization=True

with_optimization=True

enable_optimization=True

disable_optimization=False (still double negation)

optimization=True ... the annoying part with this one is that compile() already has optimize parameter and there is already sys.flags.optimize, whereas cache_from_source() already has optimization :-(

To me, with_optimization=True makes sense (optimize), as well as with_optimization=False (don't optimize).

with fits well with an action like "compile". But it doesn't fit well with the global sys.flags. Maybe use_optimization=True is better.

I was considering doing this but the problem is that all options are too similar to optimization=... and that raises more confusion due to 'what do I have an option to set the optimization and another to deactivate the option I just used?'. Given that using disable_optimizations=False will likely never be used (the default is None) I would prefer to use disable_optimizations.

Another option is to deprecate optimization (still keeping it), rename it to optimization_level and then using enable_optimization. What do you think?

pablogsal · 2020-08-31T14:23:01Z

Idea, maybe to avoid this problem we can pass the disabling options as a compiler flag. This has the advantage that we don't need an extra parameter and feels a bit more justified IMHO.

Unless anyone has a problem with this I will go ahead with this version.

vstinner · 2020-09-01T09:22:42Z

Another option is to deprecate optimization (still keeping it), rename it to optimization_level and then using enable_optimization. What do you think?

Slow deprecations and migrations never end and confuse everybody. It's too late to change the parameter name. I only see two options:

Add a new parameter (what your PR does): we should just pick the "best" name to reduce confusion.
Add a new marker value to disable optimizations, and so reuse the existing parameter. For example, optimization="noopt" in Python and -Onoopt on the command line. There are many choices for the special values: singleton object, string, "noopt", "disable", "off", etc.

We can use a different API for public functions like compile() and the command line interface, and for the internal C implementation (use two parameters).

Sadly, the optimization level is also passed as "optimization" in some public C API functions.

pablogsal · 2020-09-01T09:30:21Z

Another option is to deprecate optimization (still keeping it), rename it to optimization_level and then using enable_optimization. What do you think?

Slow deprecations and migrations never end and confuse everybody. It's too late to change the parameter name. I only see two options:

Add a new parameter (what your PR does): we should just pick the "best" name to reduce confusion.

Add a new marker value to disable optimizations, and so reuse the existing parameter. For example, optimization="noopt" in Python and -Onoopt on the command line. There are many choices for the special values: singleton object, string, "noopt", "disable", "off", etc.

We can use a different API for public functions like compile() and the command line interface, and for the internal C implementation (use two parameters).

Sadly, the optimization level is also passed as "optimization" in some public C API functions.

I have opted to implement this as a new bit that can be passed to the flags parameter. The only downside is that is not possible to enable optimizations per-compile call if you deactivate them globally using -X noopt, but that should never be something that is needed. Also, the use-case described by @nedbat does not need even this level of customizability.

pablogsal · 2020-09-02T20:23:12Z

@vstinner I would like to land this PR once and for all so it does not die in spacetime like the other two attempts. When you have some time, could you do a final review? We could address some minor details in other PRs but given the change, the important thing here is having the bulk of the work so reviewing other future PRs will be easier.

The important things to consider:

We deactivate the optimizations globally with -X noopt
We deactivate the optimizations per compile call using a new flag: ast.PyCF_DISABLE_ALL_OPTIMIZATIONS. Using a flag allows us to not have to add a new parameter to lots of APIs everywhere so the change is less invasive.

pablogsal · 2020-09-02T20:36:42Z

@nedbat Please, could you build this patch and check that this works as expected on coverage.py? Let's make sure this works as expected. As I said before, you can either use the -X noopt flag or to pass ast.PyCF_DISABLE_ALL_OPTIMIZATIONS in the compiler flags.

Include/compile.h

nedbat · 2020-09-05T13:58:34Z

@pablogsal Thanks for pushing this forward. Is there a way to enable this with an environment variable? I'm not sure how best to use this during a test run.

Can you find me on IRC? nedbat

nedbat · 2020-09-05T14:10:26Z

Also, coverage.py will need to take this flag into account where it already tries to understand that optimizations might happen. If I hard-code "noopt" in the interpreter, the coverage.py test suite gets these failures:

FAILED tests/test_arcs.py::LoopArcTest::test_bug_496_continue_in_constant_while - AssertionError: 3 failed assertions:
FAILED tests/test_arcs.py::LoopArcTest::test_while_true - AssertionError: 3 failed assertions:
FAILED tests/test_arcs.py::OptimizedIfTest::test_if_debug - AssertionError: 2 failed assertions:
FAILED tests/test_arcs.py::OptimizedIfTest::test_optimized_away_if_1 - AssertionError: Lists differ: [1, 2, 3, 4, 5, 6, 8, 9] != [1, 2, 3, 5, 6, 9]
FAILED tests/test_arcs.py::OptimizedIfTest::test_if_not_debug - AssertionError: 2 failed assertions:
FAILED tests/test_arcs.py::OptimizedIfTest::test_optimized_nested - AssertionError: Lists differ: [1, 2, 3, 4, 6, 8, 9, 11, 12, 13, 14, 15] != [1, 12, 14, 15]
FAILED tests/test_arcs.py::LoopArcTest::test_zero_coverage_while_loop - AssertionError: 'zero.py 2 2 0 0 0% 1-3' not found in 'zero.py 3 3 0 0 0% 1-3'
FAILED tests/test_arcs.py::OptimizedIfTest::test_optimized_away_if_0 - AssertionError: Lists differ: [1, 2, 3, 4, 5, 6, 8, 9] != [1, 2, 3, 8, 9]
FAILED tests/test_html.py::HtmlGoldTests::test_partial - AssertionError: Files differ: /Users/ned/coverage/trunk/tests/gold/html/partial/index.html != out/pa...
FAILED tests/test_coverage.py::CompoundStatementTest::test_constant_if - AssertionError: Lists differ: [1, 2, 3] != [2, 3]

pablogsal · 2020-09-05T18:38:22Z

Also, coverage.py will need to take this flag into account where it already tries to understand that optimizations might happen. If I hard-code "noopt" in the interpreter, the coverage.py test suite gets these failures:

Sorry, I am not sure I understand what I am reading. Are these failures due to some code in coverage.py trying to understand the optimizations (and therefore coverage.py should update its code to handle it or this is due to something wrong on our side (or on this PR)? Are thos 8000 e lists line numbers?

nedbat · 2020-09-06T12:56:46Z

Those tests are checks that coverage.py is correctly understanding the optimizations that happen. With "noopt", the tests fail, because the optimizations aren't happening. Coverage.py would need to take "noopt" into account when reporting on what lines could have run.

nedbat · 2020-09-07T13:01:54Z

Digging into this, I'm beginning to think the unintended consequences are not worth the change. Definitely this will need an environment variable to control it if it makes it to master.

pablogsal · 2020-09-07T17:54:50Z

Digging into this, I'm beginning to think the unintended consequences are not worth the change. Definitely this will need an environment variable to control it if it makes it to master.

We could add the environment variable, but could you elaborate both points? What unintended consequences? Why you need an environment variable?

nedbat · 2020-09-07T19:05:38Z

Unintended Consequences

Coverage.py works in three main phases:

Measure what lines of code were run
Determine what lines of code could ever possibly be run
Report on the difference between 1 and 2.

Keep in mind, "lines" here really means, lines or transitions between lines in the case of branch coverage.

When determining what lines of code could possibly be run, coverage.py takes into account the version of Python, and the optimizations done by that version. For example (taken from a test in the coverage.py test suite):

a, i = 1, 0                     # line 1
while 1:                        # 2
    if i >= 3:                  # 3
        a = 4                   # 4
        break                   # 5
    i += 1                      # 6
assert a == 4 and i == 3        # 7

In Python up until 3.8, there is a line event traced on line 2, setting up the loop. In Python 3.8, this was optimized away, and line 1 is followed by line 3. Coverage.py understands that this will happen. If it did not, it would treat 3.7 and 3.8 the same, and on 3.8 would report that line 2 didn't run when it should have.

With -X noopt, coverage.py now has to take into account whether this option is in effect or not, so that it can again expect that line 2 will be executed.

Another example (also from test_arcs.py):

a = 1
if len([2]):
    c = 3
if 0:               # line 4
    if len([5]):
        d = 6
else:
    e = 8
f = 9

Normally the "if 0" is optimized away, and lines 4, 5, and 6 are completely missing from the compiled code. With noopt, they are all present. I haven't finished working on this case, so I'm not sure how much adaptation it will need, but something will need to be done.

Environment Variable support

We need an environment variable to enable "noopt" because Python code, and in particular, test suites, get run too many different ways to assume that it will literally be a "python" command, and that an option can be provided to it.

The command running the test suite might be "pytest", or "coverage run". The pytest runner can use xdist to spawn multiple processes to run tests in parallel. xdist will copy the environment variable from the parent, but doesn't know about "noopt". With an environment variable, I can run my pytest-xdist test runner entirely without optimization. Without an environment variable, I cannot.

Providing an environment variable offers the greatest flexibility when running test suites.

nedbat · 2020-09-07T19:15:55Z

pablogsal#4 adds the environment variable.

PYTHONNOOPT=1 will set the noopt flag

pablogsal · 2020-09-07T19:21:22Z

Digging into this, I'm beginning to think the unintended consequences are not worth the change

This adds some maintenance burden for us but it seems that it may add even more to coverage.py. Given that coverage.py will be the main consumer of this, I would say that it's your call if we should try to continue iterating or abandoning the idea.

pablogsal · 2020-09-07T19:22:07Z

Btw, thanks for the thorough explanation about the unintended consequences and why the env var is needed

markshannon · 2020-12-22T21:37:01Z

I think PEP 626 makes this unnecessary.
Any reasons other than line tracing to have a "no opt" flag?

pablogsal · 2020-12-22T21:38:48Z

Any reasons other than line tracing to have a "no opt" flag?

I don't think so. I have to note that I the more I iterate over this the more I realize that this is quite a complex task with no much gain, honestly. Is not even possible to have an API that I am happy about.

nedbat · 2020-12-22T21:46:39Z

Mark, I appreciate your attention to this, but please, let's figure out where we end up with PEP 626 before deciding. Honestly, I've been wondering whether going back to this pull request might not be the way to go.

pablogsal requested a review from gpshead as a code owner August 31, 2020 13:11

the-knights-who-say-ni added the 8000 CLA signed label Aug 31, 2020

bedevere-bot added the awaiting core review label Aug 31, 2020

pablogsal force-pushed the noopt branch 2 times, most recently from 2e9b121 to 1699ba0 Compare August 31, 2020 13:23

pablogsal force-pushed the noopt branch 2 times, most recently from d32ec2a to 45bbaef Compare August 31, 2020 13:37

vstinner reviewed Aug 31, 2020

View reviewed changes

Doc/c-api/init_config.rst Outdated Show resolved Hide resolved

Doc/library/py_compile.rst Show resolved Hide resolved

Include/compile.h Outdated Show resolved Hide resolved

Include/cpython/initconfig.h Outdated Show resolved Hide resolved

pablogsal force-pushed the noopt branch from 45bbaef to 445778f Compare August 31, 2020 13:40

python deleted a comment from vstinner Aug 31, 2020

terryjreedy mentioned this pull request Aug 31, 2020

bpo-2506: Experiment with adding a "-X noopt" flag #9693

Closed

8 tasks

fixup! bpo-2506: Add -X noopt command line option

1b9e8f6

pablogsal force-pushed the noopt branch from e83623c to f6a827c Compare August 31, 2020 16:32

pablogsal requested a review from lysnikolaou as a code owner August 31, 2020 16:32

pablogsal force-pushed the noopt branch from f6a827c to 21fb518 Compare August 31, 2020 16:41

Use a compiler flag instead of an extra argument

c558ad5

pablogsal force-pushed the noopt branch from 21fb518 to c558ad5 Compare September 1, 2020 09:44

pablogsal requested a review from vstinner September 2, 2020 20:19

Fix docs

eb0cddb

vstinner reviewed Sep 4, 2020

View reviewed changes

Include/compile.h Outdated Show resolved Hide resolved

Fix the value of the new flag

a59ecce

PYTHONNOOPT=1 will set the noopt flag

9d7f340

Merge pull request #4 from nedbat/nedbat/22027-envvar

a635867

PYTHONNOOPT=1 will set the noopt flag

nedbat added a commit to nedbat/coveragepy that referenced this pull request Sep 12, 2020

Adapt to python/cpython#22027

1c13cb9

pablogsal closed this Mar 5, 2021

pablogsal deleted the noopt branch May 19, 2021 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

bpo-2506: Add mechanism to disable optimizations #22027

bpo-2506: Add mechanism to disable optimizations #22027

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bpo-2506: Add mechanism to disable optimizations #22027

bpo-2506: Add mechanism to disable optimizations #22027

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Unintended Consequences

Environment Variable support

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!