Store fast property index in stubs generated for interceptor loads

In case of holder with fast properties that allows to fetch the property
immediately if holder has this property or saves binary search on holder if
property doesn't belong to holder.  Of course, in the cases when named getter
returns nothing.

That gives ~20% for dom benchmark/Document Object String Get, speeds up overall
dom_perf (not dramatically) and overall score for peacekeeper.  Strange, but DOM
part of peacekeepr runs somewhat slower.

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

[Mads Ager (chromium), 2009-06-02 17:14:04]

LGTM when the following comments have been addressed.  

This is good stuff.

Also, you should implement this for ARM as well at which point I guess one of
the GenerateLoadInterceptor functions can be removed?

http://codereview.chromium.org/118118/diff/1/4
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/118118/diff/1/4#newcode363
Line 363: // TODO(367): maybe don't push lookup_hint for LOOKUP_IN_HOLDER and/or
We normally capitalize the start of these TODO(xxx): messages: 'maybe' ->
'Maybe'.

http://codereview.chromium.org/118118/diff/1/4#newcode1001
Line 1001: // TODO(368): compile in the whole chain: all the interceptors in
Capitalize for consistency with other TODOs: 'compile' -> 'Compile'.

http://codereview.chromium.org/118118/diff/1/7
File src/objects-inl.h (right):

http://codereview.chromium.org/118118/diff/1/7#newcode2558
Line 2558: // TODO(antonm): do we want to do any shortcuts for global object?
I'm not sure I understand this TODO.  Could you elaborate (and potentially file
a bug report and use the number)?

http://codereview.chromium.org/118118/diff/1/3
File src/objects.cc (right):

http://codereview.chromium.org/118118/diff/1/3#newcode5621
Line 5621: JSObject* receiver, String* name, PropertyAttributes* attributes) {
Use one line per argument here (as in GetInterceptorPropertyWithLookupHint)?

http://codereview.chromium.org/118118/diff/1/3#newcode5664
Line 5664: RETURN_IF_SCHEDULED_EXCEPTION();
I'm not sure this one is needed?  I think that GetPropertyWithInterceptorProper
handles scheduled exceptions by returning Failure::Exception.  In that case, you
should just propatate that out which is done by the return in the next line.

http://codereview.chromium.org/118118/diff/1/3#newcode5676
Line 5676: RETURN_IF_SCHEDULED_EXCEPTION();
Is this one needed?  What here can cause an exception to be scheduled?

http://codereview.chromium.org/118118/diff/1/3#newcode5702
Line 5702: JSObject* receiver, String* name, PropertyAttributes* attributes) {
One line per argument?

http://codereview.chromium.org/118118/diff/1/3#newcode5709
Line 5709: RETURN_IF_SCHEDULED_EXCEPTION();
Is this one needed or is it handled by GetPropertyWithInterceptorProper already?

http://codereview.chromium.org/118118/diff/1/6
File src/objects.h (right):

http://codereview.chromium.org/118118/diff/1/6#newcode1354
Line 1354: Smi* payload,
'payload' -> 'hint'?

http://codereview.chromium.org/118118/diff/1/6#newcode1357
Line 1357: enum { LOOKUP_IN_HOLDER = -1, LOOKUP_IN_PROTOTYPE = -2 };
For consistency with the rest of the code I would add these as constants to the
list of constants later in the class definition (kMaxGap and friends).

static const int kLookupInHolderHint = -1;
static const int kLookupInPrototypeHint = -2;

http://codereview.chromium.org/118118/diff/1/8
File src/stub-cache.h (right):

http://codereview.chromium.org/118118/diff/1/8#newcode363
Line 363: Smi* lookup_payload,
lookup_payload -> lookup_hint?

[antonm, 2009-06-02 18:14:46]

Thanks a lot for review, Mads!

http://codereview.chromium.org/118118/diff/1/4
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/118118/diff/1/4#newcode363
Line 363: // TODO(367): maybe don't push lookup_hint for LOOKUP_IN_HOLDER and/or
On 2009/06/02 17:14:04, Mads Ager wrote:
> We normally capitalize the start of these TODO(xxx): messages: 'maybe' ->
> 'Maybe'.

Done.

http://codereview.chromium.org/118118/diff/1/4#newcode1001
Line 1001: // TODO(368): compile in the whole chain: all the interceptors in
On 2009/06/02 17:14:04, Mads Ager wrote:
> Capitalize for consistency with other TODOs: 'compile' -> 'Compile'.

Done.

http://codereview.chromium.org/118118/diff/1/7
File src/objects-inl.h (right):

http://codereview.chromium.org/118118/diff/1/7#newcode2558
Line 2558: // TODO(antonm): do we want to do any shortcuts for global object?
On 2009/06/02 17:14:04, Mads Ager wrote:
> I'm not sure I understand this TODO.  Could you elaborate (and potentially
file
> a bug report and use the number)?

Sure.  The code below is pretty close to JSObject::LocalLookupRealNamedProperty,
but doesn't have the check IsJSGlobalProxy() (cf. with the method), so I wonder
if it should be here as well.

BTW, what do you think about it?

http://codereview.chromium.org/118118/diff/1/3
File src/objects.cc (right):

http://codereview.chromium.org/118118/diff/1/3#newcode5621
Line 5621: JSObject* receiver, String* name, PropertyAttributes* attributes) {
On 2009/06/02 17:14:04, Mads Ager wrote:
> Use one line per argument here (as in GetInterceptorPropertyWithLookupHint)?

The problem is that if formatted as method_name(arg1,
                                                arg2

it goes out of 80 limit.  What would you say about this variant?

http://codereview.chromium.org/118118/diff/1/3#newcode5664
Line 5664: RETURN_IF_SCHEDULED_EXCEPTION();
On 2009/06/02 17:14:04, Mads Ager wrote:
> I'm not sure this one is needed?  I think that
GetPropertyWithInterceptorProper
> handles scheduled exceptions by returning Failure::Exception.  In that case,
you
> should just propatate that out which is done by the return in the next line.

Aha, thanks.  Done.

http://codereview.chromium.org/118118/diff/1/3#newcode5676
Line 5676: RETURN_IF_SCHEDULED_EXCEPTION();
On 2009/06/02 17:14:04, Mads Ager wrote:
> Is this one needed?  What here can cause an exception to be scheduled?

Sorry, should have swapped with the line below.

http://codereview.chromium.org/118118/diff/1/3#newcode5702
Line 5702: JSObject* receiver, String* name, PropertyAttributes* attributes) {
On 2009/06/02 17:14:04, Mads Ager wrote:
> One line per argument?

Done.  Same problem as above: otherwise breaks 80 limit

http://codereview.chromium.org/118118/diff/1/3#newcode5709
Line 5709: RETURN_IF_SCHEDULED_EXCEPTION();
On 2009/06/02 17:14:04, Mads Ager wrote:
> Is this one needed or is it handled by GetPropertyWithInterceptorProper
already?

Done.

http://codereview.chromium.org/118118/diff/1/6
File src/objects.h (right):

http://codereview.chromium.org/118118/diff/1/6#newcode1354
Line 1354: Smi* payload,
On 2009/06/02 17:14:04, Mads Ager wrote:
> 'payload' -> 'hint'?

Done.

http://codereview.chromium.org/118118/diff/1/6#newcode1357
Line 1357: enum { LOOKUP_IN_HOLDER = -1, LOOKUP_IN_PROTOTYPE = -2 };
On 2009/06/02 17:14:04, Mads Ager wrote:
> For consistency with the rest of the code I would add these as constants to
the
> list of constants later in the class definition (kMaxGap and friends).
> 
> static const int kLookupInHolderHint = -1;
> static const int kLookupInPrototypeHint = -2;
> 
> 

Done.

http://codereview.chromium.org/118118/diff/1/8
File src/stub-cache.h (right):

http://codereview.chromium.org/118118/diff/1/8#newcode363
Line 363: Smi* lookup_payload,
On 2009/06/02 17:14:04, Mads Ager wrote:
> lookup_payload -> lookup_hint?

Done.

[Kasper Lund, 2009-06-02 18:27:16]

LGTM.

http://codereview.chromium.org/118118/diff/1006/1009
File src/objects-inl.h (right):

http://codereview.chromium.org/118118/diff/1006/1009#newcode2558
Line 2558: // TODO(antonm): do we want to do any shortcuts for global object?
do -> Do

[Mads Ager (chromium), 2009-06-03 05:56:27]

Still LGTM!

http://codereview.chromium.org/118118/diff/1/7
File src/objects-inl.h (right):

http://codereview.chromium.org/118118/diff/1/7#newcode2558
Line 2558: // TODO(antonm): do we want to do any shortcuts for global object?
On 2009/06/02 18:14:46, antonm wrote:
> On 2009/06/02 17:14:04, Mads Ager wrote:
> > I'm not sure I understand this TODO.  Could you elaborate (and potentially
> file
> > a bug report and use the number)?
> 
> Sure.  The code below is pretty close to
JSObject::LocalLookupRealNamedProperty,
> but doesn't have the check IsJSGlobalProxy() (cf. with the method), so I
wonder
> if it should be here as well.
>
> BTW, what do you think about it?

Ah, I understand now, thanks.  I don't think the shortcutting for the
JSGlobalProxy matters much for the current use of interceptors in chromium. 
However, the shortcutting is easy to do, so it probably makes sense to add it. 
If we add the shortcutting for the JSGlobalProxy here, we have to make sure to
add it to GetInterceptorPropertyWithLookupHint as well so we use the hint on the
right object.  It is fine to leave that for another changelist.

http://codereview.chromium.org/118118/diff/1/3
File src/objects.cc (right):

http://codereview.chromium.org/118118/diff/1/3#newcode5621
Line 5621: JSObject* receiver, String* name, PropertyAttributes* attributes) {
On 2009/06/02 18:14:46, antonm wrote:
> On 2009/06/02 17:14:04, Mads Ager wrote:
> > Use one line per argument here (as in GetInterceptorPropertyWithLookupHint)?
> 
> The problem is that if formatted as method_name(arg1,
>                                                 arg2
> 
> it goes out of 80 limit.  What would you say about this variant?

In that case, I normally do:

method_name(
    arg1,
    arg2,
    ...)