Removing redundant stubs for API functions.

Removing redundant stubs for API functions.


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

[antonm, 2010-11-10 14:53:54]

First round

http://codereview.chromium.org/4695003/diff/27001/src/code-stubs.h
File src/code-stubs.h (left):

http://codereview.chromium.org/4695003/diff/27001/src/code-stubs.h#oldcode535
src/code-stubs.h:535: virtual bool GetCustomCache(Code** code_out);
looks like these were the only stubs with custom caches, isn't it time to kill
this stuff?

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:504: __ PrepareCallApiFunction(argc +
kFastApiCallArguments + 1,
not sure if it's worth it, but maybe unify exit frame construction and arguments
pushing?

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:509: __ mov(ApiParameterOperand(3), edx);  //
v8::Arguments::length_.
cannot we fuse those moves and moves above (ln. 490--492), for example why not
drop __ Set(edx, Immediate(args)) @492 and fuse it with __
mov(ApiParemeterOperand(3), Immediate(argc))?

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:521: MaybeObject* result =
masm->TryCallApiFunctionAndReturn(&fun, kApiArgc +
that might be just me, but I am slightly uneasy with this indentation: I do
understand you want to emphasize the sum, but maybe do it differently:

MO* result =
    masm->......?

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1079: // Insert additional parameters into the stack
frame above return address.
are we sure it is always safe to piggy back on outer frame?

can we at least make sure tests fail if this assumption is wrong?  I'd rather
not to debug those issue in Chromium

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1080: ASSERT(!scratch3.is(reg));
shouldn't you check as well that !scratch{2,3}.is(receiver)?

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1109: // to the values (it considered as smi in GC).
are you talking about ApiParameters below?  if yes, I used to think they are not
GCed?

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1113: __ PrepareCallApiFunction(kStackSpace,
kApiArgc);
again, couldn't we merge pushing and exit frame setup?

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1116: __ mov(ApiParameterOperand(1), eax);  //
arguments pointer.
I won't be surprised if __mov(APO(0), ebx); __ add(ebx, kPointerSize);
__mov(APO(1), ebx); would be a shorter sequence with better performance.

Not insisting

http://codereview.chromium.org/4695003/diff/27001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/4695003/diff/27001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:506: __ movq(rcx, rsp);
should we pass a register to store rsp into as an argument?  or at least amend
the documentation, please

[SeRya, 2010-11-11 15:50:39]

The comments addressed except two about "unifying exit frame construction and
arguments". Anton, please explain.

Also fixed WIN64 crash.

http://codereview.chromium.org/4695003/diff/27001/src/code-stubs.h
File src/code-stubs.h (left):

http://codereview.chromium.org/4695003/diff/27001/src/code-stubs.h#oldcode535
src/code-stubs.h:535: virtual bool GetCustomCache(Code** code_out);
On 2010/11/10 14:53:54, antonm wrote:
> looks like these were the only stubs with custom caches, isn't it time to kill
> this stuff?

Done.

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:504: __ PrepareCallApiFunction(argc +
kFastApiCallArguments + 1,
On 2010/11/10 14:53:54, antonm wrote:
> not sure if it's worth it, but maybe unify exit frame construction and
arguments
> pushing?

Please, explain what do you mean by that?

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:509: __ mov(ApiParameterOperand(3), edx);  //
v8::Arguments::length_.
On 2010/11/10 14:53:54, antonm wrote:
> cannot we fuse those moves and moves above (ln. 490--492), for example why not
> drop __ Set(edx, Immediate(args)) @492 and fuse it with __
> mov(ApiParemeterOperand(3), Immediate(argc))?

Sure. I just forgot to do it.

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:521: MaybeObject* result =
masm->TryCallApiFunctionAndReturn(&fun, kApiArgc +
On 2010/11/10 14:53:54, antonm wrote:
> that might be just me, but I am slightly uneasy with this indentation: I do
> understand you want to emphasize the sum, but maybe do it differently:
> 
> MO* result =
>     masm->......?

Done.

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1079: // Insert additional parameters into the stack
frame above return address.
On 2010/11/10 14:53:54, antonm wrote:
> are we sure it is always safe to piggy back on outer frame?
> 
> can we at least make sure tests fail if this assumption is wrong?  I'd rather
> not to debug those issue in Chromium

Technically this is not outer frame, it's our own. The frame consists of:
arguments, a few special fields (return address, saved frame pointer etc) and
local variables. Usually arguments' part of the frame is constructed by caller
but here we reconstruct it.

This type of code exists at least from r3825 (ReserveSpaceForFastApiCall) and
apparently didn't cause a problems.

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1080: ASSERT(!scratch3.is(reg));
On 2010/11/10 14:53:54, antonm wrote:
> shouldn't you check as well that !scratch{2,3}.is(receiver)?

Caller code must give 5 different registers. If they are not different generated
code is wrong. Should I check each pair of registers? I don't think so. It is
different story with 'reg'. It's returned by CheckPrototypes and is scratch1 or
scratch2 but never scratch3. Here I rely on this aspect of CheckPrototypes
behaviour.

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1109: // to the values (it considered as smi in GC).
On 2010/11/10 14:53:54, antonm wrote:
> are you talking about ApiParameters below?  if yes, I used to think they are
not
> GCed?

stake space (the first parameter of PrepareCallApiFunction) means area above the
return address and is GCed. api argc (the second one) means area below the
return address and isn't GCed.

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1116: __ mov(ApiParameterOperand(1), eax);  //
arguments pointer.
On 2010/11/10 14:53:54, antonm wrote:
> I won't be surprised if __mov(APO(0), ebx); __ add(ebx, kPointerSize);
> __mov(APO(1), ebx); would be a shorter sequence with better performance.
> 
> Not insisting

Done.

http://codereview.chromium.org/4695003/diff/27001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/4695003/diff/27001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:506: __ movq(rcx, rsp);
On 2010/11/10 14:53:54, antonm wrote:
> should we pass a register to store rsp into as an argument?  or at least amend
> the documentation, please

Added comments.

[antonm, 2010-11-12 18:35:54]

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:504: __ PrepareCallApiFunction(argc +
kFastApiCallArguments + 1,
On 2010/11/11 15:50:39, SeRya wrote:
> On 2010/11/10 14:53:54, antonm wrote:
> > not sure if it's worth it, but maybe unify exit frame construction and
> arguments
> > pushing?
> 
> Please, explain what do you mean by that?

Sure and sorry for not making it clear.

Take a look
PrepareCallApiFunction->EnterApiExitFrame->EnterExitFrameEpoligue(int argc)

Here we explicitly reserve place on stack and then later fill it with
ApiParameterOperands.  I wonder if we could just push values to stack with plain
__ push and get rid of ApiParemeterOperands and make code more compact.

Of course, keeping the stack aligned might be a challenge, hence 'not sure if
it's worth it' :)

Does that make sense now?

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:509: __ mov(ApiParameterOperand(3), edx);  //
v8::Arguments::length_.
On 2010/11/11 15:50:39, SeRya wrote:
> On 2010/11/10 14:53:54, antonm wrote:
> > cannot we fuse those moves and moves above (ln. 490--492), for example why
not
> > drop __ Set(edx, Immediate(args)) @492 and fuse it with __
> > mov(ApiParemeterOperand(3), Immediate(argc))?
> 
> Sure. I just forgot to do it.

Is it the only opportunity?  For example we could probably nuke __ lea(ebx,
Operand(esp, (argc + 3) * kPointerSize)); as it could be calculated changed to
__ add(eax, argc * kPointerSize).

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1080: ASSERT(!scratch3.is(reg));
On 2010/11/11 15:50:39, SeRya wrote:
> On 2010/11/10 14:53:54, antonm wrote:
> > shouldn't you check as well that !scratch{2,3}.is(receiver)?
> 
> Caller code must give 5 different registers. If they are not different
generated
> code is wrong. Should I check each pair of registers? I don't think so. It is
> different story with 'reg'. It's returned by CheckPrototypes and is scratch1
or
> scratch2 but never scratch3. Here I rely on this aspect of CheckPrototypes
> behaviour.

I personally find this check somewhat misleading.  And, btw, reg should be
either receiver or scratch1.

http://codereview.chromium.org/4695003/diff/73001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/4695003/diff/73001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:509: // ecx must be used to pass the pointer to
the return value slot.
ecx -> rcx

[SeRya, 2010-11-13 09:19:49]

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:504: __ PrepareCallApiFunction(argc +
kFastApiCallArguments + 1,
On 2010/11/12 18:35:55, antonm wrote:
> On 2010/11/11 15:50:39, SeRya wrote:
> > On 2010/11/10 14:53:54, antonm wrote:
> > > not sure if it's worth it, but maybe unify exit frame construction and
> > arguments
> > > pushing?
> > 
> > Please, explain what do you mean by that?
> 
> Sure and sorry for not making it clear.
> 
> Take a look
> PrepareCallApiFunction->EnterApiExitFrame->EnterExitFrameEpoligue(int argc)
> 
> Here we explicitly reserve place on stack and then later fill it with
> ApiParameterOperands.  I wonder if we could just push values to stack with
plain
> __ push and get rid of ApiParemeterOperands and make code more compact.
> 
> Of course, keeping the stack aligned might be a challenge, hence 'not sure if
> it's worth it' :)
> 
> Does that make sense now?

Apparently push is shorter than mov and may be it worth to refactor this code.
But for now I'd prefer to to share usage pattern with PrepareCallCFunction.

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:509: __ mov(ApiParameterOperand(3), edx);  //
v8::Arguments::length_.
On 2010/11/12 18:35:55, antonm wrote:
> On 2010/11/11 15:50:39, SeRya wrote:
> > On 2010/11/10 14:53:54, antonm wrote:
> > > cannot we fuse those moves and moves above (ln. 490--492), for example why
> not
> > > drop __ Set(edx, Immediate(args)) @492 and fuse it with __
> > > mov(ApiParemeterOperand(3), Immediate(argc))?
> > 
> > Sure. I just forgot to do it.
> 
> Is it the only opportunity?  For example we could probably nuke __ lea(ebx,
> Operand(esp, (argc + 3) * kPointerSize)); as it could be calculated changed to
> __ add(eax, argc * kPointerSize).

Done.

http://codereview.chromium.org/4695003/diff/27001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1080: ASSERT(!scratch3.is(reg));
On 2010/11/12 18:35:55, antonm wrote:
> On 2010/11/11 15:50:39, SeRya wrote:
> > On 2010/11/10 14:53:54, antonm wrote:
> > > shouldn't you check as well that !scratch{2,3}.is(receiver)?
> > 
> > Caller code must give 5 different registers. If they are not different
> generated
> > code is wrong. Should I check each pair of registers? I don't think so. It
is
> > different story with 'reg'. It's returned by CheckPrototypes and is scratch1
> or
> > scratch2 but never scratch3. Here I rely on this aspect of CheckPrototypes
> > behaviour.
> 
> I personally find this check somewhat misleading.  And, btw, reg should be
> either receiver or scratch1.

This asserts what we are not trying to use a register which still contains a
value we need to push. Why it's misleading?

http://codereview.chromium.org/4695003/diff/73001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/4695003/diff/73001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:509: // ecx must be used to pass the pointer to
the return value slot.
On 2010/11/12 18:35:57, antonm wrote:
> ecx -> rcx

Done.

[antonm, 2010-11-13 11:34:45]

http://codereview.chromium.org/4695003/diff/82001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/4695003/diff/82001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:454: static bool GenerateFastApiCall(MacroAssembler*
masm,
as a further idea: probably this function should return MaybeObject instead of
using output Failure** argument

http://codereview.chromium.org/4695003/diff/82001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:489: // Prepare arguments for ApiCallEntryStub.
there is no ApiCallEntryStub any more, probably you should comment what gets
moved into eax

http://codereview.chromium.org/4695003/diff/82001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1099: __ mov(ebx, esp);
why we need this move?  please, add a comment

http://codereview.chromium.org/4695003/diff/82001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1101: __ push(scratch3);  // Restore return
addresss.
address<s> -> address

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:512: ASSERT(argc <= 6);  // EnterApiExitFrame
supports only register based args.
you don't have checks that revert to slow path if argc > 6, do you?

and is it completely impossible to handle argc > 6 here?

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.h:836: // Uses calle-saved esi to restore stack
state after call. In WIN64 puts
esi -> rsi
calle -> callee

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.h:836: // Uses calle-saved esi to restore stack
state after call. In WIN64 puts
"In WIN64 puts" doesn't sound fine imho.

WIN64 calling conventions require or something of that kind

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.h:838: // untill TryCallApiFunctionAndReturn).
untill -> until

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.h:839: void PrepareCallApiFunction(int stack_space,
int argc);
Please, add comments about semantics of stack_space and argc

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.h:844: MUST_USE_RESULT MaybeObject*
TryCallApiFunctionAndReturn(
I think you should unify doc with ia32 variant

http://codereview.chromium.org/4695003/diff/82001/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/4695003/diff/82001/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:2616: if (!name_arg.is(scratch2)) {
maybe just require that scratch2 is not rdi?  or swap scratch2 and scratch3
earlier

http://codereview.chromium.org/4695003/diff/82001/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:2618: __ push(scratch2);  // Restore return addresss.
addresss -> address

http://codereview.chromium.org/4695003/diff/82001/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:2620: __ push(scratch2);  // Restore return addresss.
ditto

http://codereview.chromium.org/4695003/diff/82001/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:2635: // If accessor_info_arg is rsi this is safe
point to set it up.
what do you mean by that?  may you be more explicit?

[SeRya, 2010-11-15 12:09:11]

http://codereview.chromium.org/4695003/diff/82001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/4695003/diff/82001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:489: // Prepare arguments for ApiCallEntryStub.
On 2010/11/13 11:34:46, antonm wrote:
> there is no ApiCallEntryStub any more, probably you should comment what gets
> moved into eax

Done.

http://codereview.chromium.org/4695003/diff/82001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1099: __ mov(ebx, esp);
On 2010/11/13 11:34:46, antonm wrote:
> why we need this move?  please, add a comment

Done.

http://codereview.chromium.org/4695003/diff/82001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:1101: __ push(scratch3);  // Restore return
addresss.
On 2010/11/13 11:34:46, antonm wrote:
> address<s> -> address

Done.

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:512: ASSERT(argc <= 6);  // EnterApiExitFrame
supports only register based args.
On 2010/11/13 11:34:46, antonm wrote:
> you don't have checks that revert to slow path if argc > 6, do you?
> 
> and is it completely impossible to handle argc > 6 here?

Actually I see the following issue. EnterExitFrameEpologue used in
EnterApiExitFrame designed to deal with v8::internal::Arguments parameters (it's
mentioned in its comments) but also suits for many other argument types.
EnterExitFrameEpologue could be modified to remove this limitation not now I'd
prefer to just add ASSERT for simplicity. Anyway,
MacroAssembler::EnterApiExitFrame is probaly better place for this assert.
Moved.

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.h:836: // Uses calle-saved esi to restore stack
state after call. In WIN64 puts
On 2010/11/13 11:34:46, antonm wrote:
> esi -> rsi
> calle -> callee

Done.

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.h:838: // untill TryCallApiFunctionAndReturn).
On 2010/11/13 11:34:46, antonm wrote:
> untill -> until

Done.

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.h:839: void PrepareCallApiFunction(int stack_space,
int argc);
On 2010/11/13 11:34:46, antonm wrote:
> Please, add comments about semantics of stack_space and argc

Done.

http://codereview.chromium.org/4695003/diff/82001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.h:844: MUST_USE_RESULT MaybeObject*
TryCallApiFunctionAndReturn(
On 2010/11/13 11:34:46, antonm wrote:
> I think you should unify doc with ia32 variant

Done.

http://codereview.chromium.org/4695003/diff/82001/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/4695003/diff/82001/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:2616: if (!name_arg.is(scratch2)) {
On 2010/11/13 11:34:46, antonm wrote:
> maybe just require that scratch2 is not rdi?  or swap scratch2 and scratch3
> earlier

Done.

http://codereview.chromium.org/4695003/diff/82001/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:2618: __ push(scratch2);  // Restore return addresss.
On 2010/11/13 11:34:46, antonm wrote:
> addresss -> address

Done.

http://codereview.chromium.org/4695003/diff/82001/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:2620: __ push(scratch2);  // Restore return addresss.
On 2010/11/13 11:34:46, antonm wrote:
> ditto

Done.

http://codereview.chromium.org/4695003/diff/82001/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:2635: // If accessor_info_arg is rsi this is safe
point to set it up.
On 2010/11/13 11:34:46, antonm wrote:
> what do you mean by that?  may you be more explicit?

Done.

[antonm, 2010-11-15 16:22:06]

That's almost LGTM.

My main concern is case > 6 arguments for non-Win x64.

http://codereview.chromium.org/4695003/diff/112001/src/code-stubs.cc
File src/code-stubs.cc (right):

http://codereview.chromium.org/4695003/diff/112001/src/code-stubs.cc#newcode148
src/code-stubs.cc:148: Heap::code_stubs()->AtNumberPut(GetKey(), code);
[cosmetic nit] just asking: can this fit one line now?

http://codereview.chromium.org/4695003/diff/112001/src/x64/macro-assembler-x6...
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/4695003/diff/112001/src/x64/macro-assembler-x6...
src/x64/macro-assembler-x64.cc:1800: ASSERT(argc <= 6);  // EnterApiExitFrame
supports only register based args.
Sorry, I still don't understand what.  What will happen if I write something
like:

for (var i = 0; i < 1000; i++) obj.appFunction(1, 2, 3, 4, 5, 6, 7, 8, 9)?

Will v8 just crash?

[SeRya, 2010-11-15 16:37:50]

http://codereview.chromium.org/4695003/diff/112001/src/code-stubs.cc
File src/code-stubs.cc (right):

http://codereview.chromium.org/4695003/diff/112001/src/code-stubs.cc#newcode148
src/code-stubs.cc:148: Heap::code_stubs()->AtNumberPut(GetKey(), code);
On 2010/11/15 16:22:06, antonm wrote:
> [cosmetic nit] just asking: can this fit one line now?

It can't (only if shorten the var name).

http://codereview.chromium.org/4695003/diff/112001/src/x64/macro-assembler-x6...
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/4695003/diff/112001/src/x64/macro-assembler-x6...
src/x64/macro-assembler-x64.cc:1800: ASSERT(argc <= 6);  // EnterApiExitFrame
supports only register based args.
On 2010/11/15 16:22:06, antonm wrote:
> Sorry, I still don't understand what.  What will happen if I write something
> like:
> 
> for (var i = 0; i < 1000; i++) obj.appFunction(1, 2, 3, 4, 5, 6, 7, 8, 9)?
> 
> Will v8 just crash?

Currently it's used only for getters for x64 (yet). But in case it was the
arguments would be: argc is 1 (because InvocationCallback accepts single
argument - reference to v8::Arguments). stack_space will be 13: receiver + 9 JS
arguments + 3-element array to represent v8::Arguments::implicit_values_.