ia32: Fuse map and type checks in call ICs for API functions.

ia32: Fuse map and type checks in call ICs for API functions.

This uses the fact that if a map stayed the same then the object
still passes the type check. A new builtin is added to handle the
API call in this case.

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

[Vitaly Repeshko, 2010-02-04 08:21:13]

Mads, Anton,

This is only for ia32 for now. I'll file a bug for other platforms before
submitting. I'm working on adding some tests. Any suggestions on how to test
this are welcome.


Thanks,
Vitaly

[Mads Ager (chromium), 2010-02-04 09:52:01]

This stuff is getting hairy. :)  In particular, I'm uncertain about the GC
safety of the pushing of the object that passed the type check.  Could you at
least add a comment explaining why it is safe?

Please add some API tests for this.  Insert '__ int3();' in the branch that you
are trying to test to verify that you hit it and see if you can make the test
exhaustive.

http://codereview.chromium.org/573003/diff/1009/11
File src/builtins.cc (right):

http://codereview.chromium.org/573003/diff/1009/11#newcode520
src/builtins.cc:520: false/* is_construct */,
Could you add a named 'bool is_construct = false;' and use that instead of
having a comment here?

http://codereview.chromium.org/573003/diff/1009/15
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/573003/diff/1009/15#newcode492
src/ia32/stub-cache-ia32.cc:492: // So far the most popular case for failed
interceptor is
I'm not sure I understand this comment, could you expand or just state that you
only consider optimizing the call if it is a call of a constant function
property?

http://codereview.chromium.org/573003/diff/1009/15#newcode730
src/ia32/stub-cache-ia32.cc:730: __ push(Immediate(0));
Please explicitly smi tag: __ push(Immediate(Smi::FromInt(0)));

Are you in an internal frame when you push this so the GC knows about the value,
or are you 100% sure that a GC cannot happen between the time when you push on
the stack and the time when you pop again?

http://codereview.chromium.org/573003/diff/1009/15#newcode1248
src/ia32/stub-cache-ia32.cc:1248: __ push(Immediate(0));
Please explicitly Smi tag: __ push(Immediate(Smi::FromInt(0)))

http://codereview.chromium.org/573003/diff/1009/18
File src/v8-counters.h (right):

http://codereview.chromium.org/573003/diff/1009/18#newcode146
src/v8-counters.h:146: SC(call_const_interceptor_fast_api,
V8.CallConstInterceptorFastApi) \
Please right align all the '\'

[antonm, 2010-02-04 11:49:51]

overall comment: could you through in more tests for the cases you added?

http://codereview.chromium.org/573003/diff/1009/15
File src/ia32/stub-cache-ia32.cc (left):

http://codereview.chromium.org/573003/diff/1009/15#oldcode1112
src/ia32/stub-cache-ia32.cc:1112: CompileLoadInterceptor(&compiler,
maybe you should get rid of this helper function and add ::Compile method to
LoadInterceptorCompiler.

http://codereview.chromium.org/573003/diff/1009/15
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/573003/diff/1009/15#newcode534
src/ia32/stub-cache-ia32.cc:534: if (expected_receiver_type_ == NULL ||
expected_receiver_type_ doesn't change in this function call?  if yes, could the
check be lifted at the beginning of the function?

http://codereview.chromium.org/573003/diff/1009/15#newcode538
src/ia32/stub-cache-ia32.cc:538: object =
JSObject::cast(object->GetPrototype());
is it possible case: imagine holder has null as a proto.  and we need to
traverse the whole chain (btw, is it really possible?)---in this case when
object == holder, this cast would fail, no?

http://codereview.chromium.org/573003/diff/1009/15#newcode554
src/ia32/stub-cache-ia32.cc:554: bool IsSimpleApiFunction(JSFunction* function)
{
that might be my personal preference, so feel free to ignore, but I am somewhat
uneasy with functions which have both side-effects on fields and returns a value
to be assigned to field.  Maybe is_simple_api_call_ assignment could be moved
into IsSimpleApiFunction?  And the name should be probably changed then.

http://codereview.chromium.org/573003/diff/1009/15#newcode582
src/ia32/stub-cache-ia32.cc:582: return object->IsJSArray();
maybe this function is worth commenting.

http://codereview.chromium.org/573003/diff/1009/15#newcode590
src/ia32/stub-cache-ia32.cc:590: //  -- esp[0]              : object passing the
type check
object passing type check a.k.a. holder?  if yes, maybe it's a better name?

http://codereview.chromium.org/573003/diff/1009/15#newcode715
src/ia32/stub-cache-ia32.cc:715: !lookup->holder()->IsGlobalObject()) {
I think we don't have constant functions on global objects, so is it possible to
have optimization.is_contant_call() and lookup->holder()->IsGlobalObject()?  But
I think you leave it for my refactoring, correct?

http://codereview.chromium.org/573003/diff/1009/15#newcode721
src/ia32/stub-cache-ia32.cc:721: can_do_fast_api_call = (depth1 !=
kInvalidProtoDepth) ||
just curious, do you know the case when depth1 != kInvalidProtoDepth?  Probably
we could synthesize one, but if I understand it correctly, that should be really
sketchy.

http://codereview.chromium.org/573003/diff/1009/15#newcode728
src/ia32/stub-cache-ia32.cc:728: __
IncrementCounter(&Counters::call_const_interceptor_fast_api, 1);
just curious, what those counters say us about ratio of fast api vs.
const_interceptor for, say, DOM core?

http://codereview.chromium.org/573003/diff/1009/15#newcode729
src/ia32/stub-cache-ia32.cc:729: // Reserve space for the object that will pass
the type check.
maybe it's worth adding a comment that this object would be overwritten from
CheckPrototypes() below

http://codereview.chromium.org/573003/diff/1009/15#newcode1247
src/ia32/stub-cache-ia32.cc:1247: // Reserve space for the object that will pass
the type check.
ditto for reference to CheckPrototypes(), but it's up to you

http://codereview.chromium.org/573003/diff/1009/17
File src/stub-cache.h (right):

http://codereview.chromium.org/573003/diff/1009/17#newcode394
src/stub-cache.h:394: Register CheckPrototypes(JSObject* object,
maybe save_at_depth could be an optional parameter?

http://codereview.chromium.org/573003/diff/1009/18
File src/v8-counters.h (right):

http://codereview.chromium.org/573003/diff/1009/18#newcode146
src/v8-counters.h:146: SC(call_const_interceptor_fast_api,
V8.CallConstInterceptorFastApi) \
could you share with us any stats you gathered?

[Vitaly Repeshko, 2010-02-05 18:48:36]

All issues address. Tests added. Please take another look.


Thanks,
Vitaly

http://codereview.chromium.org/573003/diff/1009/11
File src/builtins.cc (right):

http://codereview.chromium.org/573003/diff/1009/11#newcode520
src/builtins.cc:520: false/* is_construct */,
On 2010/02/04 09:52:01, Mads Ager wrote:
> Could you add a named 'bool is_construct = false;' and use that instead of
> having a comment here?

Done.

http://codereview.chromium.org/573003/diff/1009/15
File src/ia32/stub-cache-ia32.cc (left):

http://codereview.chromium.org/573003/diff/1009/15#oldcode1112
src/ia32/stub-cache-ia32.cc:1112: CompileLoadInterceptor(&compiler,
On 2010/02/04 11:49:51, antonm wrote:
> maybe you should get rid of this helper function and add ::Compile method to
> LoadInterceptorCompiler.

I'd prefer this to happen in a separate patch.

http://codereview.chromium.org/573003/diff/1009/15
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/573003/diff/1009/15#newcode534
src/ia32/stub-cache-ia32.cc:534: if (expected_receiver_type_ == NULL ||
On 2010/02/04 11:49:51, antonm wrote:
> expected_receiver_type_ doesn't change in this function call?  if yes, could
the
> check be lifted at the beginning of the function?

Done.

http://codereview.chromium.org/573003/diff/1009/15#newcode538
src/ia32/stub-cache-ia32.cc:538: object =
JSObject::cast(object->GetPrototype());
On 2010/02/04 11:49:51, antonm wrote:
> is it possible case: imagine holder has null as a proto.  and we need to
> traverse the whole chain (btw, is it really possible?)---in this case when
> object == holder, this cast would fail, no?

Good catch. Fixed.

http://codereview.chromium.org/573003/diff/1009/15#newcode554
src/ia32/stub-cache-ia32.cc:554: bool IsSimpleApiFunction(JSFunction* function)
{
On 2010/02/04 11:49:51, antonm wrote:
> that might be my personal preference, so feel free to ignore, but I am
somewhat
> uneasy with functions which have both side-effects on fields and returns a
value
> to be assigned to field.  Maybe is_simple_api_call_ assignment could be moved
> into IsSimpleApiFunction?  And the name should be probably changed then.

Yeah, I don't like this style either. Did a small refactoring here.

http://codereview.chromium.org/573003/diff/1009/15#newcode582
src/ia32/stub-cache-ia32.cc:582: return object->IsJSArray();
On 2010/02/04 11:49:51, antonm wrote:
> maybe this function is worth commenting.

Moved this to Top, next to LookupSpecialFunction.

http://codereview.chromium.org/573003/diff/1009/15#newcode590
src/ia32/stub-cache-ia32.cc:590: //  -- esp[0]              : object passing the
type check
On 2010/02/04 11:49:51, antonm wrote:
> object passing type check a.k.a. holder?  if yes, maybe it's a better name?

In this context "holder" is already overloaded: interceptor holder vs. property
holder. Clearly another one is too much.

http://codereview.chromium.org/573003/diff/1009/15#newcode715
src/ia32/stub-cache-ia32.cc:715: !lookup->holder()->IsGlobalObject()) {
On 2010/02/04 11:49:51, antonm wrote:
> I think we don't have constant functions on global objects, so is it possible
to
> have optimization.is_contant_call() and lookup->holder()->IsGlobalObject()? 
But
> I think you leave it for my refactoring, correct?

Yup.

http://codereview.chromium.org/573003/diff/1009/15#newcode721
src/ia32/stub-cache-ia32.cc:721: can_do_fast_api_call = (depth1 !=
kInvalidProtoDepth) ||
On 2010/02/04 11:49:51, antonm wrote:
> just curious, do you know the case when depth1 != kInvalidProtoDepth? 
Probably
> we could synthesize one, but if I understand it correctly, that should be
really
> sketchy.

Actually, depth1 is usually 0, because the receiver passes the type check. To
get greater depths you need some hackery with __proto__ (including hidden
prototypes).

http://codereview.chromium.org/573003/diff/1009/15#newcode728
src/ia32/stub-cache-ia32.cc:728: __
IncrementCounter(&Counters::call_const_interceptor_fast_api, 1);
On 2010/02/04 11:49:51, antonm wrote:
> just curious, what those counters say us about ratio of fast api vs.
> const_interceptor for, say, DOM core?

I posted some info to the review thread. It's almost 100%.

http://codereview.chromium.org/573003/diff/1009/15#newcode729
src/ia32/stub-cache-ia32.cc:729: // Reserve space for the object that will pass
the type check.
On 2010/02/04 11:49:51, antonm wrote:
> maybe it's worth adding a comment that this object would be overwritten from
> CheckPrototypes() below

There's now a better function call here.

http://codereview.chromium.org/573003/diff/1009/15#newcode730
src/ia32/stub-cache-ia32.cc:730: __ push(Immediate(0));
On 2010/02/04 09:52:01, Mads Ager wrote:
> Please explicitly smi tag: __ push(Immediate(Smi::FromInt(0)));
> 
> Are you in an internal frame when you push this so the GC knows about the
value,
> or are you 100% sure that a GC cannot happen between the time when you push on
> the stack and the time when you pop again?

Good catch. GC actually can happen during the interceptor call so the found
object has to be put in the caller's internal frame right from the beginning. I
refactored the code to do exactly this.

http://codereview.chromium.org/573003/diff/1009/15#newcode1247
src/ia32/stub-cache-ia32.cc:1247: // Reserve space for the object that will pass
the type check.
On 2010/02/04 11:49:51, antonm wrote:
> ditto for reference to CheckPrototypes(), but it's up to you

See above.

http://codereview.chromium.org/573003/diff/1009/15#newcode1248
src/ia32/stub-cache-ia32.cc:1248: __ push(Immediate(0));
On 2010/02/04 09:52:01, Mads Ager wrote:
> Please explicitly Smi tag: __ push(Immediate(Smi::FromInt(0)))

See above. Handled by the refactoring.

http://codereview.chromium.org/573003/diff/1009/18
File src/v8-counters.h (right):

http://codereview.chromium.org/573003/diff/1009/18#newcode146
src/v8-counters.h:146: SC(call_const_interceptor_fast_api,
V8.CallConstInterceptorFastApi) \
On 2010/02/04 09:52:01, Mads Ager wrote:
> Please right align all the '\'

Done.

[Mads Ager (chromium), 2010-02-08 08:56:58]

LGTM

http://codereview.chromium.org/573003/diff/5002/8008
File src/stub-cache.h (right):

http://codereview.chromium.org/573003/diff/5002/8008#newcode394
src/stub-cache.h:394: Register CheckPrototypes(JSObject* object,
Add vertical space between the function declarations?

http://codereview.chromium.org/573003/diff/5002/8013
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/573003/diff/5002/8013#newcode5851
test/cctest/test-api.cc:5851: GenerateSomeGarbage();
Could you document why you generate some extra garbage in all of these tests?

http://codereview.chromium.org/573003/diff/5002/8013#newcode5855
test/cctest/test-api.cc:5855: "for (var i = 0; i < 1000; i++) {"
I think we use 1000 in other places as well, but do we need that many
iterations?  It is nice to have the tests run as fast as possible and still
cover all the cases.

[antonm, 2010-02-08 12:07:38]

lgtm

http://codereview.chromium.org/573003/diff/5002/8006
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/573003/diff/5002/8006#newcode803
src/ia32/stub-cache-ia32.cc:803: __ bind(&miss);
feel free to ignore, but in case when !can_do_fast_api_call this adds
unnecessary jump instruction---instead of branching to miss, we could
immediately branch to miss_label.  Apparently relatively easy to fix, but up to
you.

http://codereview.chromium.org/573003/diff/5002/8013
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/573003/diff/5002/8013#newcode5796
test/cctest/test-api.cc:5796: static v8::Handle<Value>
InterceptorCallICFastApi(Local<String> name,
I know that's boring, but apparently you didn't cover some cases like
invalidation due to overriding in interceptor's holder-cached const function's
holder chain.

http://codereview.chromium.org/573003/diff/5002/8013#newcode5858
test/cctest/test-api.cc:5858: CHECK_EQ(42,
context->Global()->Get(v8_str("result"))->Int32Value());
if you add result as a last line of script, you could use value here, might be
more convenient

[Vitaly Repeshko, 2010-02-09 16:15:24]

Thanks for review! Submitted.


-- Vitaly

http://codereview.chromium.org/573003/diff/5002/8006
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/573003/diff/5002/8006#newcode803
src/ia32/stub-cache-ia32.cc:803: __ bind(&miss);
On 2010/02/08 12:07:38, antonm wrote:
> feel free to ignore, but in case when !can_do_fast_api_call this adds
> unnecessary jump instruction---instead of branching to miss, we could
> immediately branch to miss_label.  Apparently relatively easy to fix, but up
to
> you.

Done.

http://codereview.chromium.org/573003/diff/5002/8008
File src/stub-cache.h (right):

http://codereview.chromium.org/573003/diff/5002/8008#newcode394
src/stub-cache.h:394: Register CheckPrototypes(JSObject* object,
On 2010/02/08 08:56:58, Mads Ager wrote:
> Add vertical space between the function declarations?

Done.

http://codereview.chromium.org/573003/diff/5002/8013
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/573003/diff/5002/8013#newcode5796
test/cctest/test-api.cc:5796: static v8::Handle<Value>
InterceptorCallICFastApi(Local<String> name,
On 2010/02/08 12:07:38, antonm wrote:
> I know that's boring, but apparently you didn't cover some cases like
> invalidation due to overriding in interceptor's holder-cached const function's
> holder chain.

Thanks, added a test. Yeah, it's boring...

http://codereview.chromium.org/573003/diff/5002/8013#newcode5851
test/cctest/test-api.cc:5851: GenerateSomeGarbage();
On 2010/02/08 08:56:58, Mads Ager wrote:
> Could you document why you generate some extra garbage in all of these tests?

Done.

http://codereview.chromium.org/573003/diff/5002/8013#newcode5855
test/cctest/test-api.cc:5855: "for (var i = 0; i < 1000; i++) {"
On 2010/02/08 08:56:58, Mads Ager wrote:
> I think we use 1000 in other places as well, but do we need that many
> iterations?  It is nice to have the tests run as fast as possible and still
> cover all the cases.

Sure we don't need that many. Reduced iterations by a factor of 10.

http://codereview.chromium.org/573003/diff/5002/8013#newcode5858
test/cctest/test-api.cc:5858: CHECK_EQ(42,
context->Global()->Get(v8_str("result"))->Int32Value());
On 2010/02/08 12:07:38, antonm wrote:
> if you add result as a last line of script, you could use value here, might be
> more convenient

Yup, but I prefer a slightly more uniform way here.