From 6fbad6e230eac6726eaea8f47c5453dc1274e70d Mon Sep 17 00:00:00 2001
From: "vitalyr@chromium.org"
 <vitalyr@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Date: Thu, 25 Mar 2010 17:08:22 +0000
Subject: [PATCH] Simplify passing of AccessorInfo to interceptors:

 * Use slots on the native stack when possible instead of Relocatable.
 * Got rid of a gap in AccessorInfo fields.
 * Added test for non-cacheable post-interceptor lookup.

Review URL: http://codereview.chromium.org/1327002

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@4288 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
---
 include/v8.h                |  2 +-
 src/arguments.h             | 11 +++++------
 src/arm/stub-cache-arm.cc   |  7 +++----
 src/codegen.h               |  2 +-
 src/handles.cc              |  2 +-
 src/ia32/stub-cache-ia32.cc | 16 ++++++++--------
 src/stub-cache.cc           | 36 +++++++++++++++++++++---------------
 src/x64/stub-cache-x64.cc   |  7 +++----
 test/cctest/test-api.cc     | 32 ++++++++++++++++++++++++++++++++
 9 files changed, 75 insertions(+), 40 deletions(-)

diff --git a/include/v8.h b/include/v8.h
index 7b42178..f64b386 100644
--- a/include/v8.h
+++ b/include/v8.h
@@ -3361,7 +3361,7 @@ External* External::Cast(v8::Value* value) {
 
 
 Local<Value> AccessorInfo::Data() const {
-  return Local<Value>(reinterpret_cast<Value*>(&args_[-3]));
+  return Local<Value>(reinterpret_cast<Value*>(&args_[-2]));
 }
 
 
diff --git a/src/arguments.h b/src/arguments.h
index 3fed223..c17f4cf 100644
--- a/src/arguments.h
+++ b/src/arguments.h
@@ -72,7 +72,7 @@ class Arguments BASE_EMBEDDED {
 };
 
 
-// Cursom arguments replicate a small segment of stack that can be
+// Custom arguments replicate a small segment of stack that can be
 // accessed through an Arguments object the same way the actual stack
 // can.
 class CustomArguments : public Relocatable {
@@ -80,15 +80,14 @@ class CustomArguments : public Relocatable {
   inline CustomArguments(Object* data,
                          JSObject* self,
                          JSObject* holder) {
-    values_[3] = self;
-    values_[2] = holder;
-    values_[1] = Smi::FromInt(0);
+    values_[2] = self;
+    values_[1] = holder;
     values_[0] = data;
   }
   void IterateInstance(ObjectVisitor* v);
-  Object** end() { return values_ + 3; }
+  Object** end() { return values_ + ARRAY_SIZE(values_) - 1; }
  private:
-  Object* values_[4];
+  Object* values_[3];
 };
 
 
diff --git a/src/arm/stub-cache-arm.cc b/src/arm/stub-cache-arm.cc
index abf2f64..bbffef22 100644
--- a/src/arm/stub-cache-arm.cc
+++ b/src/arm/stub-cache-arm.cc
@@ -396,15 +396,14 @@ static void PushInterceptorArguments(MacroAssembler* masm,
                                      Register holder,
                                      Register name,
                                      JSObject* holder_obj) {
-  __ push(receiver);
-  __ push(holder);
   __ push(name);
   InterceptorInfo* interceptor = holder_obj->GetNamedInterceptor();
   ASSERT(!Heap::InNewSpace(interceptor));
-
-  Register scratch = receiver;
+  Register scratch = name;
   __ mov(scratch, Operand(Handle<Object>(interceptor)));
   __ push(scratch);
+  __ push(receiver);
+  __ push(holder);
   __ ldr(scratch, FieldMemOperand(scratch, InterceptorInfo::kDataOffset));
   __ push(scratch);
 }
diff --git a/src/codegen.h b/src/codegen.h
index 4634f4c..0dfea8d 100644
--- a/src/codegen.h
+++ b/src/codegen.h
@@ -450,7 +450,7 @@ class ApiGetterEntryStub : public CodeStub {
   virtual bool GetCustomCache(Code** code_out);
   virtual void SetCustomCache(Code* value);
 
-  static const int kStackSpace = 6;
+  static const int kStackSpace = 5;
   static const int kArgc = 4;
  private:
   Handle<AccessorInfo> info() { return info_; }
diff --git a/src/handles.cc b/src/handles.cc
index f8a679b..d4c593f 100644
--- a/src/handles.cc
+++ b/src/handles.cc
@@ -541,7 +541,7 @@ int GetScriptLineNumberSafe(Handle<Script> script, int code_pos) {
 
 
 void CustomArguments::IterateInstance(ObjectVisitor* v) {
-  v->VisitPointers(values_, values_ + 4);
+  v->VisitPointers(values_, values_ + ARRAY_SIZE(values_));
 }
 
 
diff --git a/src/ia32/stub-cache-ia32.cc b/src/ia32/stub-cache-ia32.cc
index 858d9ec..315600b 100644
--- a/src/ia32/stub-cache-ia32.cc
+++ b/src/ia32/stub-cache-ia32.cc
@@ -276,14 +276,15 @@ static void PushInterceptorArguments(MacroAssembler* masm,
                                      Register holder,
                                      Register name,
                                      JSObject* holder_obj) {
-  __ push(receiver);
-  __ push(holder);
   __ push(name);
   InterceptorInfo* interceptor = holder_obj->GetNamedInterceptor();
   ASSERT(!Heap::InNewSpace(interceptor));
-  __ mov(receiver, Immediate(Handle<Object>(interceptor)));
+  Register scratch = name;
+  __ mov(scratch, Immediate(Handle<Object>(interceptor)));
+  __ push(scratch);
   __ push(receiver);
-  __ push(FieldOperand(receiver, InterceptorInfo::kDataOffset));
+  __ push(holder);
+  __ push(FieldOperand(scratch, InterceptorInfo::kDataOffset));
 }
 
 
@@ -1045,17 +1046,16 @@ bool StubCompiler::GenerateLoadCallback(JSObject* object,
   __ push(receiver);  // receiver
   __ push(reg);  // holder
   __ mov(other, Immediate(callback_handle));
-  __ push(other);
   __ push(FieldOperand(other, AccessorInfo::kDataOffset));  // data
   __ push(name_reg);  // name
   // Save a pointer to where we pushed the arguments pointer.
-  // This will be passed as the const Arguments& to the C++ callback.
+  // This will be passed as the const AccessorInfo& to the C++ callback.
   __ mov(eax, esp);
-  __ add(Operand(eax), Immediate(5 * kPointerSize));
+  __ add(Operand(eax), Immediate(4 * kPointerSize));
   __ mov(ebx, esp);
 
   // Do call through the api.
-  ASSERT_EQ(6, ApiGetterEntryStub::kStackSpace);
+  ASSERT_EQ(5, ApiGetterEntryStub::kStackSpace);
   Address getter_address = v8::ToCData<Address>(callback->getter());
   ApiFunction fun(getter_address);
   ApiGetterEntryStub stub(callback_handle, &fun);
diff --git a/src/stub-cache.cc b/src/stub-cache.cc
index c942d9d..ce587bc 100644
--- a/src/stub-cache.cc
+++ b/src/stub-cache.cc
@@ -782,6 +782,10 @@ Object* StoreCallbackProperty(Arguments args) {
   return *value;
 }
 
+
+static const int kAccessorInfoOffsetInInterceptorArgs = 2;
+
+
 /**
  * Attempts to load a property with an interceptor (which must be present),
  * but doesn't search the prototype chain.
@@ -790,11 +794,12 @@ Object* StoreCallbackProperty(Arguments args) {
  * provide any value for the given name.
  */
 Object* LoadPropertyWithInterceptorOnly(Arguments args) {
-  JSObject* receiver_handle = JSObject::cast(args[0]);
-  JSObject* holder_handle = JSObject::cast(args[1]);
-  Handle<String> name_handle = args.at<String>(2);
-  Handle<InterceptorInfo> interceptor_info = args.at<InterceptorInfo>(3);
-  Object* data_handle = args[4];
+  Handle<String> name_handle = args.at<String>(0);
+  Handle<InterceptorInfo> interceptor_info = args.at<InterceptorInfo>(1);
+  ASSERT(kAccessorInfoOffsetInInterceptorArgs == 2);
+  ASSERT(args[2]->IsJSObject());  // Receiver.
+  ASSERT(args[3]->IsJSObject());  // Holder.
+  ASSERT(args.length() == 5);  // Last arg is data object.
 
   Address getter_address = v8::ToCData<Address>(interceptor_info->getter());
   v8::NamedPropertyGetter getter =
@@ -803,8 +808,8 @@ Object* LoadPropertyWithInterceptorOnly(Arguments args) {
 
   {
     // Use the interceptor getter.
-    CustomArguments args(data_handle, receiver_handle, holder_handle);
-    v8::AccessorInfo info(args.end());
+    v8::AccessorInfo info(args.arguments() -
+                          kAccessorInfoOffsetInInterceptorArgs);
     HandleScope scope;
     v8::Handle<v8::Value> r;
     {
@@ -842,11 +847,12 @@ static Object* ThrowReferenceError(String* name) {
 
 static Object* LoadWithInterceptor(Arguments* args,
                                    PropertyAttributes* attrs) {
-  Handle<JSObject> receiver_handle = args->at<JSObject>(0);
-  Handle<JSObject> holder_handle = args->at<JSObject>(1);
-  Handle<String> name_handle = args->at<String>(2);
-  Handle<InterceptorInfo> interceptor_info = args->at<InterceptorInfo>(3);
-  Handle<Object> data_handle = args->at<Object>(4);
+  Handle<String> name_handle = args->at<String>(0);
+  Handle<InterceptorInfo> interceptor_info = args->at<InterceptorInfo>(1);
+  ASSERT(kAccessorInfoOffsetInInterceptorArgs == 2);
+  Handle<JSObject> receiver_handle = args->at<JSObject>(2);
+  Handle<JSObject> holder_handle = args->at<JSObject>(3);
+  ASSERT(args->length() == 5);  // Last arg is data object.
 
   Address getter_address = v8::ToCData<Address>(interceptor_info->getter());
   v8::NamedPropertyGetter getter =
@@ -855,8 +861,8 @@ static Object* LoadWithInterceptor(Arguments* args,
 
   {
     // Use the interceptor getter.
-    CustomArguments args(*data_handle, *receiver_handle, *holder_handle);
-    v8::AccessorInfo info(args.end());
+    v8::AccessorInfo info(args->arguments() -
+                          kAccessorInfoOffsetInInterceptorArgs);
     HandleScope scope;
     v8::Handle<v8::Value> r;
     {
@@ -891,7 +897,7 @@ Object* LoadPropertyWithInterceptorForLoad(Arguments args) {
 
   // If the property is present, return it.
   if (attr != ABSENT) return result;
-  return ThrowReferenceError(String::cast(args[2]));
+  return ThrowReferenceError(String::cast(args[0]));
 }
 
 
diff --git a/src/x64/stub-cache-x64.cc b/src/x64/stub-cache-x64.cc
index aa620fc..03b21a5 100644
--- a/src/x64/stub-cache-x64.cc
+++ b/src/x64/stub-cache-x64.cc
@@ -138,14 +138,13 @@ static void PushInterceptorArguments(MacroAssembler* masm,
                                      Register holder,
                                      Register name,
                                      JSObject* holder_obj) {
-  __ push(receiver);
-  __ push(holder);
   __ push(name);
   InterceptorInfo* interceptor = holder_obj->GetNamedInterceptor();
   ASSERT(!Heap::InNewSpace(interceptor));
-  __ movq(kScratchRegister, Handle<Object>(interceptor),
-          RelocInfo::EMBEDDED_OBJECT);
+  __ Move(kScratchRegister, Handle<Object>(interceptor));
   __ push(kScratchRegister);
+  __ push(receiver);
+  __ push(holder);
   __ push(FieldOperand(kScratchRegister, InterceptorInfo::kDataOffset));
 }
 
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index e579143..d9b5d49 100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -5974,6 +5974,38 @@ THREADED_TEST(InterceptorLoadICInvalidatedField) {
 }
 
 
+static int interceptor_load_not_handled_calls = 0;
+static v8::Handle<Value> InterceptorLoadNotHandled(Local<String> name,
+                                                   const AccessorInfo& info) {
+  ++interceptor_load_not_handled_calls;
+  return v8::Handle<v8::Value>();
+}
+
+
+// Test how post-interceptor lookups are done in the non-cacheable
+// case: the interceptor should not be invoked during this lookup.
+THREADED_TEST(InterceptorLoadICPostInterceptor) {
+  interceptor_load_not_handled_calls = 0;
+  CheckInterceptorLoadIC(InterceptorLoadNotHandled,
+    "receiver = new Object();"
+    "receiver.__proto__ = o;"
+    "proto = new Object();"
+    "/* Make proto a slow-case object. */"
+    "for (var i = 0; i < 1000; i++) {"
+    "  proto[\"xxxxxxxx\" + i] = [];"
+    "}"
+    "proto.x = 17;"
+    "o.__proto__ = proto;"
+    "var result = 0;"
+    "for (var i = 0; i < 1000; i++) {"
+    "  result += receiver.x;"
+    "}"
+    "result;",
+    17 * 1000);
+  CHECK_EQ(1000, interceptor_load_not_handled_calls);
+}
+
+
 // Test the case when we stored field into
 // a stub, but it got invalidated later on due to override on
 // global object which is between interceptor and fields' holders.
-- 
2.7.4