Use CopyElements for SetFastDoubleElementsCapacityAndLength

src/builtins.cc

 // Copyright 2012 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
@@ skipping 490 matching lines @@
 
   if (new_length > elms->length()) {
     // New backing storage is needed.
     int capacity = new_length + (new_length >> 1) + 16;
     Object* obj;
     { MaybeObject* maybe_obj = heap->AllocateUninitializedFixedArray(capacity);
       if (!maybe_obj->ToObject(&obj)) return maybe_obj;
     }
     FixedArray* new_elms = FixedArray::cast(obj);
 
-    AssertNoAllocation no_gc;

[Jakob Kummerow, 2012/03/12 15:33:28]

Did you delete this line on purpose? AFAICS we don't allocate anything during
the call, so we might want to keep the assertion. 

[danno, 2012/03/12 20:35:03]

The AssertNoAllocation doesn't help here, and actually it's misleading (the code
here is wrong as it was). There can't be an allocation between the time that
new_elms is allocated and the copy happens. But the AssertNoAllocation object
doesn't guarantee that, it only guarantees that there is no allocation since the
no allocation object is created. I talked about this with Starzinger, and we
agreed that it's misleading to suggest that there is some sort of protection
through stack-allocated protection objects when there isn't. In other new code,
the WriteBarrierMode is explicit. Until there is a replacement for
AssertNoAllocation that actually is bound tighter to the allocation that it's
trying to protect (that's a bigger chunk of work), the explicit mode at least
forces the caller to think about what is safe and what is not. 

On 2012/03/12 15:33:28, Jakob wrote:

-    CopyObjectToObjectElements(&no_gc, elms, FAST_ELEMENTS, 0,
+    CopyObjectToObjectElements(elms, FAST_ELEMENTS, 0,
                                new_elms, FAST_ELEMENTS, 0, len);
     FillWithHoles(heap, new_elms, new_length, capacity);
 
     elms = new_elms;
   }
 
   // Add the provided values.
   AssertNoAllocation no_gc;
   WriteBarrierMode mode = elms->GetWriteBarrierMode(no_gc);
   for (int index = 0; index < to_add; index++) {
@@ skipping 116 matching lines @@
 
   if (new_length > elms->length()) {
     // New backing storage is needed.
     int capacity = new_length + (new_length >> 1) + 16;
     Object* obj;
     { MaybeObject* maybe_obj = heap->AllocateUninitializedFixedArray(capacity);
       if (!maybe_obj->ToObject(&obj)) return maybe_obj;
     }
     FixedArray* new_elms = FixedArray::cast(obj);
     AssertNoAllocation no_gc;
-    CopyObjectToObjectElements(&no_gc, elms, FAST_ELEMENTS, 0,
+    CopyObjectToObjectElements(elms, FAST_ELEMENTS, 0,
                                new_elms, FAST_ELEMENTS, to_add, len);
     FillWithHoles(heap, new_elms, new_length, capacity);
     elms = new_elms;
     array->set_elements(elms);
   } else {
     AssertNoAllocation no_gc;
     MoveElements(heap, &no_gc, elms, to_add, elms, 0, len);
   }
 
   // Add the provided values.
@@ skipping 91 matching lines @@
   int result_len = Max(final - k, 0);
 
   MaybeObject* maybe_array =
       heap->AllocateJSArrayAndStorage(elements_kind,
                                       result_len,
                                       result_len);
   JSArray* result_array;
   if (!maybe_array->To(&result_array)) return maybe_array;
 
   AssertNoAllocation no_gc;
-  CopyObjectToObjectElements(&no_gc, elms, FAST_ELEMENTS, k,
+  CopyObjectToObjectElements(elms, FAST_ELEMENTS, k,
                              FixedArray::cast(result_array->elements()),
                              FAST_ELEMENTS, 0, result_len);
 
   return result_array;
 }
 
 
 BUILTIN(ArraySplice) {
   Heap* heap = isolate->heap();
   Object* receiver = *args.receiver();
@@ skipping 54 matching lines @@
       JSObject::cast(receiver)->GetElementsKind();
   MaybeObject* maybe_array =
       heap->AllocateJSArrayAndStorage(elements_kind,
                                       actual_delete_count,
                                       actual_delete_count);
   if (!maybe_array->To(&result_array)) return maybe_array;
 
   {
     AssertNoAllocation no_gc;
     // Fill newly created array.
-    CopyObjectToObjectElements(&no_gc, elms, FAST_ELEMENTS, actual_start,
+    CopyObjectToObjectElements(elms, FAST_ELEMENTS, actual_start,
                                FixedArray::cast(result_array->elements()),
                                FAST_ELEMENTS, 0, actual_delete_count);
   }
 
   int item_count = (n_arguments > 1) ? (n_arguments - 2) : 0;
   int new_length = len - actual_delete_count + item_count;
 
   bool elms_changed = false;
   if (item_count < actual_delete_count) {
     // Shrink the array.
@@ skipping 31 matching lines @@
       Object* obj;
       { MaybeObject* maybe_obj =
             heap->AllocateUninitializedFixedArray(capacity);
         if (!maybe_obj->ToObject(&obj)) return maybe_obj;
       }
       FixedArray* new_elms = FixedArray::cast(obj);
 
       {
         AssertNoAllocation no_gc;
         // Copy the part before actual_start as is.
-        CopyObjectToObjectElements(&no_gc, elms, FAST_ELEMENTS, 0,
+        CopyObjectToObjectElements(elms, FAST_ELEMENTS, 0,
                                    new_elms, FAST_ELEMENTS, 0, actual_start);
         const int to_copy = len - actual_delete_count - actual_start;
-        CopyObjectToObjectElements(&no_gc, elms, FAST_ELEMENTS,
+        CopyObjectToObjectElements(elms, FAST_ELEMENTS,
                                    actual_start + actual_delete_count,
                                    new_elms, FAST_ELEMENTS,
                                    actual_start + item_count, to_copy);
       }
 
       FillWithHoles(heap, new_elms, new_length, capacity);
 
       elms = new_elms;
       elms_changed = true;
     } else {
@@ skipping 64 matching lines @@
   // Allocate result.
   JSArray* result_array;
   MaybeObject* maybe_array =
       heap->AllocateJSArrayAndStorage(elements_kind,
                                       result_len,
                                       result_len);
   if (!maybe_array->To(&result_array)) return maybe_array;
   if (result_len == 0) return result_array;
 
   // Copy data.
-  AssertNoAllocation no_gc;

[Jakob Kummerow, 2012/03/12 15:33:28]

Same question here.

[danno, 2012/03/12 20:35:03]

See above.
On 2012/03/12 15:33:28, Jakob wrote:

   int start_pos = 0;
   FixedArray* result_elms(FixedArray::cast(result_array->elements()));
   for (int i = 0; i < n_arguments; i++) {
     JSArray* array = JSArray::cast(args[i]);
     int len = Smi::cast(array->length())->value();
     FixedArray* elms = FixedArray::cast(array->elements());
-    CopyObjectToObjectElements(&no_gc, elms, FAST_ELEMENTS, 0,
+    CopyObjectToObjectElements(elms, FAST_ELEMENTS, 0,
                                result_elms, FAST_ELEMENTS,
                                start_pos, len);
     start_pos += len;
   }
   ASSERT(start_pos == result_len);
 
   return result_array;
 }
 
 
@@ skipping 744 matching lines @@
   return Handle<Code>(code_address);                        \
 }
 BUILTIN_LIST_C(DEFINE_BUILTIN_ACCESSOR_C)
 BUILTIN_LIST_A(DEFINE_BUILTIN_ACCESSOR_A)
 BUILTIN_LIST_DEBUG_A(DEFINE_BUILTIN_ACCESSOR_A)
 #undef DEFINE_BUILTIN_ACCESSOR_C
 #undef DEFINE_BUILTIN_ACCESSOR_A
 
 
 } }  // namespace v8::internal