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

[Antoine Pitrou]

Hello,

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

Well, the regression stems from a commit in the ast code, not in the
peepholer code (and it's only a regression as in "misses an
optimization opportunity", not "produces bogus code").

The peepholer is actually not very subtle, and that's why it missed the
obvious optimization here.

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

I didn't get any comments against the patch before committing it, and
I trusted Eugene's comment that the second patch was fine (Eugene
proposed other, decent patches for the peepholer and thus seems
acquainted with the code). Also, as Eugene pointed out, running the
whole test suite in itself stresses the peepholer quite a bit if you
take care of removing pyc files beforehand (which the buildbots do).

Any further review is obviously welcome, as usual.

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

Well, the "much simpler fix" fixes a separate issue with -0; it is
orthogonal to the original issue, and doesn't fix it (Eugene should
probably have filed it it in a separate issue).

The patch I committed, OTOH, *is* a fix to the original issue (there's
even a test for that ;).
The fact that the fix implies a more general improvement to the
peepholer might be seen as an annoyance, or a benefit, depending on
your point of view (although I'm not sure why a general improvement
would be an annoyance).

> (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.)

As Eugene answered, there is actually an unittest for that.

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

But then, as others pointed out, why don't we rip out the peepholer?
If it's fragile and doesn't serve a purpose, it doesn't deserve staying.

There is, by the way, recent history for adding optimizations to the
peepholer, which was even approved by Raymond:
http://bugs.python.org/issue6690
... so I'm not sure why there is opposition to fixing a regression,
when apparently new optimizations are uncontroversial.

> 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 original issue was about constant-folding of tuples containing
negative numbers, which doesn't sound like an exotic situation. The
obvious solution (enabling propagation of constant-folding) also
entails constant-folding of arbitrarily complex expressions, which is
why I have added tests for it, and suitably described it in the
Misc/NEWS. Having more tests is always worthwhile IMO (even if we later
choose to change them for whatever reason).

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

I really don't think the patch is complicated. It adds a stack to
manage the current run of constants. This replaces the previous code
where that run of constants was implicitly managed through the "lastlc"
and "cumlc" variables and hand-computed indices into the bytecode; the
new approach seems less clumsy to me.

(Eugene apparently understood the patch, so hopefully I'm not alone :-))

Now, if a majority is in favour of rolling it back ("backout" in
hg terminology), let's do it. That regression obviously isn't
killing anyone. Should we ask for a vote?

Regards

Antoine.