Direct call API functions.

Direct call API functions (ia32 implementation).


Committed: http://code.google.com/p/v8/source/detail?r=5791

[antonm, 2010-11-08 13:12:33]

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

[SeRya, 2010-11-09 11:38:35]

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.

[antonm, 2010-11-09 13:15:25]

LGTM

http://codereview.chromium.org/4456002/diff/58002/79006
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/4456002/diff/58002/79006#newcode417
src/ia32/stub-cache-ia32.cc:417: static const int kArgumentsImplicitItems = 3;
I would rather prefer to mention fast api call in the name, than implicit args.

http://codereview.chromium.org/4456002/diff/58002/79006#newcode443
src/ia32/stub-cache-ia32.cc:443: //  -- esp[12] : first fast api call extra
argument
to forget about troubles you can change 12 into something like 4 *
kArgumentsImplicitItems

[SeRya, 2010-11-09 13:51:14]

http://codereview.chromium.org/4456002/diff/58002/79006
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/4456002/diff/58002/79006#newcode417
src/ia32/stub-cache-ia32.cc:417: static const int kArgumentsImplicitItems = 3;
On 2010/11/09 13:15:25, antonm wrote:
> I would rather prefer to mention fast api call in the name, than implicit
args.

Done.

http://codereview.chromium.org/4456002/diff/58002/79006#newcode443
src/ia32/stub-cache-ia32.cc:443: //  -- esp[12] : first fast api call extra
argument
On 2010/11/09 13:15:25, antonm wrote:
> to forget about troubles you can change 12 into something like 4 *
> kArgumentsImplicitItems

Done.

[antonm, 2010-11-09 13:56:19]

Still LGTM

http://codereview.chromium.org/4456002/diff/83002/47007
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/4456002/diff/83002/47007#newcode416
src/ia32/stub-cache-ia32.cc:416: // Amount of pointers to be reserved on stack
for v8::Arguments::implicit_args_.
I am not a native speaker, but I would rather say 'number of pointers'

http://codereview.chromium.org/4456002/diff/83002/47007#newcode416
src/ia32/stub-cache-ia32.cc:416: // Amount of pointers to be reserved on stack
for v8::Arguments::implicit_args_.
'v8::Arguments::implicit_args_': should either be 'for fast API call' or
constant renamed back, imho