Guys, Thanks a lot for thorough review! I'm fixing the issues (and adding tests).
As for the stats, in Dromaeo DOM core almost 100% of call ICs (both constant and interceptor constant) do in fact call FastHandleApiCall. -- Vitaly On Thu, Feb 4, 2010 at 2:49 PM, <[email protected]> wrote: > 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? > > http://codereview.chromium.org/573003 > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
