[Patches] [ python-Patches-664320 ] Replace pop/push pairs in ceval.c

[SourceForge.net]

Patches item #664320, was opened at 2003-01-08 06:34
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=664320&group_id=5470

Category: Core (C code)
Group: Python 2.3
>Status: Closed
Resolution: None
Priority: 5
Submitted By: Raymond Hettinger (rhettinger)
Assigned to: Nobody/Anonymous (nobody)
Summary: Replace pop/push pairs in ceval.c

Initial Comment:
Paired PUSH() and POP() macros unnecessarily 
decrement and then re-increment the stack pointer.  
In these cases, indexed access to and from the stack 
is faster.

Saves the add 4 and sub 4 in the assembler code. 
Allows the compiler more freedom to optimize (by 
not overspecifying or creating sequential 
interdependencies). Lets the processor run more of 
the instructions in parallel (adjusting the index 
prevents parallel execution any move or other 
instruction that depends on the index).

I wouldn't begin to know how to time the savings 
here, but examining the  generated assembler shows 
that the code has improved.

----------------------------------------------------------------------

>Comment By: Raymond Hettinger (rhettinger)
Date: 2003-01-09 10:28

Message:
Logged In: YES 
user_id=80475

Added a final POP() to GET_ITER and UNARY_NOT to make 
sure v comes off the stack upon an error exit.

Added STACKADJ to the LLTRACE logic.  This maintains its 
current definition of triggering a trace event whenever the 
stacklevel changes.  The new patch differs from the old 
only it that it makes fewer level changes.

Thank you for the thorough review. 

Committing as ceval.c 2.343
Marking as closed.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2003-01-08 23:59

Message:
Logged In: YES 
user_id=33168

Everything looks ok, except I'm not sure about the hunk in
GET_ITER.  It looks like v is left on the stack if x ==
NULL.  This is a change from the current version.  Taking a
look again, this seems to affect UNARY_NOT also.

I think LLTRACE is old.  Here's some mail where mwh talks
about it when he got rid of SET_LINENO. 
http://mail.python.org/pipermail/python-dev/2002-August/027219.html

I agree that you shouldn't change DUP_TOPX in this patch. 
It's a separate issue.  I think it might be good to remove
separately, to reduce the size of code in the eval_frame
loop.  But it's just thinking.

----------------------------------------------------------------------

Comment By: Raymond Hettinger (rhettinger)
Date: 2003-01-08 23:29

Message:
Logged In: YES 
user_id=80475

If you don't mind a little more code review, I've attached an 
extended version of the patch that does the same thing for 
other op codes (only where it could be done cleanly).  The 
goal was to use direct access as much as possible and 
have at most one adjustment to the stacklevel for each op 
code.

LLTRACE looks like a relic from 1992.  Will contact  mwh 
to ask whether it can completely eliminated.

Will leave DUP_TOPX able to handle x=4 or x=5.  It's 
harmless enough and I have no way of being sure that 
something somewhere hasn't generated bytecode that 
relies on it.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2003-01-08 22:54

Message:
Logged In: YES 
user_id=33168

One thing to note, if LLTRACE is defined, you lose that
capability since PUSH/POP are not called that frequently.  I
don't know how useful LLTRACE is and I don't care about this
"loss."  mwh may have used it recently, so it may be good to
get his opinion.

I reviewed patch in detail and didn't find any problems.  I
ran the regression suite on Linux without problems.  I also
ran pychecker over the stdlib, that sometimes provokes weird
bugs, no problems there either. :-)

If anything, I like the code better after this patch. 
Definitely +1.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2003-01-08 22:33

Message:
Logged In: YES 
user_id=33168

I'm reviewing this patch now and have a question.  There is
code to handle DUP_TOPX for values of X == 4 and 5.  I don't
see how this can be generated.  I greped the code for
DUP_TOPX and only found values of 2 and 3.  Should the code
for 4 and 5 be removed?  Am I missing something?

----------------------------------------------------------------------

Comment By: Tim Peters (tim_one)
Date: 2003-01-08 13:37

Message:
Logged In: YES 
user_id=31435

I haven't reviewed this in detail (and won't), but +1 on the 
idea -- these kinds of small code improvements may or may 
not yield immediate speedups, but reliably move things in 
the right direction over time, as they compound.  The parts 
of the patch I did stare at didn't suffer in clarity, so +1 all 
around.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=664320&group_id=5470