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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add mechanism to disable optimizations #46758
Comments
When tracing line execution with sys.settrace, a particular code Attached is a file with two copies of the same code, except in the first In the second code, the continue is properly reported. This bug has been present since version 2.3. 2.2 does not exhibit it To see the problem, execute "trace.py -c -m continue.py". Then 1: a = b = c = 0
101: for n in range(100):
100: if n % 2:
50: if n % 4:
50: a += 1
>>>>>> continue
else:
50: b += 1
50: c += 1
1: assert a == 50 and b == 50 and c == 50
101: for n in range(100): |
This is because of a "peephole" optimization of the generated bytecode: You gain a few opcodes, but tracing is confusing... |
I think this is not a bug. Here is a simpler way to illustrate the def f(x):
for i in range(10):
if x:
pass
continue
f(True)
f(False) If you run the code above under trace, you get the following coverage:
22: for i in range(10): Note that the 'continue' line is executed 10 instead of expected 20 2 0 SETUP_LOOP 34 (to 37) 3 19 LOAD_FAST 0 (x) 4 26 JUMP_ABSOLUTE 13 5 30 JUMP_ABSOLUTE 13 Note how peephole optimizer replaced jump to the 'continue' line (5) 4 26 JUMP_FORWARD 1 (to 30) with 4 26 JUMP_ABSOLUTE 13 I say this is not a bug because trace is correct in showing that the |
I see that the cause of the problem is the peephole optimizer. That I am measuring the code coverage of a set of tests, and one of my lines I don't know what the solution to this is. Some options include fixing |
On Sat, Mar 29, 2008 at 2:51 PM, Ned Batchelder <report@bugs.python.org> wrote:
.. but the continue statement on line 5 is NOT executed in x == True 3 19 LOAD_FAST 0 (x) 4 26 JUMP_FORWARD 1 (to 30) 5 >> 30 JUMP_ABSOLUTE 13 where the second jump is to the continue statement. Peephole 3 19 LOAD_FAST 0 (x) 4 26 JUMP_ABSOLUTE 13 5 30 JUMP_ABSOLUTE 13 If x is true, line five is NOT executed.
I think it is a good idea to provide a way to disable peephole I can easily implement -N (No optimization) or -g (debug) option that |
Microsoft compilers use -Od to disable optimization... |
This has basically almost never been a problem in the real world. No Also, when the peepholer is moved (after the AST is created, but before Recommend closing as "won't fix". |
I recognize that this is an unusual case, but it did come up in the real I don't understand "when the peepholer is moved", so maybe you are right In any case, I'd like to know more about the changes planned for the AST |
On Sat, Mar 29, 2008 at 4:58 PM, Raymond Hettinger
I believe Ned gave an important use case. In coverage testing,
This would not really be a new option. Most users expect varying
I don't see how moving optimization up the chain will help with this |
Weigh the cost/benefit carefully before pushing further. I don't doubt My feeling is that adding a new compiler option using a cannon to kill This little buglet has been around since Py2.3. That we're only It would be *much* more useful to direct effort improving the mis-
reporting of the number of arguments given versus those required for
instance methods:
>>> a.f(1, 2)
TypeError: f() takes exactly 1 argument (3 given) |
Raymond, do you have a cannon-less recommendation of how to kill this |
On Sun, Mar 30, 2008 at 5:01 PM, Raymond Hettinger
<report@bugs.python.org> wrote:
..
> It would be *much* more useful to direct effort improving the mis-
> reporting of the number of arguments given versus those required for
> instance methods:
> >>> a.f(1, 2)
> TypeError: f() takes exactly 1 argument (3 given) Please see bpo-2516. |
On Sun, Mar 30, 2008 at 5:01 PM, Raymond Hettinger
I agree with you, but only because fewer than 1% of Python programmers I don't know how big of a deal an extra buildbot is, but I don't think Mental overhead is important, but I think it will be easier to explain
As an alternative to the command line option, what would you say to
I still maintain that this is not a bug. Not hearing about it before |
Marking this one as closed. Also, rejecting the various ways to disable peephole optimization. Since then, I believe Neal got Guido's approval for either the -O or - |
It's hard for me to agree with your assessment that for no practical Compiled languages have long recognized the need for both types of As Python becomes more complex, and more broadly deployed, the needs of I see discussion here of moving the optimizer to the AST level instead As a developer of analysis tools, what should I tell my users when their |
While I agree with Raymond that the interpreter should be left alone, AS for continue.py, it seems that the apparent non-execution of a
Either way, it seems to me that the lack of runtime execution of |
Since the main argument for not fixing this bug seems to be that it doesn't affect many users, it seems like I should comment here that the issue is affecting me. A recently proposed addition to Twisted gets bitten by this case, resulting in a report of less than full test coverage when in fact the tests do exercise every line and branch of the change. Perhaps it is too hard to add and maintain a no-optimizations feature for Python (although I agree with Ned that this would be a useful feature for many reasons, not just to fix this bug). There are other possible solutions to the issue of inaccurate coverage reports though. For example, Python could provide an API for determining which lines have code that might be executed. coverage.py (and the stdlib trace.py) currently use the code object's lnotab to decide which lines might be executable. Maybe that should omit "continue" lines that get jumped over. If the line will never execute, it seems there is no need to have it in the lnotab. Using the lnotab is something of a hack though, so it might also make sense to leave it alone but introduce an API to get the same information, but corrected for whatever peephole optimizations the interpreter happens to have. As far as the "not a bug" arguments go, I don't think it matters much whether you ultimately decide to call it a bug or a feature request. It *is* clearly a useful feature to some people though, and rejecting the requested behavior as "not a bug" doesn't help anyone. So call it a feature request if that makes it more palletable. :) |
I think supporters of this feature request should take discussion to python-ideas to try to gather more support. The initial post should summarize reasons for the request, possible implementations, and the counter-arguments of Raymond. |
Choose pydev if you want. Discussion there is *usually* (but definitely not always) more focused on implementation of uncontroversial changes. I am pretty much +-0 on the issue, though Jean-Paul's post seems to add to the + side arguments that might be persuasive to others also. |
There has been no activity on this for several year. Marking as rejected for the reasons originally listed. |
Raymond, thanks for keeping us honest! I am still hoping to convince people that this is a good idea. I think Guido's +1 (https://mail.python.org/pipermail/python-dev/2012-December/123099.html) should help in that regard. Part of your reason for today's rejection is the lack of activity. Can I assume that with a patch you would be supportive? |
I consider peephole optimization when no optimization was requested a bug. Documentation for -O says it "Turns on basic optimizations". Peephole optimization is a basic optimization, yet it is performed even when no basic optimizations were requested. No need to add a switch. Just don't optimize if not requested. |
I believe the python-ideas thread on this topic came to the conclusion that a -X flag -- e.g., Does one of those proposal seems acceptable to everyone? Do people like Ned who asked for this feature have a preference as to whether the bytecode is or is not written out to a .pyc file? |
I would suggest -X noopt and use "noopt" in .pyc filenames. That's what I proposed in my PEP-511. |
Since the discussion restarted, I reopen the issue and assigned it to Python 3.6. Maybe it's too late for such tiny change? |
If anyone has needed a workaround in the past 9 years and hasn't yet found one: pyca/cryptography@3b585f8 |
This no longer works in 3.7 due to folding constants at the AST level. :-) |
Folding constants won't affect control flow. The important thing here is to disable optimizing away jumps to jumps. |
Few different optimizations work together here. Folding constants at the AST level allows to eliminate the constant expression statement in the code generation stage. This makes 'continue' a first statement in the 'if' body. Boolean expressions optimizations (performed in the code generation stage now) creates a conditional jump to the start of the 'if' body (which is 'continue' now). If 'continue' is not nested in 'try' or 'with' blocks, it is compiled to an unconditional jump. And finally the jump optimization in the peepholer retargets the conditional jump from the unconditional jump to the start of the loop. |
Serhiy: thanks for the fuller story about the optimizations in place now. I'll repeat my earlier plea: providing a way to disable optimization is beneficial for tooling like coverage.py, and also for helping us to ensure that the optimizer is working correctly. I doubt anyone commenting here would be happy with a C compiler that optimized with no off-switch. |
I'm another user of Ned's coverage tool. Our team at the Mount Sinai School of Medicine is building tools to model the dynamics of biochemistry inside individual cells. Our short term aims are to better understanding microbiology and model microorganisms so they can be engineered to more effectively produce drugs and do other industrial tasks. Long term, we seek to build genetically personalized models of human cells which can be used to improve the medical care of cancer and other illnesses. We're funded by several agencies of the federal government. Our field is called whole-cell modeling. We use Python because it provides a wide array of powerful tools we can reuse to reduce our development time, enables us to rapidly prototype software to test and advance our modeling ideas, and is fun to program. Using git, pip, coverage, GitHub, CircleCI, Docker and other tools we've built a robust development environment that enables multiple people to contribute to advancing our tools for whole-cell modeling. We strongly emphasize software engineering because the data we use is large, incomplete and inconsistent, and our models are complex and difficult to train, verify and validate. We want to have a high level of confidence in our tested code so that if we have trouble with a model we can focus on checking the data and understanding the model design. Coverage testing is an important part of our software engineering. We test both line and branch coverage. While working today on our simulator I found code that should have been fully covered except for a # pragma no cover, but was not fully covered. I reported it to Ned (nedbat/coveragepy#697) who reproduced it in a simpler example and pointed out that this "Add mechanism to disable optimizations" issue contributed to the problem. I realize that approximately 0.0% of Python users work on whole-cell modeling, which diminishes the importance of this use case. But Python is widely used in computational biomedicine, which represents many more users. Case in point -- I've created and teach a course in Biomedical Software Engineering which uses Python and teaches coverage testing to masters, PhD, and MD/PhD students. We'd appreciate your help improving Ned's coverage tool. You can learn more about us at http://www.karrlab.org/ and https://github.com/KarrLab. Regards |
Having properly working coverage tooling is simply invaluable to pretty much every serious Python user. I support Ned's idea of adding an option to disable peephole optimizer (and similar other optimization passes). Why is this even debated? |
Sounds good to me. |
This issue is well into the 2nd decade of debate. Who has the power to effect this change? Happy New Year |
If someone can get a PR into a state that is acceptable, then this can be resolved, Arthur. But at this point that hasn't occurred. |
FWIW, Yury started a pull request: #9693 |
My PR 13600 works as expected. I simplified attached continue.py: see attached traceme.py. Output with optimizations $ ./python traceme.py
6 0 LOAD_CONST 1 (0)
2 STORE_FAST 0 (a) 7 4 LOAD_GLOBAL 0 (range) 8 18 LOAD_FAST 1 (n) 9 26 LOAD_FAST 1 (n) 10 34 LOAD_FAST 0 (a) 11 42 JUMP_ABSOLUTE 14 Output without optimizations (-X noopt): $ ./python -X noopt traceme.py
6 0 LOAD_CONST 1 (0)
2 STORE_FAST 0 (a) 7 4 LOAD_GLOBAL 0 (range) 8 18 LOAD_FAST 1 (n) 9 26 LOAD_FAST 1 (n) 10 34 LOAD_FAST 0 (a) 11 >> 42 JUMP_ABSOLUTE 14 The difference on the trace is that using -X noopt, "traceme.py(11): continue" line is traced as expected. The difference on the bytecode is that jumps are no longer optimized using -X noopt:
The peephole optimizer replaces a jump to an unconditional jump with a jump directly to the target of the unconditional jump. I documented peephole optimizations in my reimplementation in pure Python: |
There are different optimizations on different levels (AST, bytecode generation, peepholer), would be nice to control them separately. This means that we should pass a bitset to the compiler. |
Appreciate you working on this Serhiy and Victor! |
What's the use case for enabling some AST optimizations but disable bytecode generation optimizations? |
I will pick this up from Victor's last patch |
If there is any doubt that this affects people, it was reported *again* against coverage.py today: nedbat/coveragepy#1025 |
I think this can finally be closed. PEP-626 specifies what the correct behavior is, regardless of whether optimizations are turned on or off, so there is no point in a no-optimize option. If the bytecode optimizer produces incorrect or inefficient code for a particular example, please file a bug report for that case, and assign me. |
In general, it is hard to define what is an optimization, and what is part of the compiler. The original request was to disable optimizations that changed observable behavior w.r.t. line numbers. All optimizations now respect line numbers, so proposed mechanism would be pointless. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: