Avoid one repeated property lookup when computing load ICs.

Avoid one repeated property lookup when computing load ICs.

R=verwaest@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=22802

[Jakob Kummerow, 2014-07-30 12:56:06]

PTAL.

Coming up next: same trick for stores. That will eliminate the temporary
duplication introduced in this CL.

[Toon Verwaest, 2014-07-30 16:54:53]

Overall looks good, just some comments.

https://codereview.chromium.org/429053005/diff/1/src/field-index.cc
File src/field-index.cc (right):

https://codereview.chromium.org/429053005/diff/1/src/field-index.cc#newcode24
src/field-index.cc:24: FieldIndex FieldIndex::ForLookupIterator(const
LookupIterator* lookup) {
Let the LookupIterator calculate the FieldIndex instead, perhaps by calling a
lower-level interface on FieldIndex.

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

https://codereview.chromium.org/429053005/diff/1/src/ic.cc#newcode208
src/ic.cc:208: if (!holder->GetNamedInterceptor()->getter()->IsUndefined()) {
In the future we should probably get the property from the interceptor to see if
it's there. If not, we should continue the lookup behind the interceptor and
compile this into the IC as well.

Since this matches what we did before, it's fine for now.

https://codereview.chromium.org/429053005/diff/1/src/ic.cc#newcode855
src/ic.cc:855: bool receiver_is_holder = lookup->HolderIsReceiver();
These 2 things are different.
Rename HolderIsReceiver to HolderIsReceiverOrHiddenPrototype() along the way to
make it explicit?

https://codereview.chromium.org/429053005/diff/1/src/ic.cc#newcode862
src/ic.cc:862: lookup->has_fast_properties() ? Code::FAST : Code::NORMAL);
What about just doing lookup->holder_map()->is_dictionary_map() and removing
this method again from the LookupIterator?

The difference between Code::FAST and Code::NORMAL should go away any time soon
now anyway ;)

https://codereview.chromium.org/429053005/diff/1/src/ic.cc#newcode967
src/ic.cc:967: Handle<JSObject> holder(lookup->GetHolder<JSObject>());
Handle<JSObject> holder = lookup->GetHolder<JSObject>();

https://codereview.chromium.org/429053005/diff/1/src/ic.cc#newcode977
src/ic.cc:977: switch (lookup->property_details().type()) {
What about doing the same as GetProperty; using property_kind() to switch
between callbacks and others, then using property_encoding for dict vs
field/constant; and only at the end using property_details?

I'd like to get rid of the current form of PropertyType at some point.

https://codereview.chromium.org/429053005/diff/1/src/ic.cc#newcode997
src/ic.cc:997: type, global, cell, name, !lookup->IsConfigurable());
What about changing the interface of CompileLoadGlobal? Otherwise you negate the
flag 3 times :)
!is_dont_delete -> !configurable -> !is_dont_delete

https://codereview.chromium.org/429053005/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/429053005/diff/1/src/objects.cc#newcode15169
src/objects.cc:15169: PropertyCell*
GlobalObject::GetPropertyCell(LookupIterator* lookup) {
Let the LookupIterator return the cell instead.

[Jakob Kummerow, 2014-07-31 13:55:51]

Comments addressed, PTAL.

https://codereview.chromium.org/429053005/diff/1/src/field-index.cc
File src/field-index.cc (right):

https://codereview.chromium.org/429053005/diff/1/src/field-index.cc#newcode24
src/field-index.cc:24: FieldIndex FieldIndex::ForLookupIterator(const
LookupIterator* lookup) {
On 2014/07/30 16:54:52, Toon Verwaest wrote:
> Let the LookupIterator calculate the FieldIndex instead, perhaps by calling a
> lower-level interface on FieldIndex.

Done.

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

https://codereview.chromium.org/429053005/diff/1/src/ic.cc#newcode208
src/ic.cc:208: if (!holder->GetNamedInterceptor()->getter()->IsUndefined()) {
On 2014/07/30 16:54:52, Toon Verwaest wrote:
> In the future we should probably get the property from the interceptor to see
if
> it's there. If not, we should continue the lookup behind the interceptor and
> compile this into the IC as well.

But that assumes that the interceptor never changes its behavior, right? I'm not
sure we should make that assumption.

> Since this matches what we did before, it's fine for now.

Acknowledged.

https://codereview.chromium.org/429053005/diff/1/src/ic.cc#newcode855
src/ic.cc:855: bool receiver_is_holder = lookup->HolderIsReceiver();
On 2014/07/30 16:54:53, Toon Verwaest wrote:
> These 2 things are different.
> Rename HolderIsReceiver to HolderIsReceiverOrHiddenPrototype() along the way
to
> make it explicit?

Done.

https://codereview.chromium.org/429053005/diff/1/src/ic.cc#newcode862
src/ic.cc:862: lookup->has_fast_properties() ? Code::FAST : Code::NORMAL);
On 2014/07/30 16:54:53, Toon Verwaest wrote:
> What about just doing lookup->holder_map()->is_dictionary_map() and removing
> this method again from the LookupIterator?
> 
> The difference between Code::FAST and Code::NORMAL should go away any time
soon
> now anyway ;)

Done.

https://codereview.chromium.org/429053005/diff/1/src/ic.cc#newcode967
src/ic.cc:967: Handle<JSObject> holder(lookup->GetHolder<JSObject>());
On 2014/07/30 16:54:52, Toon Verwaest wrote:
> Handle<JSObject> holder = lookup->GetHolder<JSObject>();

Done.

https://codereview.chromium.org/429053005/diff/1/src/ic.cc#newcode977
src/ic.cc:977: switch (lookup->property_details().type()) {
On 2014/07/30 16:54:52, Toon Verwaest wrote:
> What about doing the same as GetProperty; using property_kind() to switch
> between callbacks and others, then using property_encoding for dict vs
> field/constant; and only at the end using property_details?
> 
> I'd like to get rid of the current form of PropertyType at some point.

Done.

https://codereview.chromium.org/429053005/diff/1/src/ic.cc#newcode997
src/ic.cc:997: type, global, cell, name, !lookup->IsConfigurable());
On 2014/07/30 16:54:52, Toon Verwaest wrote:
> What about changing the interface of CompileLoadGlobal? Otherwise you negate
the
> flag 3 times :)
> !is_dont_delete -> !configurable -> !is_dont_delete

Done.

https://codereview.chromium.org/429053005/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/429053005/diff/1/src/objects.cc#newcode15169
src/objects.cc:15169: PropertyCell*
GlobalObject::GetPropertyCell(LookupIterator* lookup) {
On 2014/07/30 16:54:53, Toon Verwaest wrote:
> Let the LookupIterator return the cell instead.

Done.

[Toon Verwaest, 2014-07-31 14:49:52]

lgtm

[Jakob Kummerow, 2014-08-04 08:35:08]

Committed patchset #3 manually as r22802 (presubmit successful).