bpo-2506: Add mechanism to disable optimizations

[pablogsal]

https://bugs.python.org/issue2506

[pablogsal]

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.

[pablogsal]

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]

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]

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]

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]

@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]

@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.

[nedbat]

@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]

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]

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 those lists line numbers?

[nedbat]

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]

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]

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]

Unintended Consequences

Coverage.py works in three main phases:

1. Measure what lines of code were run
2. Determine what lines of code could ever possibly be run
3. 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]

pablogsal#4 adds the environment variable.

[pablogsal]

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]

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

[markshannon]

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

[pablogsal]

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]

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.