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

[Guido van Rossum]

On Sat, Mar 12, 2011 at 11:05 AM, Raymond Hettinger
<raymond.hettinger at gmail.com> wrote:
> There are separate social, strategic, and tactical questions.
>
> The social question:  if the person who designed, implemented, and maintained the optimizer recommends against a patch and another committer just checks it in anyway, do we care?  I've taken responsibility for this code and have treated it will a good deal of care.  I don't believe the patch should go in because the risk/reward ratio is too low and because a better solution exists.

Hm. A few comments:

- you're dangerously close here to putting your ego ahead of things
- further review of the patch may give a better understanding of the risk
- a "better solution" that doesn't exist yet shouldn't be too strong
an argument against "good enough"

> The strategic question:  constant folding in the peephole optimizer pre-dates the AST branch.  When AST landed, there was general agreement among myself and those involved that any future optimizations which could be done with the AST, should be done there.  It is the correct technological solution.
>
> At one point, we had strategic agreement to stop building-out the peepholer in any significant way.   In fairness to Antoine, the conversations took place several years before he became a committer.  The strategic question is do we want to come to that agreement again?  Can we agree to not have significant changes to the peepholer, to treat it with great care and restraint?   This patch doesn't have to go in.  Py3.3 won't be out for 18 to 24 months anyway.  There is a lot of time to do the job right and not add weight to a house of cards.

Feel free to go ahead and start it. Write a PEP, or just write a patch
and get people to review and understand it. If it's successful, we can
delete peephole.c and the effect of Antoine's checkin will be
nullified. If it's not successful, Antoine's code has the advantage of
possibly being good enough, and will have had those same 18-24 months
to have its tires kicked a bit.

> The tactical question:  is this particular patch correct?  Maybe it is.  I don't know, I didn't get past all the new macros.  I do know that I could have whipped up something similar years ago but chose not to based on advice from Thomas, Andrew, Tim, and Guido.   The only change since then is that there is a newer committer doesn't buy into that reasoning.

A recurring problem I hear about is that there is too much resistance
to new ideas coming from new developers. Clearly new developers may
not be aware of where the skeletons are hiding. But just as clearly
the old guard isn't always right. Antoine is by now not a newbie any
more. So please consider Antoine's patch on its own merit and not just
from the perspective of a decision made years ago.

> Constant folding is the most complex part of the optimizer.  Why would we add to it, when a better and more reliable approach exists?  The patch hasn't even been demonstrated to be necessary in any real-world code.

peephole.c is now 773 lines; it was 700 lines, so it grew by about
10%. But that's not very large: ceval.c is 4500 lines (many of which
are much deeper magic which I personally don't feel I understand at
all :-). I am not at all sure that you can create an AST-based
optimizer in less than 700 lines. (Nor am I sure that you can't -- nor
am I sure it matters. I'm, just saying. :-)

All in all I think it would be more productive to try and understand
Antoine's patch than to try to get it revert based on "because I [or
Tim Peters] said so" arguments.

Finally: There appears to be some disagreement on who said what, in
particular Raymond claims that he told Antoine not to commit whereas
Antoine claims he did not hear this feedback. I'm guessing it happened
in IRC (#python-dev), which is intentionally not logged anywhere. This
is an illustration of what is wrong with using IRC for important
decisions. I would have been much happier if the discussion about the
desirability of the patch could have happened *before* it was
committed, and I think maybe we need to have a stricter rule requiring
code reviews and possibly cool-off periods.

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