When compiling a stub for store with interceptor, check if this interceptor...

Don't do local lookup on global object as those (currently) are not JITed anyway.


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

[Mads Ager (chromium), 2009-06-02 17:22:44]

LGTM

Do we have test cases for this?

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

http://codereview.chromium.org/119048/diff/1/2#newcode878
Line 878: bool interceptor_found = lookup.IsValid() && lookup.IsCacheable()
Could you include a comment?

Maybe combine interceptor_found and receiver->...->IsUndefined() to one
has_interceptor_setter bool?

I guess there is a symmetric case for a property that has an interceptor setter
but no getter?

[Kasper Lund, 2009-06-02 18:31:41]

LGTM.

[antonm, 2009-06-02 19:42:29]

Thanks a lot Mads!

I've added a test case, but...

For some reason ScriptUsingAsciiStringResource started to fail.  Most
surprisingly it fails even if I add empty test.  Does anybody know what goes on?
 It was the same story with load interceptor, but it cured itself somehow.

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

http://codereview.chromium.org/119048/diff/1/2#newcode878
Line 878: bool interceptor_found = lookup.IsValid() && lookup.IsCacheable()
On 2009/06/02 17:22:44, Mads Ager wrote:
> Could you include a comment?
> 
> Maybe combine interceptor_found and receiver->...->IsUndefined() to one
> has_interceptor_setter bool?
> 
> I guess there is a symmetric case for a property that has an interceptor
setter
> but no getter?
> 

What kind of comment?

I reworked this piece notably and noted that if receiver is global proxy we do a
lookup just to be immediately discarded.  And surprise surprise for older
dom_bench 90.5% (sic!) try to do that.  Alas, peacekeeper doesn't do anything of
that kind :(

And yes, there is a symmetric case.  However, at least for DOM (but I'd guess
overall), it's more typical to have only a getter, not vice versa.  Maybe file
an issue?

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

LGTM

On 2009/06/02 19:42:29, antonm wrote:
> Thanks a lot Mads!
> 
> I've added a test case, but...
> 
> For some reason ScriptUsingAsciiStringResource started to fail.  Most
> surprisingly it fails even if I add empty test.  Does anybody know what goes
on?
>  It was the same story with load interceptor, but it cured itself somehow.

Odd, I have no idea.  How does it fail?

> http://codereview.chromium.org/119048/diff/1/2
> File src/ic.cc (right):
> 
> http://codereview.chromium.org/119048/diff/1/2#newcode878
> Line 878: bool interceptor_found = lookup.IsValid() && lookup.IsCacheable()
> On 2009/06/02 17:22:44, Mads Ager wrote:
> > Could you include a comment?
> > 
> > Maybe combine interceptor_found and receiver->...->IsUndefined() to one
> > has_interceptor_setter bool?
> > 
> > I guess there is a symmetric case for a property that has an interceptor
> setter
> > but no getter?
> > 
> 
> What kind of comment?

With the new naming I think it is fine without a comment.  Thanks.

The reason for not generating store IC stubs for JSGlobalProxy is that the stubs
do not correctly handle the JSGlobalProxy object (it would store directly in the
proxy object which should never be done).  We should fix that.

[antonm, 2009-06-04 13:28:04]

Kasper, Mads, tnx a lot for review.

I did some more profiling and found out that apparently check if interceptor has
a setter might be sometimes expensive.  So for this CL only saves us a lookup on
global object.

What would you say if I try to reserve another bit for fast check if interceptor
has a setter (like has_named_interceptor do)?  How precious these bits are?