Synchronize store buffer processing and concurrent sweeping.

src/spaces.cc

Index: src/spaces.cc
diff --git a/src/spaces.cc b/src/spaces.cc
index 7831ef1ecdf66ecd1f4bcbd9c92abd8a392349a1..d1890f17314b62311186d40558fd4b516131773d 100644
--- a/src/spaces.cc
+++ b/src/spaces.cc
@@ -2017,10 +2017,13 @@ void FreeListNode::set_size(Heap* heap, int size_in_bytes) {
   // field and a next pointer, we give it a filler map that gives it the
   // correct size.
   if (size_in_bytes > FreeSpace::kHeaderSize) {
-    set_map_no_write_barrier(heap->raw_unchecked_free_space_map());
     // Can't use FreeSpace::cast because it fails during deserialization.
+    // We have to set the size first with a release store before we store
+    // the map because a concurrent store buffer scan on scavenge must not
+    // observe a map with an invalid size.
     FreeSpace* this_as_free_space = reinterpret_cast<FreeSpace*>(this);
-    this_as_free_space->set_size(size_in_bytes);
+    this_as_free_space->synchronized_set_size(size_in_bytes);
+    set_map_no_write_barrier(heap->raw_unchecked_free_space_map());

[Jarin, 2014/04/08 13:00:53]

Here we should do unsynchronized write of size and then release write of the
map, no?

As it is written here, other threads could still see the new map with a bogus
size.

[Hannes Payer (out of office), 2014/04/08 14:04:35]

Done.

   } else if (size_in_bytes == kPointerSize) {
     set_map_no_write_barrier(heap->raw_unchecked_one_pointer_filler_map());
   } else if (size_in_bytes == 2 * kPointerSize) {
@@ -2064,11 +2067,11 @@ void FreeListNode::set_next(FreeListNode* next) {
   // stage.
   if (map() == GetHeap()->raw_unchecked_free_space_map()) {
     ASSERT(map() == NULL || Size() >= kNextOffset + kPointerSize);
-    Memory::Address_at(address() + kNextOffset) =
-        reinterpret_cast<Address>(next);
+    Release_Store(reinterpret_cast<AtomicWord*>(address() + kNextOffset),

[Jarin, 2014/04/08 13:00:53]

What visibility does the Release_Store provide here?

I believe the free list is only manipulated "concurrently" under a lock (in
FreeListCategory::Concatenate), so we should not need synchronization here. (The
same goes for the line below.)

[Hannes Payer (out of office), 2014/04/08 14:04:35]

Done. Good point.

+                  reinterpret_cast<AtomicWord>(next));
   } else {
-    Memory::Address_at(address() + kPointerSize) =
-        reinterpret_cast<Address>(next);
+    Release_Store(reinterpret_cast<AtomicWord*>(address() + kPointerSize),
+                  reinterpret_cast<AtomicWord>(next));
   }
 }