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
