[Python-Dev] Suggest reverting today's checkin (recursive constant folding in the peephole optimizer)

[Guido van Rossum]

I recall several occasions where the peephole optimizer was subtly
buggy -- on one occasion the bug remained undetected for at least a
whole release cycle. (Sorry, I can't recall the details.) In fact, the
bug reported in http://bugs.python.org/issue11244 is another example
of how subtle the peephole optimizer is. Adding more complexity to it
just increases the chances that it will be broken by seemingly
unrelated changes.

As far as Antoine's commit goes, it looks like it went in hastily,
without review, and before any discussion. Clearly it *is*
controversial, and controversial fixes always need to be held back
until at least rough consensus is reached or until a BDFL decision.
Also it is not a fix to the bug reported in the issue, so the
Misc/NEWS text is misleading. A much simpler fix was proposed in the
bug and even approved by Antoine. (One note on the patch: it allocates
an extra stack which is dynamically grown; but there is no unittest to
exercise the stack-growing code. This note does constitute a full
review.)

I have always felt uncomfortable with *any* kind of optimization --
whether AST-based or bytecode-based. I feel the cost in code
complexity is pretty high and in most cases the optimization is not
worth the effort. Also I don't see the point in optimizing expressions
like "3 * 4 * 5" in Python. In C, this type of thing occurs frequently
due to macro expansion and the like, but in Python your code usually
looks pretty silly if you write that. So this is basically a solution
to a non-problem. The only two places where I personally see a need
for compile-time optimization are unary minus (since there is no way
to write negative constants otherwise) and string literal
concatenation (which is a common way to break long string literals).

Now, should Antoine's checkin be rolled back? I think the case is in
Antoine's court to convince us that his patch is correct *and* to help
at least one or two others feel comfortable they understand how it
works -- if only Antoine understands it, it is too complex. If Antoine
does not succeed with this task he should roll it back. Or he can
choose to roll it back immediately, and then we can continue the
discussion under a lot less pressure, and come to a conclusion
amiably.

--Guido

On Fri, Mar 11, 2011 at 10:01 PM, Benjamin Peterson <benjamin at python.org> wrote:
> 2011/3/11 Raymond Hettinger <raymond.hettinger at gmail.com>:
>> Today, there was a significant check-in to the peephole optimizer that I
>> think should be reverted:
>>                http://hg.python.org/cpython/rev/14205d0fee45/
>> The peephole optimizer pre-dated the introduction of the abstract syntax
>> tree.  Now that we have an AST, the preferred way to implement additional
>> optimizations is with AST manipulation, upstream from code generation.  This
>> approach is faster and much more reliable than the more brittle approach
>> of disassembling, analyzing, and rewriting the bytecode created by the
>> compiler.
>
> The problem is no such AST optimizer exists. It's one thing avoid
> changing old code because an improved version is in the works or
> available (say argparse in lieu of getopt) and quite another when no
> replacement code exists. At the moment, there is little reason not to
> accept progressive improvements (with sights on overall design as
> usual) to the code.
>
> IMO, Ast or not ast statically optimizing python in any meaningful way
> is a impossible task anyway. So, a better solution would be to just
> rip the thing out.
>
>> There should no longer be any significant changes to the existing
>> optimizer.  It took a good deal of care to get it right in the first place.
>>  The code is stable and has been proven successful by many years of
>> deployment.  The nature of the peephole optimizer  is that changes to it are
>> high risk, that the procedure is brittle, and that it is easily mucked-up in
>> ways that are hard to detect. Accordingly, we've kept to simple conservative
>> transformations and have assiduously avoided more complex (and interesting)
>> optimizations.
>
> Can you point to examples where changes introduced long-term
> instability? ISTM, that the peepholer is one of the better tested
> parts of the interpreter if not directly from its own tests, from
> running the entire stdlib tests.
>
>> FWIW, I've been the maintainer (as well as the original designer and
>> implementer) of the peephole optimizer from the beginning; however, today's
>> committer did not accept my advice to be very conservative with changes to
>> it and to strongly prefer AST transformations instead.  Please consider
>> reverting this change.
>
>
>
> --
> Regards,
> Benjamin
> _______________________________________________
> Python-Dev mailing list
> Python-Dev at python.org
> http://mail.python.org/mailman/listinfo/python-dev
> Unsubscribe: http://mail.python.org/mailman/options/python-dev/guido%40python.org
>



-- 
--Guido van Rossum (python.org/~guido)