Reapply load ICs for nonexistent properties.
authorager@chromium.org <ager@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Thu, 15 Apr 2010 11:25:41 +0000 (11:25 +0000)
committerager@chromium.org <ager@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Thu, 15 Apr 2010 11:25:41 +0000 (11:25 +0000)
We need to be careful to check global property cells for the property
encountered during lookup.  Therefore, the ICs have to be specific to
the name of the property if global objects are involved.  In
principle, this means that we could get a large number of monomorphic
ICs for the same map if there is a global object in the prototype
chain.  However, since this is only done for normal load ICs and not
for keyed load ICs I do not expect this to be a problem.  I will
experiment with it once this goes in.

BUG=675
Review URL: http://codereview.chromium.org/1559033

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

src/arm/stub-cache-arm.cc
src/globals.h
src/ia32/stub-cache-ia32.cc
src/ic.cc
src/stub-cache.cc
src/stub-cache.h
src/x64/stub-cache-x64.cc
test/mjsunit/regress/regress-675.js [new file with mode: 0644]

index bbffef2..1762eb6 100644 (file)
@@ -600,6 +600,28 @@ static void CompileLoadInterceptor(LoadInterceptorCompiler* compiler,
 }
 
 
+// Generate code to check that a global property cell is empty. Create
+// the property cell at compilation time if no cell exists for the
+// property.
+static Object* GenerateCheckPropertyCell(MacroAssembler* masm,
+                                         GlobalObject* global,
+                                         String* name,
+                                         Register scratch,
+                                         Label* miss) {
+  Object* probe = global->EnsurePropertyCell(name);
+  if (probe->IsFailure()) return probe;
+  JSGlobalPropertyCell* cell = JSGlobalPropertyCell::cast(probe);
+  ASSERT(cell->value()->IsTheHole());
+  __ mov(scratch, Operand(Handle<Object>(cell)));
+  __ ldr(scratch,
+         FieldMemOperand(scratch, JSGlobalPropertyCell::kValueOffset));
+  __ LoadRoot(ip, Heap::kTheHoleValueRootIndex);
+  __ cmp(scratch, ip);
+  __ b(ne, miss);
+  return cell;
+}
+
+
 #undef __
 #define __ ACCESS_MASM(masm())
 
@@ -620,23 +642,19 @@ Register StubCompiler::CheckPrototypes(JSObject* object,
       masm()->CheckMaps(object, object_reg, holder, holder_reg, scratch, miss);
 
   // If we've skipped any global objects, it's not enough to verify
-  // that their maps haven't changed.
+  // that their maps haven't changed.  We also need to check that the
+  // property cell for the property is still empty.
   while (object != holder) {
     if (object->IsGlobalObject()) {
-      GlobalObject* global = GlobalObject::cast(object);
-      Object* probe = global->EnsurePropertyCell(name);
-      if (probe->IsFailure()) {
-        set_failure(Failure::cast(probe));
+      Object* cell = GenerateCheckPropertyCell(masm(),
+                                               GlobalObject::cast(object),
+                                               name,
+                                               scratch,
+                                               miss);
+      if (cell->IsFailure()) {
+        set_failure(Failure::cast(cell));
         return result;
       }
-      JSGlobalPropertyCell* cell = JSGlobalPropertyCell::cast(probe);
-      ASSERT(cell->value()->IsTheHole());
-      __ mov(scratch, Operand(Handle<Object>(cell)));
-      __ ldr(scratch,
-             FieldMemOperand(scratch, JSGlobalPropertyCell::kValueOffset));
-      __ LoadRoot(ip, Heap::kTheHoleValueRootIndex);
-      __ cmp(scratch, ip);
-      __ b(ne, miss);
     }
     object = JSObject::cast(object->GetPrototype());
   }
@@ -1389,6 +1407,46 @@ Object* StoreStubCompiler::CompileStoreGlobal(GlobalObject* object,
 }
 
 
+Object* LoadStubCompiler::CompileLoadNonexistent(String* name,
+                                                 JSObject* object,
+                                                 JSObject* last) {
+  // ----------- S t a t e -------------
+  //  -- r2    : name
+  //  -- lr    : return address
+  //  -- [sp]  : receiver
+  // -----------------------------------
+  Label miss;
+
+  // Load receiver.
+  __ ldr(r0, MemOperand(sp, 0));
+
+  // Check the maps of the full prototype chain.
+  CheckPrototypes(object, r0, last, r3, r1, name, &miss);
+
+  // If the last object in the prototype chain is a global object,
+  // check that the global property cell is empty.
+  if (last->IsGlobalObject()) {
+    Object* cell = GenerateCheckPropertyCell(masm(),
+                                             GlobalObject::cast(last),
+                                             name,
+                                             r1,
+                                             &miss);
+    if (cell->IsFailure()) return cell;
+  }
+
+  // Return undefined if maps of the full prototype chain are still the
+  // same and no global property with this name contains a value.
+  __ LoadRoot(r0, Heap::kUndefinedValueRootIndex);
+  __ Ret();
+
+  __ bind(&miss);
+  GenerateLoadMiss(masm(), Code::LOAD_IC);
+
+  // Return the generated code.
+  return GetCode(NONEXISTENT, Heap::empty_string());
+}
+
+
 Object* LoadStubCompiler::CompileLoadField(JSObject* object,
                                            JSObject* holder,
                                            int index,
index 3d48e2d..410cb3f 100644 (file)
@@ -428,7 +428,11 @@ enum PropertyType {
   CONSTANT_TRANSITION = 6,  // only in fast mode
   NULL_DESCRIPTOR     = 7,  // only in fast mode
   // All properties before MAP_TRANSITION are real.
-  FIRST_PHANTOM_PROPERTY_TYPE = MAP_TRANSITION
+  FIRST_PHANTOM_PROPERTY_TYPE = MAP_TRANSITION,
+  // There are no IC stubs for NULL_DESCRIPTORS. Therefore,
+  // NULL_DESCRIPTOR can be used as the type flag for IC stubs for
+  // nonexistent properties.
+  NONEXISTENT = NULL_DESCRIPTOR
 };
 
 
index 315600b..dd8a496 100644 (file)
@@ -950,6 +950,26 @@ void StubCompiler::GenerateStoreField(MacroAssembler* masm,
 }
 
 
+// Generate code to check that a global property cell is empty. Create
+// the property cell at compilation time if no cell exists for the
+// property.
+static Object* GenerateCheckPropertyCell(MacroAssembler* masm,
+                                         GlobalObject* global,
+                                         String* name,
+                                         Register scratch,
+                                         Label* miss) {
+  Object* probe = global->EnsurePropertyCell(name);
+  if (probe->IsFailure()) return probe;
+  JSGlobalPropertyCell* cell = JSGlobalPropertyCell::cast(probe);
+  ASSERT(cell->value()->IsTheHole());
+  __ mov(scratch, Immediate(Handle<Object>(cell)));
+  __ cmp(FieldOperand(scratch, JSGlobalPropertyCell::kValueOffset),
+         Immediate(Factory::the_hole_value()));
+  __ j(not_equal, miss, not_taken);
+  return cell;
+}
+
+
 #undef __
 #define __ ACCESS_MASM(masm())
 
@@ -968,21 +988,19 @@ Register StubCompiler::CheckPrototypes(JSObject* object,
                         push_at_depth, miss);
 
   // If we've skipped any global objects, it's not enough to verify
-  // that their maps haven't changed.
+  // that their maps haven't changed.  We also need to check that the
+  // property cell for the property is still empty.
   while (object != holder) {
     if (object->IsGlobalObject()) {
-      GlobalObject* global = GlobalObject::cast(object);
-      Object* probe = global->EnsurePropertyCell(name);
-      if (probe->IsFailure()) {
-        set_failure(Failure::cast(probe));
+      Object* cell = GenerateCheckPropertyCell(masm(),
+                                               GlobalObject::cast(object),
+                                               name,
+                                               scratch,
+                                               miss);
+      if (cell->IsFailure()) {
+        set_failure(Failure::cast(cell));
         return result;
       }
-      JSGlobalPropertyCell* cell = JSGlobalPropertyCell::cast(probe);
-      ASSERT(cell->value()->IsTheHole());
-      __ mov(scratch, Immediate(Handle<Object>(cell)));
-      __ cmp(FieldOperand(scratch, JSGlobalPropertyCell::kValueOffset),
-             Immediate(Factory::the_hole_value()));
-      __ j(not_equal, miss, not_taken);
     }
     object = JSObject::cast(object->GetPrototype());
   }
@@ -1948,6 +1966,44 @@ Object* KeyedStoreStubCompiler::CompileStoreField(JSObject* object,
 }
 
 
+Object* LoadStubCompiler::CompileLoadNonexistent(String* name,
+                                                 JSObject* object,
+                                                 JSObject* last) {
+  // ----------- S t a t e -------------
+  //  -- eax    : receiver
+  //  -- ecx    : name
+  //  -- esp[0] : return address
+  // -----------------------------------
+  Label miss;
+
+  // Check the maps of the full prototype chain. Also check that
+  // global property cells up to (but not including) the last object
+  // in the prototype chain are empty.
+  CheckPrototypes(object, eax, last, ebx, edx, name, &miss);
+
+  // If the last object in the prototype chain is a global object,
+  // check that the global property cell is empty.
+  if (last->IsGlobalObject()) {
+    Object* cell = GenerateCheckPropertyCell(masm(),
+                                             GlobalObject::cast(last),
+                                             name,
+                                             edx,
+                                             &miss);
+    if (cell->IsFailure()) return cell;
+  }
+
+  // Return undefined if maps of the full prototype chain are still the
+  // same and no global property with this name contains a value.
+  __ mov(eax, Factory::undefined_value());
+  __ ret(0);
+
+  __ bind(&miss);
+  GenerateLoadMiss(masm(), Code::LOAD_IC);
+
+  // Return the generated code.
+  return GetCode(NONEXISTENT, Heap::empty_string());
+}
+
 
 Object* LoadStubCompiler::CompileLoadField(JSObject* object,
                                            JSObject* holder,
index b9ca00f..eaa0554 100644 (file)
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -694,8 +694,8 @@ void LoadIC::UpdateCaches(LookupResult* lookup,
                           State state,
                           Handle<Object> object,
                           Handle<String> name) {
-  // Bail out if we didn't find a result.
-  if (!lookup->IsProperty() || !lookup->IsCacheable()) return;
+  // Bail out if the result is not cacheable.
+  if (!lookup->IsCacheable()) return;
 
   // Loading properties from values is not common, so don't try to
   // deal with non-JS objects here.
@@ -709,6 +709,9 @@ void LoadIC::UpdateCaches(LookupResult* lookup,
     // Set the target to the pre monomorphic stub to delay
     // setting the monomorphic state.
     code = pre_monomorphic_stub();
+  } else if (!lookup->IsProperty()) {
+    // Nonexistent property. The result is undefined.
+    code = StubCache::ComputeLoadNonexistent(*name, *receiver);
   } else {
     // Compute monomorphic stub.
     switch (lookup->type()) {
index 95877fb..a3aaf5e 100644 (file)
@@ -93,6 +93,38 @@ Code* StubCache::Set(String* name, Map* map, Code* code) {
 }
 
 
+Object* StubCache::ComputeLoadNonexistent(String* name, JSObject* receiver) {
+  // If no global objects are present in the prototype chain, the load
+  // nonexistent IC stub can be shared for all names for a given map
+  // and we use the empty string for the map cache in that case.  If
+  // there are global objects involved, we need to check global
+  // property cells in the stub and therefore the stub will be
+  // specific to the name.
+  String* cache_name = Heap::empty_string();
+  if (receiver->IsGlobalObject()) cache_name = name;
+  JSObject* last = receiver;
+  while (last->GetPrototype() != Heap::null_value()) {
+    last = JSObject::cast(last->GetPrototype());
+    if (last->IsGlobalObject()) cache_name = name;
+  }
+  // Compile the stub that is either shared for all names or
+  // name specific if there are global objects involved.
+  Code::Flags flags =
+      Code::ComputeMonomorphicFlags(Code::LOAD_IC, NONEXISTENT);
+  Object* code = receiver->map()->FindInCodeCache(cache_name, flags);
+  if (code->IsUndefined()) {
+    LoadStubCompiler compiler;
+    code = compiler.CompileLoadNonexistent(cache_name, receiver, last);
+    if (code->IsFailure()) return code;
+    PROFILE(CodeCreateEvent(Logger::LOAD_IC_TAG, Code::cast(code), cache_name));
+    Object* result =
+        receiver->map()->UpdateCodeCache(cache_name, Code::cast(code));
+    if (result->IsFailure()) return result;
+  }
+  return Set(name, receiver->map(), Code::cast(code));
+}
+
+
 Object* StubCache::ComputeLoadField(String* name,
                                     JSObject* receiver,
                                     JSObject* holder,
index 0ca37e7..57442ff 100644 (file)
@@ -56,6 +56,8 @@ class StubCache : public AllStatic {
 
   // Computes the right stub matching. Inserts the result in the
   // cache before returning.  This might compile a stub if needed.
+  static Object* ComputeLoadNonexistent(String* name, JSObject* receiver);
+
   static Object* ComputeLoadField(String* name,
                                   JSObject* receiver,
                                   JSObject* holder,
@@ -461,18 +463,25 @@ class StubCompiler BASE_EMBEDDED {
 
 class LoadStubCompiler: public StubCompiler {
  public:
+  Object* CompileLoadNonexistent(String* name,
+                                 JSObject* object,
+                                 JSObject* last);
+
   Object* CompileLoadField(JSObject* object,
                            JSObject* holder,
                            int index,
                            String* name);
+
   Object* CompileLoadCallback(String* name,
                               JSObject* object,
                               JSObject* holder,
                               AccessorInfo* callback);
+
   Object* CompileLoadConstant(JSObject* object,
                               JSObject* holder,
                               Object* value,
                               String* name);
+
   Object* CompileLoadInterceptor(JSObject* object,
                                  JSObject* holder,
                                  String* name);
@@ -494,17 +503,21 @@ class KeyedLoadStubCompiler: public StubCompiler {
                            JSObject* object,
                            JSObject* holder,
                            int index);
+
   Object* CompileLoadCallback(String* name,
                               JSObject* object,
                               JSObject* holder,
                               AccessorInfo* callback);
+
   Object* CompileLoadConstant(String* name,
                               JSObject* object,
                               JSObject* holder,
                               Object* value);
+
   Object* CompileLoadInterceptor(JSObject* object,
                                  JSObject* holder,
                                  String* name);
+
   Object* CompileLoadArrayLength(String* name);
   Object* CompileLoadStringLength(String* name);
   Object* CompileLoadFunctionPrototype(String* name);
index 03b21a5..fa342b3 100644 (file)
@@ -649,6 +649,26 @@ class CallInterceptorCompiler BASE_EMBEDDED {
 };
 
 
+// Generate code to check that a global property cell is empty. Create
+// the property cell at compilation time if no cell exists for the
+// property.
+static Object* GenerateCheckPropertyCell(MacroAssembler* masm,
+                                         GlobalObject* global,
+                                         String* name,
+                                         Register scratch,
+                                         Label* miss) {
+  Object* probe = global->EnsurePropertyCell(name);
+  if (probe->IsFailure()) return probe;
+  JSGlobalPropertyCell* cell = JSGlobalPropertyCell::cast(probe);
+  ASSERT(cell->value()->IsTheHole());
+  __ Move(scratch, Handle<Object>(cell));
+  __ Cmp(FieldOperand(scratch, JSGlobalPropertyCell::kValueOffset),
+         Factory::the_hole_value());
+  __ j(not_equal, miss);
+  return cell;
+}
+
+
 #undef __
 
 #define __ ACCESS_MASM((masm()))
@@ -1144,6 +1164,48 @@ Object* LoadStubCompiler::CompileLoadConstant(JSObject* object,
 }
 
 
+Object* LoadStubCompiler::CompileLoadNonexistent(String* name,
+                                                 JSObject* object,
+                                                 JSObject* last) {
+  // ----------- S t a t e -------------
+  //  -- rcx    : name
+  //  -- rsp[0] : return address
+  //  -- rsp[8] : receiver
+  // -----------------------------------
+  Label miss;
+
+  // Load receiver.
+  __ movq(rax, Operand(rsp, kPointerSize));
+
+  // Check the maps of the full prototype chain. Also check that
+  // global property cells up to (but not including) the last object
+  // in the prototype chain are empty.
+  CheckPrototypes(object, rax, last, rbx, rdx, name, &miss);
+
+  // If the last object in the prototype chain is a global object,
+  // check that the global property cell is empty.
+  if (last->IsGlobalObject()) {
+    Object* cell = GenerateCheckPropertyCell(masm(),
+                                             GlobalObject::cast(last),
+                                             name,
+                                             rdx,
+                                             &miss);
+    if (cell->IsFailure()) return cell;
+  }
+
+  // Return undefined if maps of the full prototype chain are still the
+  // same and no global property with this name contains a value.
+  __ LoadRoot(rax, Heap::kUndefinedValueRootIndex);
+  __ ret(0);
+
+  __ bind(&miss);
+  GenerateLoadMiss(masm(), Code::LOAD_IC);
+
+  // Return the generated code.
+  return GetCode(NONEXISTENT, Heap::empty_string());
+}
+
+
 Object* LoadStubCompiler::CompileLoadField(JSObject* object,
                                            JSObject* holder,
                                            int index,
@@ -1765,21 +1827,19 @@ Register StubCompiler::CheckPrototypes(JSObject* object,
       __ CheckMaps(object, object_reg, holder, holder_reg, scratch, miss);
 
   // If we've skipped any global objects, it's not enough to verify
-  // that their maps haven't changed.
+  // that their maps haven't changed.  We also need to check that the
+  // property cell for the property is still empty.
   while (object != holder) {
     if (object->IsGlobalObject()) {
-      GlobalObject* global = GlobalObject::cast(object);
-      Object* probe = global->EnsurePropertyCell(name);
-      if (probe->IsFailure()) {
-        set_failure(Failure::cast(probe));
+      Object* cell = GenerateCheckPropertyCell(masm(),
+                                               GlobalObject::cast(object),
+                                               name,
+                                               scratch,
+                                               miss);
+      if (cell->IsFailure()) {
+        set_failure(Failure::cast(cell));
         return result;
       }
-      JSGlobalPropertyCell* cell = JSGlobalPropertyCell::cast(probe);
-      ASSERT(cell->value()->IsTheHole());
-      __ Move(scratch, Handle<Object>(cell));
-      __ Cmp(FieldOperand(scratch, JSGlobalPropertyCell::kValueOffset),
-             Factory::the_hole_value());
-      __ j(not_equal, miss);
     }
     object = JSObject::cast(object->GetPrototype());
   }
diff --git a/test/mjsunit/regress/regress-675.js b/test/mjsunit/regress/regress-675.js
new file mode 100644 (file)
index 0000000..19ca646
--- /dev/null
@@ -0,0 +1,61 @@
+// Copyright 2010 the V8 project authors. All rights reserved.
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+//       notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+//       copyright notice, this list of conditions and the following
+//       disclaimer in the documentation and/or other materials provided
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// Regression test for http://code.google.com/p/v8/issues/detail?id=675.
+//
+// Test that load ICs for nonexistent properties check global
+// property cells.
+
+function f() { return this.x; }
+
+// Initialize IC for nonexistent x property on global object.
+f();
+f();
+
+// Assign to global property cell for x.
+this.x = 23;
+
+// Check that we bail out from the IC.
+assertEquals(23, f());
+
+
+// Same test, but test that the global property cell is also checked
+// if the global object is the last object in the prototype chain for
+// the load.
+this.__proto__ = null;
+function g() { return this.y; }
+
+// Initialize IC.
+g();
+g();
+
+// Update global property cell.
+this.y = 42;
+
+// Check that IC bails out.
+assertEquals(42, g());
+