RFC: Try to be much more careful with where we skip the write barrier by:

  
  1. Avoid using SKIP_WRITE_BARRIER when we don't have to (smis).
  2. Check and document the remaining uses of SKIP_WRITE_BARRIER.
  3. Only allow GetWriteBarrierMode when in an AssertNoAllocation scope.

The only functional change should be in DeepCopyBoilerplate where we
no longer use the write barrier mode (because of allocations). I'm
running benchmarks to see if this has a measurable impact on performance.
Review URL: http://codereview.chromium.org/558041

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

src/builtins.cc

@@ -247,8 +247,10 @@ BUILTIN(ArrayCodeGeneric) {
   Smi* len = Smi::FromInt(number_of_elements);
   Object* obj = Heap::AllocateFixedArrayWithHoles(len->value());
   if (obj->IsFailure()) return obj;
+
+  AssertNoAllocation no_gc;
   FixedArray* elms = FixedArray::cast(obj);
-  WriteBarrierMode mode = elms->GetWriteBarrierMode();
+  WriteBarrierMode mode = elms->GetWriteBarrierMode(no_gc);
   // Fill in the content
   for (int index = 0; index < number_of_elements; index++) {
     elms->set(index, args[index+1], mode);
@@ -256,7 +258,7 @@ BUILTIN(ArrayCodeGeneric) {
 
   // Set length and elements on the array.
   array->set_elements(FixedArray::cast(obj));
-  array->set_length(len, SKIP_WRITE_BARRIER);
+  array->set_length(len);
 
   return array;
 }
@@ -283,8 +285,10 @@ BUILTIN(ArrayPush) {
     int capacity = new_length + (new_length >> 1) + 16;
     Object* obj = Heap::AllocateFixedArrayWithHoles(capacity);
     if (obj->IsFailure()) return obj;
+
+    AssertNoAllocation no_gc;
     FixedArray* new_elms = FixedArray::cast(obj);
-    WriteBarrierMode mode = new_elms->GetWriteBarrierMode();
+    WriteBarrierMode mode = new_elms->GetWriteBarrierMode(no_gc);
     // Fill out the new array with old elements.
     for (int i = 0; i < len; i++) new_elms->set(i, elms->get(i), mode);
     // Add the provided values.
@@ -295,7 +299,7 @@ BUILTIN(ArrayPush) {
     array->set_elements(new_elms);
   }
   // Set the length.
-  array->set_length(Smi::FromInt(new_length), SKIP_WRITE_BARRIER);
+  array->set_length(Smi::FromInt(new_length));
   return array->length();
 }
 
@@ -313,7 +317,7 @@ BUILTIN(ArrayPop) {
   Object* top = elms->get(len - 1);
 
   // Set the length.
-  array->set_length(Smi::FromInt(len - 1), SKIP_WRITE_BARRIER);
+  array->set_length(Smi::FromInt(len - 1));
 
   if (!top->IsTheHole()) {
     // Delete the top element.

src/dateparser.cc

@@ -72,15 +72,9 @@ bool DateParser::DayComposer::Write(FixedArray* output) {
 
   if (!Smi::IsValid(year) || !IsMonth(month) || !IsDay(day)) return false;
 
-  output->set(YEAR,
-              Smi::FromInt(year),
-              SKIP_WRITE_BARRIER);
-  output->set(MONTH,
-              Smi::FromInt(month - 1),
-              SKIP_WRITE_BARRIER);  // 0-based
-  output->set(DAY,
-              Smi::FromInt(day),
-              SKIP_WRITE_BARRIER);
+  output->set(YEAR, Smi::FromInt(year));
+  output->set(MONTH, Smi::FromInt(month - 1));  // 0-based
+  output->set(DAY, Smi::FromInt(day));
   return true;
 }
 
@@ -103,15 +97,9 @@ bool DateParser::TimeComposer::Write(FixedArray* output) {
 
   if (!IsHour(hour) || !IsMinute(minute) || !IsSecond(second)) return false;
 
-  output->set(HOUR,
-              Smi::FromInt(hour),
-              SKIP_WRITE_BARRIER);
-  output->set(MINUTE,
-              Smi::FromInt(minute),
-              SKIP_WRITE_BARRIER);
-  output->set(SECOND,
-              Smi::FromInt(second),
-              SKIP_WRITE_BARRIER);
+  output->set(HOUR, Smi::FromInt(hour));
+  output->set(MINUTE, Smi::FromInt(minute));
+  output->set(SECOND, Smi::FromInt(second));
   return true;
 }
 
@@ -121,13 +109,9 @@ bool DateParser::TimeZoneComposer::Write(FixedArray* output) {
     if (minute_ == kNone) minute_ = 0;
     int total_seconds = sign_ * (hour_ * 3600 + minute_ * 60);
     if (!Smi::IsValid(total_seconds)) return false;
-    output->set(UTC_OFFSET,
-                Smi::FromInt(total_seconds),
-                SKIP_WRITE_BARRIER);
+    output->set(UTC_OFFSET, Smi::FromInt(total_seconds));
   } else {
-    output->set(UTC_OFFSET,
-                Heap::null_value(),
-                SKIP_WRITE_BARRIER);
+    output->set_null(UTC_OFFSET);
   }
   return true;
 }

src/globals.h

@@ -204,6 +204,7 @@ class AccessorInfo;
 class Allocation;
 class Arguments;
 class Assembler;
+class AssertNoAllocation;
 class BreakableStatement;
 class Code;
 class CodeGenerator;

src/heap.cc

@@ -1729,7 +1729,7 @@ void Heap::SetNumberStringCache(Object* number, String* string) {
   int mask = (number_string_cache()->length() >> 1) - 1;
   if (number->IsSmi()) {
     hash = smi_get_hash(Smi::cast(number)) & mask;
-    number_string_cache()->set(hash * 2, number, SKIP_WRITE_BARRIER);
+    number_string_cache()->set(hash * 2, Smi::cast(number));
   } else {
     hash = double_get_hash(number->Number()) & mask;
     number_string_cache()->set(hash * 2, number);
@@ -1986,8 +1986,10 @@ Object* Heap::AllocateConsString(String* first, String* second) {
 
   Object* result = Allocate(map, NEW_SPACE);
   if (result->IsFailure()) return result;
+
+  AssertNoAllocation no_gc;
   ConsString* cons_string = ConsString::cast(result);
-  WriteBarrierMode mode = cons_string->GetWriteBarrierMode();
+  WriteBarrierMode mode = cons_string->GetWriteBarrierMode(no_gc);
   cons_string->set_length(length);
   cons_string->set_hash_field(String::kEmptyHashField);
   cons_string->set_first(first, mode);
@@ -2285,7 +2287,7 @@ Object* Heap::InitializeFunction(JSFunction* function,
   function->set_shared(shared);
   function->set_prototype_or_initial_map(prototype);
   function->set_context(undefined_value());
-  function->set_literals(empty_fixed_array(), SKIP_WRITE_BARRIER);
+  function->set_literals(empty_fixed_array());
   return function;
 }
 
@@ -2886,8 +2888,10 @@ Object* Heap::CopyFixedArray(FixedArray* src) {
   HeapObject::cast(obj)->set_map(src->map());
   FixedArray* result = FixedArray::cast(obj);
   result->set_length(len);
+
   // Copy the content
-  WriteBarrierMode mode = result->GetWriteBarrierMode();
+  AssertNoAllocation no_gc;
+  WriteBarrierMode mode = result->GetWriteBarrierMode(no_gc);
   for (int i = 0; i < len; i++) result->set(i, src->get(i), mode);
   return result;
 }
@@ -2905,6 +2909,7 @@ Object* Heap::AllocateFixedArray(int length) {
     Object* value = undefined_value();
     // Initialize body.
     for (int index = 0; index < length; index++) {
+      ASSERT(!Heap::InNewSpace(value));  // value = undefined
       array->set(index, value, SKIP_WRITE_BARRIER);
     }
   }
@@ -2960,6 +2965,7 @@ Object* Heap::AllocateFixedArray(int length, PretenureFlag pretenure) {
   array->set_length(length);
   Object* value = undefined_value();
   for (int index = 0; index < length; index++) {
+    ASSERT(!Heap::InNewSpace(value));  // value = undefined
     array->set(index, value, SKIP_WRITE_BARRIER);
   }
   return array;
@@ -2977,6 +2983,7 @@ Object* Heap::AllocateFixedArrayWithHoles(int length) {
     // Initialize body.
     Object* value = the_hole_value();
     for (int index = 0; index < length; index++)  {
+      ASSERT(!Heap::InNewSpace(value));  // value = the hole
       array->set(index, value, SKIP_WRITE_BARRIER);
     }
   }

src/objects-inl.h

@@ -1349,7 +1349,7 @@ void FixedArray::set(int index, Object* value) {
 }
 
 
-WriteBarrierMode HeapObject::GetWriteBarrierMode() {
+WriteBarrierMode HeapObject::GetWriteBarrierMode(const AssertNoAllocation&) {
   if (Heap::InNewSpace(this)) return SKIP_WRITE_BARRIER;
   return UPDATE_WRITE_BARRIER;
 }
@@ -1548,9 +1548,7 @@ uint32_t NumberDictionary::max_number_key() {
 }
 
 void NumberDictionary::set_requires_slow_elements() {
-  set(kMaxNumberKeyIndex,
-      Smi::FromInt(kRequiresSlowElementsMask),
-      SKIP_WRITE_BARRIER);
+  set(kMaxNumberKeyIndex, Smi::FromInt(kRequiresSlowElementsMask));
 }
 
 
@@ -2973,7 +2971,8 @@ void Dictionary<Shape, Key>::SetEntry(int entry,
                                       PropertyDetails details) {
   ASSERT(!key->IsString() || details.IsDeleted() || details.index() > 0);
   int index = HashTable<Shape, Key>::EntryToIndex(entry);
-  WriteBarrierMode mode = FixedArray::GetWriteBarrierMode();
+  AssertNoAllocation no_gc;
+  WriteBarrierMode mode = FixedArray::GetWriteBarrierMode(no_gc);
   FixedArray::set(index, key, mode);
   FixedArray::set(index+1, value, mode);
   FixedArray::fast_set(this, index+2, details.AsSmi());
@@ -3007,8 +3006,13 @@ void JSArray::EnsureSize(int required_size) {
 }
 
 
+void JSArray::set_length(Smi* length) {
+  set_length(static_cast<Object*>(length), SKIP_WRITE_BARRIER);
+}
+
+
 void JSArray::SetContent(FixedArray* storage) {
-  set_length(Smi::FromInt(storage->length()), SKIP_WRITE_BARRIER);
+  set_length(Smi::FromInt(storage->length()));
   set_elements(storage);
 }
 

src/objects.h

@@ -1023,8 +1023,12 @@ class HeapObject: public Object {
   // Casting.
   static inline HeapObject* cast(Object* obj);
 
-  // Return the write barrier mode for this.
-  inline WriteBarrierMode GetWriteBarrierMode();
+  // Return the write barrier mode for this. Callers of this function
+  // must be able to present a reference to an AssertNoAllocation
+  // object as a sign that they are not going to use this function
+  // from code that allocates and thus invalidates the returned write
+  // barrier mode.
+  inline WriteBarrierMode GetWriteBarrierMode(const AssertNoAllocation&);
 
   // Dispatched behavior.
   void HeapObjectShortPrint(StringStream* accumulator);
@@ -4475,6 +4479,10 @@ class JSArray: public JSObject {
   // [length]: The length property.
   DECL_ACCESSORS(length, Object)
 
+  // Overload the length setter to skip write barrier when the length
+  // is set to a smi. This matches the set function on FixedArray.
+  inline void set_length(Smi* length);
+
   Object* JSArrayUpdateLengthFromIndex(uint32_t index, Object* value);
 
   // Initialize the array with the given capacity. The function may

src/runtime.cc

@@ -107,25 +107,23 @@ static Object* DeepCopyBoilerplate(JSObject* boilerplate) {
   // Deep copy local properties.
   if (copy->HasFastProperties()) {
     FixedArray* properties = copy->properties();
-    WriteBarrierMode mode = properties->GetWriteBarrierMode();
     for (int i = 0; i < properties->length(); i++) {
       Object* value = properties->get(i);
       if (value->IsJSObject()) {
-        JSObject* jsObject = JSObject::cast(value);
-        result = DeepCopyBoilerplate(jsObject);
+        JSObject* js_object = JSObject::cast(value);
+        result = DeepCopyBoilerplate(js_object);
         if (result->IsFailure()) return result;
-        properties->set(i, result, mode);
+        properties->set(i, result);
       }
     }
-    mode = copy->GetWriteBarrierMode();
     int nof = copy->map()->inobject_properties();
     for (int i = 0; i < nof; i++) {
       Object* value = copy->InObjectPropertyAt(i);
       if (value->IsJSObject()) {
-        JSObject* jsObject = JSObject::cast(value);
-        result = DeepCopyBoilerplate(jsObject);
+        JSObject* js_object = JSObject::cast(value);
+        result = DeepCopyBoilerplate(js_object);
         if (result->IsFailure()) return result;
-        copy->InObjectPropertyAtPut(i, result, mode);
+        copy->InObjectPropertyAtPut(i, result);
       }
     }
   } else {
@@ -135,20 +133,20 @@ static Object* DeepCopyBoilerplate(JSObject* boilerplate) {
     copy->GetLocalPropertyNames(names, 0);
     for (int i = 0; i < names->length(); i++) {
       ASSERT(names->get(i)->IsString());
-      String* keyString = String::cast(names->get(i));
+      String* key_string = String::cast(names->get(i));
       PropertyAttributes attributes =
-        copy->GetLocalPropertyAttribute(keyString);
+          copy->GetLocalPropertyAttribute(key_string);
       // Only deep copy fields from the object literal expression.
       // In particular, don't try to copy the length attribute of
       // an array.
       if (attributes != NONE) continue;
-      Object* value = copy->GetProperty(keyString, &attributes);
+      Object* value = copy->GetProperty(key_string, &attributes);
       ASSERT(!value->IsFailure());
       if (value->IsJSObject()) {
-        JSObject* jsObject = JSObject::cast(value);
-        result = DeepCopyBoilerplate(jsObject);
+        JSObject* js_object = JSObject::cast(value);
+        result = DeepCopyBoilerplate(js_object);
         if (result->IsFailure()) return result;
-        result = copy->SetProperty(keyString, result, NONE);
+        result = copy->SetProperty(key_string, result, NONE);
         if (result->IsFailure()) return result;
       }
     }
@@ -160,14 +158,13 @@ static Object* DeepCopyBoilerplate(JSObject* boilerplate) {
   switch (copy->GetElementsKind()) {
     case JSObject::FAST_ELEMENTS: {
       FixedArray* elements = FixedArray::cast(copy->elements());
-      WriteBarrierMode mode = elements->GetWriteBarrierMode();
       for (int i = 0; i < elements->length(); i++) {
         Object* value = elements->get(i);
         if (value->IsJSObject()) {
-          JSObject* jsObject = JSObject::cast(value);
-          result = DeepCopyBoilerplate(jsObject);
+          JSObject* js_object = JSObject::cast(value);
+          result = DeepCopyBoilerplate(js_object);
           if (result->IsFailure()) return result;
-          elements->set(i, result, mode);
+          elements->set(i, result);
         }
       }
       break;
@@ -180,8 +177,8 @@ static Object* DeepCopyBoilerplate(JSObject* boilerplate) {
         if (element_dictionary->IsKey(k)) {
           Object* value = element_dictionary->ValueAt(i);
           if (value->IsJSObject()) {
-            JSObject* jsObject = JSObject::cast(value);
-            result = DeepCopyBoilerplate(jsObject);
+            JSObject* js_object = JSObject::cast(value);
+            result = DeepCopyBoilerplate(js_object);
             if (result->IsFailure()) return result;
             element_dictionary->ValueAtPut(i, result);
           }
@@ -1439,6 +1436,8 @@ static Object* Runtime_SetCode(Arguments args) {
       literals->set(JSFunction::kLiteralGlobalContextIndex,
                     context->global_context());
     }
+    // It's okay to skip the write barrier here because the literals
+    // are guaranteed to be in old space.
     target->set_literals(*literals, SKIP_WRITE_BARRIER);
   }
 
@@ -4717,7 +4716,9 @@ static Object* Runtime_NewArguments(Arguments args) {
     if (obj->IsFailure()) return obj;
     FixedArray* array = FixedArray::cast(obj);
     ASSERT(array->length() == length);
-    WriteBarrierMode mode = array->GetWriteBarrierMode();
+
+    AssertNoAllocation no_gc;
+    WriteBarrierMode mode = array->GetWriteBarrierMode(no_gc);
     for (int i = 0; i < length; i++) {
       array->set(i, frame->GetParameter(i), mode);
     }
@@ -4742,10 +4743,13 @@ static Object* Runtime_NewArgumentsFast(Arguments args) {
     // Allocate the fixed array.
     Object* obj = Heap::AllocateRawFixedArray(length);
     if (obj->IsFailure()) return obj;
+
+    AssertNoAllocation no_gc;
     reinterpret_cast<Array*>(obj)->set_map(Heap::fixed_array_map());
     FixedArray* array = FixedArray::cast(obj);
     array->set_length(length);
-    WriteBarrierMode mode = array->GetWriteBarrierMode();
+
+    WriteBarrierMode mode = array->GetWriteBarrierMode(no_gc);
     for (int i = 0; i < length; i++) {
       array->set(i, *--parameters, mode);
     }
@@ -6030,7 +6034,7 @@ static Object* Runtime_MoveArrayContents(Arguments args) {
   to->SetContent(FixedArray::cast(from->elements()));
   to->set_length(from->length());
   from->SetContent(Heap::empty_fixed_array());
-  from->set_length(0);
+  from->set_length(Smi::FromInt(0));
   return to;
 }
 
@@ -6073,9 +6077,7 @@ static Object* Runtime_GetArrayKeys(Arguments args) {
   } else {
     Handle<FixedArray> single_interval = Factory::NewFixedArray(2);
     // -1 means start of array.
-    single_interval->set(0,
-                         Smi::FromInt(-1),
-                         SKIP_WRITE_BARRIER);
+    single_interval->set(0, Smi::FromInt(-1));
     uint32_t actual_length = static_cast<uint32_t>(array->elements()->length());
     uint32_t min_length = actual_length < length ? actual_length : length;
     Handle<Object> length_object =
@@ -7448,7 +7450,9 @@ static Handle<Object> GetArgumentsObject(JavaScriptFrame* frame,
   const int length = frame->GetProvidedParametersCount();
   Handle<JSObject> arguments = Factory::NewArgumentsObject(function, length);
   Handle<FixedArray> array = Factory::NewFixedArray(length);
-  WriteBarrierMode mode = array->GetWriteBarrierMode();
+
+  AssertNoAllocation no_gc;
+  WriteBarrierMode mode = array->GetWriteBarrierMode(no_gc);
   for (int i = 0; i < length; i++) {
     array->set(i, frame->GetParameter(i), mode);
   }
@@ -8032,7 +8036,7 @@ static Object* Runtime_CollectStackTrace(Arguments args) {
       if (cursor + 2 < elements->length()) {
         elements->set(cursor++, recv);
         elements->set(cursor++, fun);
-        elements->set(cursor++, offset, SKIP_WRITE_BARRIER);
+        elements->set(cursor++, offset);
       } else {
         HandleScope scope;
         Handle<Object> recv_handle(recv);
@@ -8045,8 +8049,7 @@ static Object* Runtime_CollectStackTrace(Arguments args) {
     iter.Advance();
   }
 
-  result->set_length(Smi::FromInt(cursor), SKIP_WRITE_BARRIER);
-
+  result->set_length(Smi::FromInt(cursor));
   return *result;
 }