Stub Cache: speed up load callback accessor by allocating data handle on stack.

src/stub-cache.cc

Index: src/stub-cache.cc
diff --git a/src/stub-cache.cc b/src/stub-cache.cc
index 7ca2677d58bba1c9f7b002f7203d5b4d560152db..859514885af73bc90352a1164a6c6b853ffd8248 100644
--- a/src/stub-cache.cc
+++ b/src/stub-cache.cc
@@ -736,22 +736,22 @@ Handle<Code> ComputeCallMiss(int argc) {
 
 Object* LoadCallbackProperty(Arguments args) {
   Handle<JSObject> recv = args.at<JSObject>(0);
-  AccessorInfo* callback = AccessorInfo::cast(args[1]);
+  Handle<JSObject> holder = args.at<JSObject>(1);
+  AccessorInfo* callback = AccessorInfo::cast(args[2]);
+  Handle<Object> data = args.at<Object>(3);

[Christian Plesner Hansen, 2009/07/24 07:33:25]

A further optimization could be to change the implementation of AccessorInfo to
hold a pointer directly into the stack and then extract the values on access
rather than up front.  That would also make things easier if we want to call the
function directly from generated code.

I'm not suggesting doing it as part of this CL but that's something we might
consider later.

[Vitaly Repeshko, 2009/07/24 13:45:38]

Sure. After this patch is in, I was going to experiment with what is described
in the NOTE comment below.

   Address getter_address = v8::ToCData<Address>(callback->getter());

[antonm, 2009/07/23 21:12:58]

maybe compile getter_address into stub as well?  As you told me on ia32 it
should be probably safe as the address would be aligned, do you know if it's
still the case for ARM?  And x64?

[Vitaly Repeshko, 2009/07/24 13:45:38]

I think this essentially means moving a few instructions from here to the stub
cache code. So I don't see how it makes a difference at this point.

[Vitaly Repeshko, 2009/07/24 15:43:56]

Anton pointed out in offline discussion that we store the address indirectly
using a Proxy object. Unfortunately, putting it in the stub code directly
besides requiring extra care with respect to alignment doesn't make any
difference according to my measurements.

   v8::AccessorGetter fun = FUNCTION_CAST<v8::AccessorGetter>(getter_address);
   ASSERT(fun != NULL);
-  Handle<String> name = args.at<String>(2);
-  Handle<JSObject> holder = args.at<JSObject>(3);
-  HandleScope scope;
-  Handle<Object> data(callback->data());
-  LOG(ApiNamedPropertyAccess("load", *recv, *name));
+  Handle<String> name = args.at<String>(4);
   // NOTE: If we can align the structure of an AccessorInfo with the
   // locations of the arguments to this function maybe we don't have
   // to explicitly create the structure but can just pass a pointer
   // into the stack.
+  LOG(ApiNamedPropertyAccess("load", *recv, *name));

[iposva, 2009/07/24 00:30:07]

Do you really intend to log for every single property access with a callback? I
know that code was there before, but maybe it is time to evaluate if we ever are
looking at this log output?

[Christian Plesner Hansen, 2009/07/24 07:33:25]

I agree.  It looks like we're already losing performance because of api logging
and I don't believe anyone is using it.

[Vitaly Repeshko, 2009/07/24 13:45:38]

Well, this does cost us a bit, but I'm not sure I should just remove logging.
What about defining something like LOG_VERBOSE macro (for performance-critical
places) that gets compiled to nothing without a special define? I could do this
for all similar places.

   v8::AccessorInfo info(v8::Utils::ToLocal(recv),
                         v8::Utils::ToLocal(data),
                         v8::Utils::ToLocal(holder));
+  HandleScope scope;

[antonm, 2009/07/23 21:12:58]

maybe move scope just before entering fun?  for example, to reduce its scope
(sorry) and make it easier for maintainer to understand that scope is only
needed for fun?

[iposva, 2009/07/24 00:30:07]

And where would the result handle on the next line be allocated?

[antonm, 2009/07/24 02:43:21]

Correct me if I'm wrong, but v8::Handle::Handle() doesn't require scope.

[Christian Plesner Hansen, 2009/07/24 07:33:25]

If you move the handle scope into the inner scope the value assigned to 'result'
will become invalid as soon as you leave the inner scope.

[antonm, 2009/07/24 08:59:34]

Aha, stupid me.

   v8::Handle<v8::Value> result;
   {
     // Leaving JavaScript.