How faster does it make us?
http://codereview.chromium.org/4456002/diff/29001/30001 File src/code-stubs.h (right): http://codereview.chromium.org/4456002/diff/29001/30001#newcode556 src/code-stubs.h:556: : info_(info), fun_(fun) { } just a nit: could you make indentation of this and ApiGetterEntryStub ctor identical? http://codereview.chromium.org/4456002/diff/29001/30002 File src/codegen.cc (right): http://codereview.chromium.org/4456002/diff/29001/30002#newcode500 src/codegen.cc:500: bool ApiCallEntryStub::GetCustomCache(Code** code_out) { I hope we could never have two api functions with the same CallHandlerInfo, but this assumption makes me slightly nervous (same applies to getters). I wonder if we could check it somehow (maybe in debug mode) http://codereview.chromium.org/4456002/diff/29001/30002#newcode500 src/codegen.cc:500: bool ApiCallEntryStub::GetCustomCache(Code** code_out) { as of now, ApiCallEntryStub seems completely generic over CallHandlerInfo (but see comments below), should we cache it on CallHandlerInfo at all (ditto for getters)? http://codereview.chromium.org/4456002/diff/29001/30002#newcode501 src/codegen.cc:501: Object* cache = info()->call_stub_cache(); smells like unnecessary code duplication to me, any chances for a nice refactoring? http://codereview.chromium.org/4456002/diff/29001/30004 File src/ia32/code-stubs-ia32.cc (right): http://codereview.chromium.org/4456002/diff/29001/30004#newcode3070 src/ia32/code-stubs-ia32.cc:3070: void ApiCallEntryStub::Generate(MacroAssembler* masm) { data (like v8::Arguments::Data) appear to be a property of CallHandlerInfo on which you cache a stub, shouldn't setup of data be performed from ApiCallEntryStub? http://codereview.chromium.org/4456002/diff/29001/30004#newcode3082 src/ia32/code-stubs-ia32.cc:3082: // v8::InvocationC allback's argument. nit: unnecessary space: InvocationC[ ]allback http://codereview.chromium.org/4456002/diff/29001/30006 File src/ia32/stub-cache-ia32.cc (left): http://codereview.chromium.org/4456002/diff/29001/30006#oldcode496 src/ia32/stub-cache-ia32.cc:496: Builtins::builtin(Builtins::FastHandleApiCall)); do we need this builtin any more? http://codereview.chromium.org/4456002/diff/29001/30006 File src/ia32/stub-cache-ia32.cc (right): http://codereview.chromium.org/4456002/diff/29001/30006#newcode433 src/ia32/stub-cache-ia32.cc:433: // -- esp[16] : first fast api call extra argument probably it should be 12 now (and below) http://codereview.chromium.org/4456002/diff/29001/30006#newcode437 src/ia32/stub-cache-ia32.cc:437: __ add(Operand(esp), Immediate(kPointerSize * 3)); maybe it's time to introduce a constant for a number of cells reserved. http://codereview.chromium.org/4456002/diff/29001/30006#newcode481 src/ia32/stub-cache-ia32.cc:481: __ mov(edx, Immediate(argc)); I think Set(edx, Immediate(argc)) macro should be preferred http://codereview.chromium.org/4456002/diff/29001/30006#newcode484 src/ia32/stub-cache-ia32.cc:484: Address getter_address = v8::ToCData<Address>(callback); getter_address is probably misleading: it's not a getter, but api_function http://codereview.chromium.org/4456002/diff/29001/30006#newcode489 src/ia32/stub-cache-ia32.cc:489: __ EnterInternalFrame(); why do we need to enter internal frame here? http://codereview.chromium.org/4456002/diff/29001/30006#newcode490 src/ia32/stub-cache-ia32.cc:490: MaybeObject* result = __ TryCallStub(&stub); please, don't use __ in expressions: masm()-> or masm-> http://codereview.chromium.org/4456002/diff/29001/30006#newcode490 src/ia32/stub-cache-ia32.cc:490: MaybeObject* result = __ TryCallStub(&stub); probably you'd better comment why you use TryCallStub here. http://codereview.chromium.org/4456002/diff/29001/30007 File src/objects-debug.cc (right): http://codereview.chromium.org/4456002/diff/29001/30007#newcode1057 src/objects-debug.cc:1057: data()->ShortPrint(); Do you want to add cache printing as well? I think at least status (undefined/code) info would be handy http://codereview.chromium.org/4456002/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
