Don't do a second lookup as we have it already.

Don't do a second lookup as we have it already.


Committed: http://code.google.com/p/v8/source/detail?r=2306

[Mads Ager (chromium), 2009-06-25 16:52:55]

LGTM

[Lasse Reichstein, 2009-06-26 06:24:29]

http://codereview.chromium.org/149007/diff/1/2
File src/ic.cc (right):

http://codereview.chromium.org/149007/diff/1/2#newcode333
Line 333: result = object->GetProperty(*object, &lookup, *name, &attr);
You still do object->lookup(...) above, so you don't really save anything by
moving this out of the conditional branches. Could you move the GetProperty call
up and replace the LookUp? (Not a rethorical question, I'm not sure what the
lookup call does exactly).
Further, this call seems more complex than the ones in the branches (passing
&lookup and &attr suggests that some assignment will happen that wouldn't in
both branches).

[antonm, 2009-06-26 19:05:35]

Lasse,

thanks a lot for review!

Somehow I didn't get any email from you and only by pure chance noticed your
comments via codereview.chromium.org.  Is it normal?  Am I misseted up (awful
English)

http://codereview.chromium.org/149007/diff/1/2
File src/ic.cc (right):

http://codereview.chromium.org/149007/diff/1/2#newcode333
Line 333: result = object->GetProperty(*object, &lookup, *name, &attr);
On 2009/06/26 06:24:29, Lasse Reichstein wrote:
> You still do object->lookup(...) above, so you don't really save anything by
> moving this out of the conditional branches. Could you move the GetProperty
call
> up and replace the LookUp? (Not a rethorical question, I'm not sure what the
> lookup call does exactly).
> Further, this call seems more complex than the ones in the branches (passing
> &lookup and &attr suggests that some assignment will happen that wouldn't in
> both branches).

Lasse, please, double check that I understand it correctly.  The main point of
this CL is to reuse LookupResult already filled up by Lookup method above.  Now
GetProperty accepts LookupResult and doesn't invoke Lookup anymore (compare to
previous state of affairs: GetProperty(String*) would invoke
GetProperty(String*, PropertyAttributes*) which would invoke
GetPropertyWithReceiver which would do lookup and invoke GetProperty(JSObject*,
LookupResult*, String*, PropertyAttributes*).  I'm trying to get rid of those
intermediate calls and esp. ::Lookup.  Do I miss something?