8-9% boost on DOM Query benchmark (before - http://dromaeo.com/?id=122108,122113,122119,122121,122123,122124, after - http://dromaeo.com/?id=122126,122128,122129,122130,122131,122132)
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) { } On 2010/11/08 13:12:33, antonm wrote:
just a nit: could you make indentation of this and ApiGetterEntryStub
ctor
identical?
Done. 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) { On 2010/11/08 13:12:33, antonm wrote:
as of now, ApiCallEntryStub seems completely generic over
CallHandlerInfo (but
see comments below), should we cache it on CallHandlerInfo at all
(ditto for
getters)?
I'd suggest to postpone it. It's not obvious for me now what is better: generalize this stub or remove it at all. http://codereview.chromium.org/4456002/diff/29001/30002#newcode500 src/codegen.cc:500: bool ApiCallEntryStub::GetCustomCache(Code** code_out) { On 2010/11/08 13:12:33, antonm wrote:
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)
CallHandlerInfo (as well as AccessorInfo) contains pointer to the bound function. Actually the last ctor parameter of both classes are redundant. I'd remove it, but in a separate CL (since it requires to touch x64 files). http://codereview.chromium.org/4456002/diff/29001/30002#newcode501 src/codegen.cc:501: Object* cache = info()->call_stub_cache(); On 2010/11/08 13:12:33, antonm wrote:
smells like unnecessary code duplication to me, any chances for a nice refactoring?
Done. 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) { On 2010/11/08 13:12:33, antonm wrote:
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?
For now I'd prefer to pack all the parameters in the same piece of code. However if we made this stub generic it would make much more sense. http://codereview.chromium.org/4456002/diff/29001/30004#newcode3082 src/ia32/code-stubs-ia32.cc:3082: // v8::InvocationC allback's argument. On 2010/11/08 13:12:33, antonm wrote:
nit: unnecessary space: InvocationC[ ]allback
Done. 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)); On 2010/11/08 13:12:33, antonm wrote:
do we need this builtin any more?
For x64 & arm 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 On 2010/11/08 13:12:33, antonm wrote:
probably it should be 12 now (and below)
Fixed. I found comments like esp[kArgumentsImplisitItems * 4] too complicated so I left 12. http://codereview.chromium.org/4456002/diff/29001/30006#newcode437 src/ia32/stub-cache-ia32.cc:437: __ add(Operand(esp), Immediate(kPointerSize * 3)); On 2010/11/08 13:12:33, antonm wrote:
maybe it's time to introduce a constant for a number of cells
reserved. Done. http://codereview.chromium.org/4456002/diff/29001/30006#newcode481 src/ia32/stub-cache-ia32.cc:481: __ mov(edx, Immediate(argc)); On 2010/11/08 13:12:33, antonm wrote:
I think Set(edx, Immediate(argc)) macro should be preferred
Done. http://codereview.chromium.org/4456002/diff/29001/30006#newcode484 src/ia32/stub-cache-ia32.cc:484: Address getter_address = v8::ToCData<Address>(callback); On 2010/11/08 13:12:33, antonm wrote:
getter_address is probably misleading: it's not a getter, but
api_function Done. http://codereview.chromium.org/4456002/diff/29001/30006#newcode489 src/ia32/stub-cache-ia32.cc:489: __ EnterInternalFrame(); On 2010/11/08 13:12:33, antonm wrote:
why do we need to enter internal frame here?
Removing this frame causes test crash. Probably without this frame the return address is not patched correctly if the stub is relocated. http://codereview.chromium.org/4456002/diff/29001/30006#newcode490 src/ia32/stub-cache-ia32.cc:490: MaybeObject* result = __ TryCallStub(&stub); On 2010/11/08 13:12:33, antonm wrote:
please, don't use __ in expressions: masm()-> or masm->
Done. http://codereview.chromium.org/4456002/diff/29001/30006#newcode490 src/ia32/stub-cache-ia32.cc:490: MaybeObject* result = __ TryCallStub(&stub); On 2010/11/08 13:12:33, antonm wrote:
probably you'd better comment why you use TryCallStub here.
Done. 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(); On 2010/11/08 13:12:33, antonm wrote:
Do you want to add cache printing as well? I think at least status (undefined/code) info would be handy
Done. http://codereview.chromium.org/4456002/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
