classification
Title: Patch: simplify complex constant assignment statements
Type: Stage:
Components: Versions:
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: georg.brandl, jhylton, novalis_dt, terry.reedy
Priority: normal Keywords: patch

Created on 2008-11-14 21:47 by novalis_dt, last changed 2010-08-27 20:41 by terry.reedy. This issue is now closed.

Files
File name Uploaded Description Edit
tlee-ast-optimize-multiassign.diff novalis_dt, 2008-11-14 21:47
Messages (10)
msg75889 - (view) Author: David Turner (novalis_dt) Date: 2008-11-14 21:47
This patch adds functionality to the optimizer to simplify complex
constant assignments like:

a, (b, c) = d, e = 1, (2, 3)

The simplification is:

a = 1
d = 1
b, c = e = 2, 3

Of course, the simplified version is semantically identical.  But the
bytecode generated is faster, because there is less need to unpack
tuples.  Naturally, this only works on constants:

a, b = 1, a is not the same as
a = 1
b = a
msg75890 - (view) Author: David Turner (novalis_dt) Date: 2008-11-14 21:50
Oh, and this also involved the creation of an additional statement type,
unfortunately. The statement type, Seq, represents a sequence of
statements.  This is so that we can replace a single assign with
multiple assigns.  If that's no good, then we could do an If with test=1
instead, since the compiler knows to optimize that out.  We could also
go into the parent statement and replace the body, but that would
require significant rejiggering of the optimizer, and significant
additional complexity.
msg76339 - (view) Author: Jeremy Hylton (jhylton) (Python triager) Date: 2008-11-24 16:53
It seems generally useful to have a helper function to replace a range
of nodes in a sequence of statements with another sequence of nodes.  A
general API like that would allow you to insert or delete nodes as well
as replacing one node with a set of nodes.  It seems like that could be
implemented pretty independently of this patch, and would then simplify
it.  I don't think it's a good idea to add the Seq type just to simplify
the implementation.
msg76341 - (view) Author: David Turner (novalis_dt) Date: 2008-11-24 17:14
jhylton, keep in mind that this would require an additional "parent"
argument to each function which takes a stmt.  Do you think this added
complexity is worth it?
msg76342 - (view) Author: Jeremy Hylton (jhylton) (Python triager) Date: 2008-11-24 17:16
On Mon, Nov 24, 2008 at 12:14 PM, David Turner <report@bugs.python.org> wrote:
>
> David Turner <novalis@novalis.org> added the comment:
>
> jhylton, keep in mind that this would require an additional "parent"
> argument to each function which takes a stmt.  Do you think this added
> complexity is worth it?

Or we could add parent pointers in the tree, right?

Jeremy

>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue4327>
> _______________________________________
> _______________________________________________
> Python-bugs-list mailing list
> Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/jeremy%40alum.mit.edu
>
>
msg76347 - (view) Author: David Turner (novalis_dt) Date: 2008-11-24 17:50
Sure, but that's an even bigger change.  Every piece of code which
modifies the AST would now also have to modify parent pointers.  Having
the pointers would make many things easier, but I didn't want to make a
very invasive change like that without thinking it through thoroughly.
msg76348 - (view) Author: Jeremy Hylton (jhylton) (Python triager) Date: 2008-11-24 17:55
I haven't thought about the code in a while, but what code that
modifies the AST are we worried about?  There are lots of
modifications in ast.c, since it is being created there.  The case we
really care about is sequences, where we want to modify the sequence.
The creation goes through macros like asdl_seq_SET(), so we could just
change the macro.  What are other cases we need to worry about?

Jeremy

On Mon, Nov 24, 2008 at 12:50 PM, David Turner <report@bugs.python.org> wrote:
>
> David Turner <novalis@novalis.org> added the comment:
>
> Sure, but that's an even bigger change.  Every piece of code which
> modifies the AST would now also have to modify parent pointers.  Having
> the pointers would make many things easier, but I didn't want to make a
> very invasive change like that without thinking it through thoroughly.
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue4327>
> _______________________________________
>
msg76354 - (view) Author: David Turner (novalis_dt) Date: 2008-11-24 18:19
Everything in optimize.c is about modifying the AST  (this is on tlee's
ast optimization branch, in case I didn't mention earlier).  Also,
changing asdl_seq_SET would be a bad idea, since it is used for
sequences of things other than stmt_tys.

What if asdl_seq were instead a doubly-linked list?  Then each node
contains enough information to add and remove elements.  asdl_seq_LEN
would be slower, but it is rarely used.  To be really slick, we could
store the len of the seq in the first element's prev pointer as (len <<
1) | 1.
msg114615 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-08-21 23:59
I'm going to reject this; it adds a whole lot of code for optimizing very a minor usecase.
msg115131 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2010-08-27 20:41
I agree. A programmer who mashes
a = d = 1
b, c = e = 2, 3
into one statement deserves the bytecode he gets ;-).
History
Date User Action Args
2010-08-27 20:41:24terry.reedysetnosy: + terry.reedy
messages: + msg115131
2010-08-21 23:59:04georg.brandlsetstatus: open -> closed

nosy: + georg.brandl
messages: + msg114615

resolution: rejected
2008-11-24 18:19:02novalis_dtsetmessages: + msg76354
2008-11-24 17:55:22jhyltonsetmessages: + msg76348
2008-11-24 17:50:02novalis_dtsetmessages: + msg76347
2008-11-24 17:16:27jhyltonsetmessages: + msg76342
2008-11-24 17:14:01novalis_dtsetmessages: + msg76341
2008-11-24 16:53:39jhyltonsetnosy: + jhylton
messages: + msg76339
2008-11-14 21:50:41novalis_dtsetmessages: + msg75890
2008-11-14 21:47:38novalis_dtcreate