classification
Title: LOAD_CONST followed by LOAD_ATTR can be optimized to just be a LOAD_CONST
Type: performance Stage: resolved
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: BreamoreBoy, alex, benjamin.peterson, collinwinter, loewis, pitrou, rhettinger, terry.reedy
Priority: low Keywords: patch

Created on 2009-05-27 23:19 by alex, last changed 2014-06-20 01:34 by terry.reedy. This issue is now closed.

Files
File name Uploaded Description Edit
python_const_fold.diff alex, 2009-05-28 03:07
python_const_fold.diff alex, 2009-05-28 03:17
python_const_fold.diff alex, 2009-05-28 03:23
python_const_fold.diff alex, 2009-05-28 04:23
python_const_fold.diff alex, 2009-05-28 20:02
python_const_fold.diff alex, 2009-05-28 22:35
constattr.diff rhettinger, 2009-05-29 20:54 Rough patch for LOAD_ATTR_CONST
Messages (22)
msg88455 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2009-05-27 23:19
Basically whenever you have a LOAD_CONST opcode, follwed by a LOAD_ATTR
you can replace both with a single LOAD_CONST.  This optimizes things
like ", ".join or "{} {}".format (in my totally unscientific byte code
hackery it's about a 30% speedup on the loading the function).  This can
be done in the peephole optimizer.

I'll try to work up a patch.
msg88460 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2009-05-28 03:07
Here's my work so far, it seems to work as described.  Except for the
fact that pyc creation with anything containign something with this
optimization will give a valueerror on bad marshall data, from trying to
marshall the method I assume.
msg88461 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2009-05-28 03:17
Small update so I don't change whitespace all over the plcae.
msg88462 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2009-05-28 03:23
Switch to using memset instead of a forloop.
msg88463 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2009-05-28 04:23
I now *almost* have PyCFunctions marshalling, they seem to marhshall ok
but fail on unmarshalling.  I think the whitespace stuff may have crept
back in, sorry :(
msg88474 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2009-05-28 20:02
Optimization now works in the shell fine, and
marshal.loads(marshal.dumps(''.join)) works fine in the shell.  However
when I try to run the tests the import of collections.namedtuple causes
the ValueError bad marshal data to appear, and I don't know why.  Could
it have to do with the fact that namedtuple dynamically creates classes
(though I don't see how one of those could ever be the subject of
LOAD_CONST).
msg88483 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-05-28 21:51
-1. const.attr is not necessarily a constant. Indeed, "".join is *not* a
constant.

Furthermore, using this approach will lead to an endless series of types
to be added to marshalling, which is bad. For example, I believe that
current patch breaks on "".__class__
msg88484 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-05-28 22:13
I would like to see the current patch finished and recorded here for
reference.  But I agree with Martin that changing marshal is a can of
worms.  The peepholer is intended to be conservative and should avoid
anything that has some risk of being complicating our lives.

Another approach may be more successful.  Here's a rough sketch:

in:  LOAD_CONST 5 '{}'   LOAD_ATTR 3 'format' 
out: LOAD_CONST 8 ('{}','format')   LOAD_CONST_ATTR 7.

def LOAD_CONST_ATTR(oparg):
   # cache the lookup of '{}.format' in a fast local
   x = GETLOCAL(oparg)
   if (x == NULL)
       UNPACK 2 --> obj, name
       bm = getattr(obj, name)
       STORE_FAST var
    LOAD_FAST var
msg88486 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2009-05-28 22:35
Fully functional.
msg88530 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2009-05-29 20:24
Martin, I agree that we would have to think carefully about all
attributes of all constants loaded by LOAD_CONST, and about
special-casing marshal, but given that "'str' object attribute 'join' is
read-only", how is ''.join not a constant?  Do you have another example
in mind?
msg88531 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-05-29 20:28
I'm working a better patch now.  Will give to collin to review when it's
ready.  Here's a draft of the new opcode:

		TARGET(LOAD_CONST_ATTR)
			u = GETLOCAL(oparg);	/* Cached attribute or NULL */
			t = TOP();				/* t = (obj, name) where obj is constant */
			if (u != NULL) {		/* If cache is non-null, use it */
				Py_INCREF(u);
				SET_TOP(u);
				Py_DECREF(t);
				FAST_DISPATCH();
			}
			/* Cache is empty, do regular attribute lookup */
			assert(PyTuple_CheckExact(t) && Py_SIZE(t) == 2);
			v = PyTuple_GET_ITEM(t, 0);
			w = PyTuple_GET_ITEM(t, 1);
			x = PyObject_GetAttr(v, w);
			Py_DECREF(t);
			if (x != NULL) {		/* Successful lookup; cache it and return */
				SETLOCAL(oparg, x);
				Py_INCREF(x);
				SET_TOP(x);
				break;
			}
			STACKADJ(-1);			/* Attribute not found; goto err handler */
			break;
msg88538 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-05-29 20:54
Attaching a rough patch for caching constant attribute lookups.   Has an
open TODO for adding the new name to co->co_varnames.
msg88546 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-05-29 22:45
Terry J. Reedy wrote:
> Terry J. Reedy <tjreedy@udel.edu> added the comment:
> 
> Martin, I agree that we would have to think carefully about all
> attributes of all constants loaded by LOAD_CONST, and about
> special-casing marshal, but given that "'str' object attribute 'join' is
> read-only", how is ''.join not a constant?  

py> s = ""
py> s.join is s.join
False

Every time you read it, you get a new object. Not what I would call a
constant. If you don't see how this matters, try

def foo():
  return "".join

print foo() is foo()

with and without your patch.
msg88552 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2009-05-29 23:22
You are right, of course: bound methods are currently created fresh on
each access, even though each is equal except for identity.  I was
thinking in terms of 
>>> str.join is str.join
True

This appears to be a classic time-space tradeoff: caching bound methods,
which is what I understand Raymond's patch does (when the attribute is a
method), saves time (with enough reuses) but takes space that could
gradually grow.

The reply to the OP could be that people who care about time should use
the standard idiom of explicitly  savingthe bound method:
>>> bjoin = ''.join
...
On the other hand, such bindings look a bit messy and tend to be local.
msg88556 - (view) Author: Collin Winter (collinwinter) * (Python committer) Date: 2009-05-30 00:04
On Fri, May 29, 2009 at 3:45 PM, Martin v. Löwis <report@bugs.python.org> wrote:
> py> s = ""
> py> s.join is s.join
> False
>
> Every time you read it, you get a new object. Not what I would call a
> constant. If you don't see how this matters, try
>
> def foo():
>  return "".join
>
> print foo() is foo()
>
> with and without your patch.

The fact that it returns a new object every time seems like an
implementation detail, and one that users find confusing (I know I
once filed a bug about it).

One problem I recognize is that the proposed patch would presumably
create an asymmetry between "x".join is "x".join and "x = 'x'; x.join
is x.join" where the former would evaluate to True and the latter to
False. That seems surmountable, though.
msg88558 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-30 00:09
Does this optimization actually help in real-world cases?
msg88561 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-05-30 03:09
Returning the same object vs new object for bound methods is a
non-guaranteed implementation detail (as long the other semantics remain
true).   I think Martin's real concern is that trying to intern bound
methods would be a can of worms (one that I wouldn't want to open).

FWIW, str.join ignored by this patch.  The "str" builtin is an
overridable builtin (LOAD_GLOBAL "str" LOAD_ATTR "join").  In contrast,
",".join or "{}".format does not have dynamically changeable behavior
(LOAD_CONST "{}" LOAD_ATTR "format").
msg88562 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-05-30 03:14
[AP]
> Does this optimization actually help in real-world cases?

Yes and no.  Yes, there are real world cases like ','.join and
'{}'.format that are dramatically sped-up.  No, there are probably no
real-world programs that are sped-up significantly in their entirety --
no one optimization will ever do that (Amdahl's law).  Pretty much
anytime the substitution gets made there is a savings on second and
subsequent calls (just like any form of caching).
msg88563 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-05-30 05:17
> Returning the same object vs new object for bound methods is a
> non-guaranteed implementation detail (as long the other semantics remain
> true).   I think Martin's real concern is that trying to intern bound
> methods would be a can of worms (one that I wouldn't want to open).

I'm also concerned about the change in behavior. Whether or not it is
guaranteed - some code in the world *will* notice. IMO, optimizations
should only be implemented if there is no change in observable behavior,
or if the improvement is so large that breaking compatibility is
justified.
msg115349 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-09-02 00:43
This proposal hasn't gotten much love or support and I'm no longer interested in it.  Aside from ''.join and '{}'.format, there doesn't seem to be many common cases, so there's no big win here.
msg115350 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2010-09-02 01:25
What's the point of '{}'.format() anyway given the format() builtin?
msg221022 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-19 21:15
Given the last two comments can this be closed as won't fix?
History
Date User Action Args
2014-06-20 01:34:01terry.reedysetstatus: open -> closed
resolution: later -> rejected
stage: resolved
2014-06-19 21:15:34BreamoreBoysetnosy: + BreamoreBoy
messages: + msg221022
2010-09-02 01:25:18benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg115350
2010-09-02 00:43:52rhettingersetassignee: rhettinger ->
resolution: later
messages: + msg115349
versions: + Python 3.3, - Python 3.2
2010-04-03 03:20:48rhettingersetpriority: low
versions: - Python 2.7
2009-08-21 16:00:18serprexsettitle: LOAD_CONST followed by LOAD_ATTR can be optimized to just be a LOAD_COST -> LOAD_CONST followed by LOAD_ATTR can be optimized to just be a LOAD_CONST
2009-05-30 05:18:10loewissetmessages: + msg88563
title: LOAD_CONST followed by LOAD_ATTR can be optimized to just be a LOAD_COST -> LOAD_CONST followed by LOAD_ATTR can be optimized to just be a LOAD_COST
2009-05-30 03:14:47rhettingersetmessages: + msg88562
2009-05-30 03:09:47rhettingersetmessages: + msg88561
2009-05-30 00:09:38pitrousetnosy: + pitrou
messages: + msg88558
2009-05-30 00:04:41collinwintersetmessages: + msg88556
title: LOAD_CONST followed by LOAD_ATTR can be optimized to just be a LOAD_COST -> LOAD_CONST followed by LOAD_ATTR can be optimized to just be a LOAD_COST
2009-05-29 23:22:19terry.reedysetmessages: + msg88552
2009-05-29 22:45:37loewissetmessages: + msg88546
title: LOAD_CONST followed by LOAD_ATTR can be optimized to just be a LOAD_COST -> LOAD_CONST followed by LOAD_ATTR can be optimized to just be a LOAD_COST
2009-05-29 20:54:46rhettingersetfiles: + constattr.diff

messages: + msg88538
2009-05-29 20:29:00rhettingersetmessages: + msg88531
2009-05-29 20:24:59terry.reedysetnosy: + terry.reedy
messages: + msg88530
2009-05-29 16:27:47collinwintersetnosy: + collinwinter
2009-05-28 22:35:51alexsetfiles: + python_const_fold.diff

messages: + msg88486
2009-05-28 22:13:26rhettingersetversions: + Python 2.7, Python 3.2
nosy: + rhettinger

messages: + msg88484

assignee: rhettinger
type: performance
2009-05-28 21:51:26loewissetnosy: + loewis
messages: + msg88483
2009-05-28 20:02:27alexsetfiles: + python_const_fold.diff

messages: + msg88474
2009-05-28 04:23:17alexsetfiles: + python_const_fold.diff

messages: + msg88463
2009-05-28 03:23:26alexsetfiles: + python_const_fold.diff

messages: + msg88462
2009-05-28 03:17:30alexsetfiles: + python_const_fold.diff

messages: + msg88461
2009-05-28 03:07:40alexsetfiles: + python_const_fold.diff
keywords: + patch
messages: + msg88460
2009-05-27 23:19:50alexcreate