Inline array loads in loops directly in the code instead of always

calling a stub.  The map to check against is unknown when generating
the code, so we patch the map check in the IC initialization code.

Loop nesting is currently not tracked on ARM.  I'll file feature
request bug reports for implementing this on ARM and add the number to
the TODOs before I commit.
Review URL: http://codereview.chromium.org/16409

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1015 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/builtins-ia32.cc b/src/builtins-ia32.cc
index 807a4bb..d773c97 100644 (file)
--- a/src/builtins-ia32.cc
+++ b/src/builtins-ia32.cc
@@ -615,6 +615,10 @@ void Builtins::Generate_FunctionApply(MacroAssembler* masm) {
   // Use inline caching to speed up access to arguments.
   Handle<Code> ic(Builtins::builtin(Builtins::KeyedLoadIC_Initialize));
   __ call(ic, RelocInfo::CODE_TARGET);
+  // It is important that we do not have a test instruction after the
+  // call.  A test instruction after the call is used to indicate that
+  // we have generated an inline version of the keyed load.  In this
+  // case, we know that we are not generating a test instruction next.
 
   // Remove IC arguments from the stack and push the nth argument.
   __ add(Operand(esp), Immediate(2 * kPointerSize));

diff --git a/src/codegen-arm.cc b/src/codegen-arm.cc
index d95a6ad..3453d52 100644 (file)
--- a/src/codegen-arm.cc
+++ b/src/codegen-arm.cc
@@ -3282,6 +3282,9 @@ void Reference::GetValue(TypeofState typeof_state) {
     case KEYED: {
       // TODO(1241834): Make sure that this it is safe to ignore the
       // distinction between expressions in a typeof and not in a typeof.
+
+      // TODO(181): Implement inlined version of array indexing once
+      // loop nesting is properly tracked on ARM.
       Comment cmnt(masm, "[ Load from keyed Property");
       ASSERT(property != NULL);
       Handle<Code> ic(Builtins::builtin(Builtins::KeyedLoadIC_Initialize));

diff --git a/src/codegen-ia32.cc b/src/codegen-ia32.cc
index 4ade882..0cc2889 100644 (file)
--- a/src/codegen-ia32.cc
+++ b/src/codegen-ia32.cc
@@ -3778,6 +3778,40 @@ void CodeGenerator::VisitCompareOperation(CompareOperation* node) {
 }
 
 
+class DeferredReferenceGetKeyedValue: public DeferredCode {
+ public:
+  DeferredReferenceGetKeyedValue(CodeGenerator* generator, bool is_global)
+      : DeferredCode(generator), is_global_(is_global) {
+    set_comment("[ DeferredReferenceGetKeyedValue");
+  }
+
+  virtual void Generate() {
+    Handle<Code> ic(Builtins::builtin(Builtins::KeyedLoadIC_Initialize));
+    // Calculate the delta from the IC call instruction to the map
+    // check cmp instruction in the inlined version.  This delta is
+    // stored in a test(eax, delta) instruction after the call so that
+    // we can find it in the IC initialization code and patch the cmp
+    // instruction.  This means that we cannot allow test instructions
+    // after calls to KeyedLoadIC stubs in other places.
+    int delta_to_patch_site = __ SizeOfCodeGeneratedSince(patch_site());
+    if (is_global_) {
+      __ call(ic, RelocInfo::CODE_TARGET_CONTEXT);
+    } else {
+      __ call(ic, RelocInfo::CODE_TARGET);
+    }
+    __ test(eax, Immediate(-delta_to_patch_site));
+    __ IncrementCounter(&Counters::keyed_load_inline_miss, 1);
+  }
+
+  Label* patch_site() { return &patch_site_; }
+
+ private:
+  Label patch_site_;
+  bool is_global_;
+};
+
+
+
 #undef __
 #define __ masm->
 
@@ -3813,42 +3847,95 @@ void Reference::GetValue(TypeofState typeof_state) {
     }
 
     case NAMED: {
-      // TODO(1241834): Make sure that this it is safe to ignore the
-      // distinction between expressions in a typeof and not in a typeof. If
-      // there is a chance that reference errors can be thrown below, we
-      // must distinguish between the two kinds of loads (typeof expression
-      // loads must not throw a reference error).
+      // TODO(1241834): Make sure that it is safe to ignore the
+      // distinction between expressions in a typeof and not in a
+      // typeof. If there is a chance that reference errors can be
+      // thrown below, we must distinguish between the two kinds of
+      // loads (typeof expression loads must not throw a reference
+      // error).
       Comment cmnt(masm, "[ Load from named Property");
       Handle<String> name(GetName());
+      Variable* var = expression_->AsVariableProxy()->AsVariable();
       Handle<Code> ic(Builtins::builtin(Builtins::LoadIC_Initialize));
       // Setup the name register.
       __ mov(ecx, name);
-
-      Variable* var = expression_->AsVariableProxy()->AsVariable();
       if (var != NULL) {
         ASSERT(var->is_global());
         __ call(ic, RelocInfo::CODE_TARGET_CONTEXT);
       } else {
         __ call(ic, RelocInfo::CODE_TARGET);
       }
-      frame->Push(eax);  // IC call leaves result in eax, push it out
+      // Push the result.
+      frame->Push(eax);
       break;
     }
 
     case KEYED: {
-      // TODO(1241834): Make sure that this it is safe to ignore the
-      // distinction between expressions in a typeof and not in a typeof.
-      Comment cmnt(masm, "[ Load from keyed Property");
-      Handle<Code> ic(Builtins::builtin(Builtins::KeyedLoadIC_Initialize));
-
+      // TODO(1241834): Make sure that it is safe to ignore the
+      // distinction between expressions in a typeof and not in a
+      // typeof.
       Variable* var = expression_->AsVariableProxy()->AsVariable();
-      if (var != NULL) {
-        ASSERT(var->is_global());
-        __ call(ic, RelocInfo::CODE_TARGET_CONTEXT);
+      // Inline array load code if inside of a loop.  We do not know
+      // the receiver map yet, so we initially generate the code with
+      // a check against an invalid map.  In the inline cache code, we
+      // patch the map check if appropriate.
+      if (cgen_->loop_nesting() > 0) {
+        Comment cmnt(masm, "[ Inlined array index load");
+        DeferredReferenceGetKeyedValue* deferred =
+            new DeferredReferenceGetKeyedValue(cgen_, var != NULL);
+        // Load receiver and check that it is not a smi (only needed
+        // if not contextual) and that it has the expected map.
+        __ mov(edx, Operand(esp, kPointerSize));
+        if (var == NULL) {
+          __ test(edx, Immediate(kSmiTagMask));
+          __ j(zero, deferred->enter(), not_taken);
+        }
+        // Initially, use an invalid map. The map is patched in the IC
+        // initialization code.
+        __ bind(deferred->patch_site());
+        __ cmp(FieldOperand(edx, HeapObject::kMapOffset),
+               Immediate(Factory::null_value()));
+        __ j(not_equal, deferred->enter(), not_taken);
+        // Load key and check that it is a smi.
+        __ mov(eax, Operand(esp, 0));
+        __ test(eax, Immediate(kSmiTagMask));
+        __ j(not_zero, deferred->enter(), not_taken);
+        // Shift to get actual index value.
+        __ sar(eax, kSmiTagSize);
+        // Get the elements array from the receiver and check that it
+        // is not a dictionary.
+        __ mov(edx, FieldOperand(edx, JSObject::kElementsOffset));
+        __ cmp(FieldOperand(edx, HeapObject::kMapOffset),
+               Immediate(Factory::hash_table_map()));
+        __ j(equal, deferred->enter(), not_taken);
+        // Check that key is within bounds.
+        __ cmp(eax, FieldOperand(edx, Array::kLengthOffset));
+        __ j(above_equal, deferred->enter(), not_taken);
+        // Load and check that the result is not the hole.
+        __ mov(eax,
+               Operand(edx, eax, times_4, Array::kHeaderSize - kHeapObjectTag));
+        __ cmp(Operand(eax), Immediate(Factory::the_hole_value()));
+        __ j(equal, deferred->enter(), not_taken);
+        __ IncrementCounter(&Counters::keyed_load_inline, 1);
+        __ bind(deferred->exit());
       } else {
-        __ call(ic, RelocInfo::CODE_TARGET);
+        Comment cmnt(masm, "[ Load from keyed Property");
+        Handle<Code> ic(Builtins::builtin(Builtins::KeyedLoadIC_Initialize));
+        if (var != NULL) {
+          ASSERT(var->is_global());
+          __ call(ic, RelocInfo::CODE_TARGET_CONTEXT);
+        } else {
+          __ call(ic, RelocInfo::CODE_TARGET);
+        }
+        // Make sure that we do not have a test instruction after the
+        // call.  A test instruction after the call is used to
+        // indicate that we have generated an inline version of the
+        // keyed load.  The explicit nop instruction is here because
+        // the push that follows might be peep-hole optimized away.
+        __ nop();
       }
-      frame->Push(eax);  // IC call leaves result in eax, push it out
+      // Push the result.
+      frame->Push(eax);
       break;
     }
 

diff --git a/src/d8.cc b/src/d8.cc
index 9e392f6..9f277bf 100644 (file)
--- a/src/d8.cc
+++ b/src/d8.cc
@@ -367,16 +367,16 @@ void Shell::Initialize() {
 
 void Shell::OnExit() {
   if (i::FLAG_dump_counters) {
-    ::printf("+----------------------------------------+----------+\n");
-    ::printf("| Name                                   | Value    |\n");
-    ::printf("+----------------------------------------+----------+\n");
+    ::printf("+----------------------------------------+-------------+\n");
+    ::printf("| Name                                   | Value       |\n");
+    ::printf("+----------------------------------------+-------------+\n");
     for (CounterMap::iterator i = counter_map_.begin();
          i != counter_map_.end();
          i++) {
       Counter* counter = (*i).second;
-      ::printf("| %-38s | %8i |\n", (*i).first, counter->value());
+      ::printf("| %-38s | %11i |\n", (*i).first, counter->value());
     }
-    ::printf("+----------------------------------------+----------+\n");
+    ::printf("+----------------------------------------+-------------+\n");
   }
   if (counters_file_ != NULL)
     delete counters_file_;

diff --git a/src/ic-arm.cc b/src/ic-arm.cc
index 55bbee4..4ccefa1 100644 (file)
--- a/src/ic-arm.cc
+++ b/src/ic-arm.cc
@@ -502,6 +502,12 @@ void LoadIC::Generate(MacroAssembler* masm, const ExternalReference& f) {
 }
 
 
+// TODO(181): Implement map patching once loop nesting is tracked on
+// the ARM platform so we can generate inlined fast-case code for
+// array indexing in loops.
+void KeyedLoadIC::PatchInlinedMapCheck(Address address, Object* value) { }
+
+
 Object* KeyedLoadIC_Miss(Arguments args);
 
 

diff --git a/src/ic-ia32.cc b/src/ic-ia32.cc
index 6021004..5603077 100644 (file)
--- a/src/ic-ia32.cc
+++ b/src/ic-ia32.cc
@@ -733,6 +733,27 @@ void LoadIC::Generate(MacroAssembler* masm, const ExternalReference& f) {
 }
 
 
+void KeyedLoadIC::PatchInlinedMapCheck(Address address, Object* value) {
+  static const byte kTestEaxByte = 0xA9;
+  Address test_instruction_address = address + 4;  // 4 = stub address
+  // The keyed load has a fast inlined case if the IC call instruction
+  // is immediately followed by a test instruction.
+  if (*test_instruction_address == kTestEaxByte) {
+    // Fetch the offset from the call instruction to the map cmp
+    // instruction.  This offset is stored in the last 4 bytes of the
+    // 5 byte test instruction.
+    Address offset_address = test_instruction_address + 1;
+    int offset_value = *(reinterpret_cast<int*>(offset_address));
+    // Compute the map address.  The operand-immediate compare
+    // instruction is two bytes larger than a call instruction so we
+    // add 2 to get to the map address.
+    Address map_address = address + offset_value + 2;
+    // patch the map check.
+    (*(reinterpret_cast<Object**>(map_address))) = value;
+  }
+}
+
+
 // Defined in ic.cc.
 Object* KeyedLoadIC_Miss(Arguments args);
 

diff --git a/src/ic.cc b/src/ic.cc
index 4b2e084..b28d6f6 100644 (file)
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -233,6 +233,10 @@ void CallIC::Clear(Address address, Code* target) {
 
 void KeyedLoadIC::Clear(Address address, Code* target) {
   if (target->ic_state() == UNINITIALIZED) return;
+  // Make sure to also clear the map used in inline fast cases.  If we
+  // do not clear these maps, cached code can keep objects alive
+  // through the embedded maps.
+  PatchInlinedMapCheck(address, Heap::null_value());
   SetTargetAtAddress(address, initialize_stub());
 }
 
@@ -718,7 +722,18 @@ Object* KeyedLoadIC::Load(State state,
   // the global object).
   bool use_ic = FLAG_use_ic && !object->IsAccessCheckNeeded();
 
-  if (use_ic) set_target(generic_stub());
+  if (use_ic) {
+    set_target(generic_stub());
+    // For JSObjects that are not value wrappers and that do not have
+    // indexed interceptors, we initialize the inlined fast case (if
+    // present) by patching the inlined map check.
+    if (object->IsJSObject() &&
+        !object->IsJSValue() &&
+        !JSObject::cast(*object)->HasIndexedInterceptor()) {
+      Map* map = JSObject::cast(*object)->map();
+      PatchInlinedMapCheck(address(), map);
+    }
+  }
 
   // Get the property.
   return Runtime::GetObjectProperty(object, key);

diff --git a/src/ic.h b/src/ic.h
index d32edaa..bbe1f6d 100644 (file)
--- a/src/ic.h
+++ b/src/ic.h
@@ -276,6 +276,11 @@ class KeyedLoadIC: public IC {
   }
 
   static void Clear(Address address, Code* target);
+
+  // Support for patching the map that is checked in an inlined
+  // version of keyed load.
+  static void PatchInlinedMapCheck(Address address, Object* map);
+
   friend class IC;
 };
 

diff --git a/src/stub-cache-ia32.cc b/src/stub-cache-ia32.cc
index c2be65d..f35877f 100644 (file)
--- a/src/stub-cache-ia32.cc
+++ b/src/stub-cache-ia32.cc
@@ -227,8 +227,8 @@ void StubCompiler::GenerateLoadFunctionPrototype(MacroAssembler* masm,
 // are loaded directly otherwise the property is loaded from the properties
 // fixed array.
 void StubCompiler::GenerateFastPropertyLoad(MacroAssembler* masm,
-                              Register dst, Register src,
-                              JSObject* holder, int index) {
+                                            Register dst, Register src,
+                                            JSObject* holder, int index) {
   // Adjust for the number of properties stored in the holder.
   index -= holder->map()->inobject_properties();
   if (index < 0) {

diff --git a/src/v8-counters.h b/src/v8-counters.h
index 586e002..93e8970 100644 (file)
--- a/src/v8-counters.h
+++ b/src/v8-counters.h
@@ -113,6 +113,8 @@ namespace v8 { namespace internal {
   SC(keyed_load_field, V8.KeyedLoadField)                           \
   SC(keyed_load_callback, V8.KeyedLoadCallback)                     \
   SC(keyed_load_interceptor, V8.KeyedLoadInterceptor)               \
+  SC(keyed_load_inline, V8.KeyedLoadInline)                         \
+  SC(keyed_load_inline_miss, V8.KeyedLoadInlineMiss)                \
   SC(keyed_store_field, V8.KeyedStoreField)                         \
   SC(for_in, V8.ForIn)                                              \
   SC(enum_cache_hits, V8.EnumCacheHits)                             \

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index 0f6ceab..926272e 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -5094,6 +5094,12 @@ THREADED_TEST(LockUnlockLock) {
 
 static int GetSurvivingGlobalObjectsCount() {
   int count = 0;
+  // We need to collect all garbage twice to be sure that everything
+  // has been collected.  This is because inline caches are cleared in
+  // the first garbage collection but some of the maps have already
+  // been marked at that point.  Therefore some of the maps are not
+  // collected until the second garbage collection.
+  v8::internal::Heap::CollectAllGarbage();
   v8::internal::Heap::CollectAllGarbage();
   v8::internal::HeapIterator it;
   while (it.has_next()) {
@@ -5114,10 +5120,6 @@ TEST(DontLeakGlobalObjects) {
 
   v8::V8::Initialize();
 
-  // TODO(121): when running "cctest test-api", the initial count is 2,
-  // after second GC, the counter drops to 1. Needs to figure out why
-  // one GC is not enough to collect all garbage.
-  GetSurvivingGlobalObjectsCount();
   int count = GetSurvivingGlobalObjectsCount();
 
   for (int i = 0; i < 5; i++) {