Avoid one repeated property lookup when computing store ICs.

Avoid one repeated property lookup when computing store ICs.

[Toon Verwaest, 2014-08-21 15:07:06]

https://codereview.chromium.org/494153002/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/494153002/diff/1/src/ic.cc#newcode1288
src/ic.cc:1288: }
You already found a property, so you need to make a decision. I presume you want
to return true?
So basically:
return it->property_encoding() == LookupIterator::DESCRIPTOR?

https://codereview.chromium.org/494153002/diff/1/src/ic.cc#newcode1348
src/ic.cc:1348: // LookupResult lookup(isolate());
Remove leftover

https://codereview.chromium.org/494153002/diff/1/src/ic.cc#newcode1351
src/ic.cc:1351: if (!can_store && strict_mode() == STRICT &&
object->IsGlobalObject() &&
This shouldn't depend on "can_store". Before the reference error you could
DCHECK(!can_store) though.

https://codereview.chromium.org/494153002/diff/1/src/ic.cc#newcode1352
src/ic.cc:1352: !(lookup.state() == LookupIterator::PROPERTY &&
lookup.has_property() &&
You shouldn't need to expose has_property(). Any iteration that doesn't
has_property after visiting PROPERTY should have called Next(); continuing to
NOT_FOUND.

https://codereview.chromium.org/494153002/diff/1/src/ic.cc#newcode1426
src/ic.cc:1426: DCHECK(lookup->state() != LookupIterator::JSPROXY);
DCHECK_NE

https://codereview.chromium.org/494153002/diff/1/src/lookup.h
File src/lookup.h (right):

https://codereview.chromium.org/494153002/diff/1/src/lookup.h#newcode118
src/lookup.h:118: Handle<JSObject> GetPropertyTarget() const;
GetStoreTarget() ?

https://codereview.chromium.org/494153002/diff/1/src/lookup.h#newcode142
src/lookup.h:142: bool has_property() const { return has_property_; }
This shouldn't be necessary

[Jakob Kummerow, 2014-08-21 16:14:53]

Thanks for the review; handing over.

https://codereview.chromium.org/494153002/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/494153002/diff/1/src/ic.cc#newcode1288
src/ic.cc:1288: }
On 2014/08/21 15:07:05, Toon Verwaest wrote:
> You already found a property, so you need to make a decision. I presume you
want
> to return true?
> So basically:
> return it->property_encoding() == LookupIterator::DESCRIPTOR?

Done.

https://codereview.chromium.org/494153002/diff/1/src/ic.cc#newcode1348
src/ic.cc:1348: // LookupResult lookup(isolate());
On 2014/08/21 15:07:05, Toon Verwaest wrote:
> Remove leftover

Done.

https://codereview.chromium.org/494153002/diff/1/src/ic.cc#newcode1351
src/ic.cc:1351: if (!can_store && strict_mode() == STRICT &&
object->IsGlobalObject() &&
On 2014/08/21 15:07:05, Toon Verwaest wrote:
> This shouldn't depend on "can_store". Before the reference error you could
> DCHECK(!can_store) though.

It does, actually, because we're interested in nonexistent properties. Removing
the condition would mean that we erroneously throw when trying to overwrite
writable global properties.

https://codereview.chromium.org/494153002/diff/1/src/ic.cc#newcode1352
src/ic.cc:1352: !(lookup.state() == LookupIterator::PROPERTY &&
lookup.has_property() &&
On 2014/08/21 15:07:06, Toon Verwaest wrote:
> You shouldn't need to expose has_property(). Any iteration that doesn't
> has_property after visiting PROPERTY should have called Next(); continuing to
> NOT_FOUND.

Done.

https://codereview.chromium.org/494153002/diff/1/src/ic.cc#newcode1426
src/ic.cc:1426: DCHECK(lookup->state() != LookupIterator::JSPROXY);
On 2014/08/21 15:07:06, Toon Verwaest wrote:
> DCHECK_NE

Done.

https://codereview.chromium.org/494153002/diff/1/src/lookup.h
File src/lookup.h (right):

https://codereview.chromium.org/494153002/diff/1/src/lookup.h#newcode118
src/lookup.h:118: Handle<JSObject> GetPropertyTarget() const;
On 2014/08/21 15:07:06, Toon Verwaest wrote:
> GetStoreTarget() ?

Done.

https://codereview.chromium.org/494153002/diff/1/src/lookup.h#newcode142
src/lookup.h:142: bool has_property() const { return has_property_; }
On 2014/08/21 15:07:06, Toon Verwaest wrote:
> This shouldn't be necessary

Done.

[Toon Verwaest, 2014-08-22 10:53:43]

Took over under https://codereview.chromium.org/478043006/