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. http://codereview.chromium.org/573003 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
