Removing redundant stubs for API functions.

src/ia32/stub-cache-ia32.cc

Index: src/ia32/stub-cache-ia32.cc
===================================================================
--- src/ia32/stub-cache-ia32.cc	(revision 5791)
+++ src/ia32/stub-cache-ia32.cc	(working copy)
@@ -495,22 +495,35 @@
   Address api_function_address = v8::ToCData<Address>(callback);
   ApiFunction fun(api_function_address);
 
-  ApiCallEntryStub stub(api_call_info_handle, &fun);
+  const int kApiArgc = 1;  // API function gets reference to the v8::Arguments.
 
-  __ EnterInternalFrame();
+  // Allocate the v8::Arguments structure in the arguments' space since
+  // it's not controlled by GC.
+  const int kApiStackSpace = 4;
 
+  __ PrepareCallApiFunction(argc + kFastApiCallArguments + 1,

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

not sure if it's worth it, but maybe unify exit frame construction and arguments
pushing?

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

Please, explain what do you mean by that?

[antonm, 2010/11/12 18:35:55]

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?

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

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.

+                            kApiArgc + kApiStackSpace);
+
+  __ mov(ApiParameterOperand(1), eax);  // v8::Arguments::implicit_args_.
+  __ mov(ApiParameterOperand(2), ebx);  // v8::Arguments::values_.
+  __ mov(ApiParameterOperand(3), edx);  // v8::Arguments::length_.

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

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))?

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

Sure. I just forgot to do it.

[antonm, 2010/11/12 18:35:55]

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).

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

Done.

+  // v8::Arguments::is_construct_call_.
+  __ mov(ApiParameterOperand(4), Immediate(0));
+
+  // v8::InvocationCallback's argument.
+  __ lea(eax, ApiParameterOperand(1));
+  __ mov(ApiParameterOperand(0), eax);
+
   // Emitting a stub call may try to allocate (if the code is not
   // already generated).  Do not allow the assembler to perform a
   // garbage collection but instead return the allocation failure
   // object.
-  MaybeObject* result = masm->TryCallStub(&stub);
+  MaybeObject* result = masm->TryCallApiFunctionAndReturn(&fun, kApiArgc +

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

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->......?

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

Done.

+                                                                kApiStackSpace);
   if (result->IsFailure()) {
     *failure = Failure::cast(result);
     return false;
   }
-
-  __ LeaveInternalFrame();
-  __ ret((argc + 4) * kPointerSize);
   return true;
 }
 
@@ -1063,44 +1076,55 @@
 
   Handle<AccessorInfo> callback_handle(callback);
 
-  __ EnterInternalFrame();
-  // Push the stack address where the list of arguments ends.
-  __ lea(scratch2, Operand(esp, -2 * kPointerSize));
-  __ push(scratch2);
+  // Insert additional parameters into the stack frame above return address.

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

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

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

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.

+  ASSERT(!scratch3.is(reg));

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

shouldn't you check as well that !scratch{2,3}.is(receiver)?

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

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.

[antonm, 2010/11/12 18:35:55]

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

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

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

+  __ pop(scratch3);  // Get return address to place it below.
+
   __ push(receiver);  // receiver
+  __ mov(scratch2, Operand(esp));
+  ASSERT(!scratch2.is(reg));
   __ push(reg);  // holder
   // Push data from AccessorInfo.
   if (Heap::InNewSpace(callback_handle->data())) {
-    __ mov(scratch2, Immediate(callback_handle));
-    __ push(FieldOperand(scratch2, AccessorInfo::kDataOffset));
+    __ mov(scratch1, Immediate(callback_handle));
+    __ push(FieldOperand(scratch1, AccessorInfo::kDataOffset));
   } else {
     __ push(Immediate(Handle<Object>(callback_handle->data())));
   }
+    // Push the stack address where the list of arguments ends.
+  __ push(scratch2);
+
   __ push(name_reg);  // name
   // Save a pointer to where we pushed the arguments pointer.
   // This will be passed as the const AccessorInfo& to the C++ callback.
-  STATIC_ASSERT(ApiGetterEntryStub::kStackSpace == 5);
-  __ lea(eax, Operand(esp, 4 * kPointerSize));
   __ mov(ebx, esp);
 
+  __ push(scratch3);  // Restore return addresss.
+
   // Do call through the api.
   Address getter_address = v8::ToCData<Address>(callback->getter());
   ApiFunction fun(getter_address);
-  ApiGetterEntryStub stub(callback_handle, &fun);
+
+  // 3 elements array for v8::Agruments::values_, handler for name and pointer
+  // to the values (it considered as smi in GC).

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

are you talking about ApiParameters below?  if yes, I used to think they are not
GCed?

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

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.

+  const int kStackSpace = 5;
+  const int kApiArgc = 2;
+
+  __ PrepareCallApiFunction(kStackSpace, kApiArgc);

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

again, couldn't we merge pushing and exit frame setup?

+  __ lea(eax, Operand(ebx, 1 * kPointerSize));
+  __ mov(ApiParameterOperand(0), ebx);  // name.
+  __ mov(ApiParameterOperand(1), eax);  // arguments pointer.

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

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

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

Done.

+
   // Emitting a stub call may try to allocate (if the code is not
   // already generated).  Do not allow the assembler to perform a
   // garbage collection but instead return the allocation failure
   // object.
-  Object* result = NULL;  // Initialization to please compiler.
-  { MaybeObject* try_call_result = masm()->TryCallStub(&stub);
-    if (!try_call_result->ToObject(&result)) {
-      *failure = Failure::cast(try_call_result);
-      return false;
-    }
+  MaybeObject* result = masm()->TryCallApiFunctionAndReturn(&fun, kApiArgc);
+  if (result->IsFailure()) {
+    *failure = Failure::cast(result);
+    return false;
   }
-  __ LeaveInternalFrame();
 
-  __ ret(0);
   return true;
 }