[heap] Remove live byte adjustments from mutator.

The effect of array/string trimming on space size is postponed until sweeping
completes. This simplifies runtime code and fixes live byte update race with
the concurrent marker.

This patch restores monotonicity of PromotedSinceLastGC by notify the heap
when sweeper discovers more free space than estimated.

Bug: chromium:694255
Change-Id: I7a8c24f2c3398bc0c8a43ffd1d35ace68010cd65
Reviewed-on: https://chromium-review.googlesource.com/621326
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47464}

src/heap/heap.cc

@@ -3352,23 +3352,6 @@ bool Heap::IsImmovable(HeapObject* object) {
   return chunk->NeverEvacuate() || chunk->owner()->identity() == LO_SPACE;
 }
 
-void Heap::AdjustLiveBytes(HeapObject* object, int by) {
-  // As long as the inspected object is black and we are currently not iterating
-  // the heap using HeapIterator, we can update the live byte count. We cannot
-  // update while using HeapIterator because the iterator is temporarily
-  // marking the whole object graph, without updating live bytes.
-  if (lo_space()->Contains(object)) {
-    lo_space()->AdjustLiveBytes(by);
-  } else if (!in_heap_iterator() &&
-             !mark_compact_collector()->sweeping_in_progress() &&
-             incremental_marking()->marking_state()->IsBlack(object)) {
-    DCHECK(MemoryChunk::FromAddress(object->address())->SweepingDone());
-    incremental_marking()->marking_state()->IncrementLiveBytes(
-        MemoryChunk::FromAddress(object->address()), by);
-  }
-}
-
-
 FixedArrayBase* Heap::LeftTrimFixedArray(FixedArrayBase* object,
                                          int elements_to_trim) {
   CHECK_NOT_NULL(object);
@@ -3416,9 +3399,6 @@ FixedArrayBase* Heap::LeftTrimFixedArray(FixedArrayBase* object,
   FixedArrayBase* new_object =
       FixedArrayBase::cast(HeapObject::FromAddress(new_start));
 
-  // Maintain consistency of live bytes during incremental marking
-  AdjustLiveBytes(new_object, -bytes_to_trim);
-
   // Remove recorded slots for the new map and length offset.
   ClearRecordedSlot(new_object, HeapObject::RawField(new_object, 0));
   ClearRecordedSlot(new_object, HeapObject::RawField(
@@ -3492,9 +3472,6 @@ void Heap::RightTrimFixedArray(FixedArrayBase* object, int elements_to_trim) {
   // avoid races with the sweeper thread.
   object->synchronized_set_length(len - elements_to_trim);
 
-  // Maintain consistency of live bytes during incremental marking
-  AdjustLiveBytes(object, -bytes_to_trim);
-
   // Notify the heap profiler of change in object layout. The array may not be
   // moved during GC, and size has to be adjusted nevertheless.
   HeapProfiler* profiler = isolate()->heap_profiler();

src/heap/heap.h

@@ -716,9 +716,6 @@ class Heap {
 
   static bool IsImmovable(HeapObject* object);
 
-  // Maintain consistency of live bytes during incremental marking.
-  void AdjustLiveBytes(HeapObject* object, int by);
-
   // Trim the given array from the left. Note that this relocates the object
   // start and hence is only valid if there is only a single reference to it.
   FixedArrayBase* LeftTrimFixedArray(FixedArrayBase* obj, int elements_to_trim);
@@ -1394,6 +1391,7 @@ class Heap {
   void UpdateOldGenerationAllocationCounter() {
     old_generation_allocation_counter_at_last_gc_ =
         OldGenerationAllocationCounter();
+    old_generation_size_at_last_gc_ = 0;
   }
 
   size_t OldGenerationAllocationCounter() {
@@ -1408,14 +1406,22 @@ class Heap {
 
   size_t PromotedSinceLastGC() {
     size_t old_generation_size = PromotedSpaceSizeOfObjects();
-    if (old_generation_size < old_generation_size_at_last_gc_) {
-      // This can happen if the promoted space size was refined in the
-      // sweeper after mutator doing array trimming.
-      return 0;
-    }
+    DCHECK_GE(old_generation_size, old_generation_size_at_last_gc_);
     return old_generation_size - old_generation_size_at_last_gc_;
   }
 
+  // This is called by the sweeper when it discovers more free space
+  // as expected at the end of the last GC.
+  void NotifyRefinedOldGenerationSize(size_t decreased_bytes) {
+    if (old_generation_size_at_last_gc_ != 0) {
+      // PromotedSpaceSizeOfObjects() is now smaller by |decreased_bytes|.
+      // Adjust old_generation_size_at_last_gc_ too so that PromotedSinceLastGC
+      // stay monotonically non-decreasing function.
+      DCHECK_GE(old_generation_size_at_last_gc_, decreased_bytes);
+      old_generation_size_at_last_gc_ -= decreased_bytes;
+    }
+  }
+
   int gc_count() const { return gc_count_; }
 
   // Returns the size of objects residing in non new spaces.

src/heap/spaces.cc

@@ -1493,8 +1493,15 @@ void PagedSpace::RefineAllocatedBytesAfterSweeping(Page* page) {
   // The live_byte on the page was accounted in the space allocated
   // bytes counter. After sweeping allocated_bytes() contains the
   // accurate live byte count on the page.
-  DecreaseAllocatedBytes(marking_state->live_bytes(page), page);
-  IncreaseAllocatedBytes(page->allocated_bytes(), page);
+  size_t old_counter = marking_state->live_bytes(page);
+  size_t new_counter = page->allocated_bytes();
+  DCHECK_GE(old_counter, new_counter);
+  if (old_counter > new_counter) {
+    DecreaseAllocatedBytes(old_counter - new_counter, page);
+    // Give the heap a chance to adjust counters in response to the
+    // more precise and smaller old generation size.
+    heap()->NotifyRefinedOldGenerationSize(old_counter - new_counter);
+  }
   marking_state->SetLiveBytes(page, 0);
 }
 
@@ -3206,12 +3213,12 @@ void PagedSpace::ReportStatistics() {
 void MapSpace::VerifyObject(HeapObject* object) { CHECK(object->IsMap()); }
 #endif
 
-Address LargePage::GetAddressToShrink() {
-  HeapObject* object = GetObject();
+Address LargePage::GetAddressToShrink(Address object_address,
+                                      size_t object_size) {
   if (executable() == EXECUTABLE) {
     return 0;
   }
-  size_t used_size = ::RoundUp((object->address() - address()) + object->Size(),
+  size_t used_size = ::RoundUp((object_address - address()) + object_size,
                                MemoryAllocator::GetCommitPageSize());
   if (used_size < CommittedPhysicalMemory()) {
     return address() + used_size;
@@ -3418,12 +3425,16 @@ void LargeObjectSpace::FreeUnmarkedObjects() {
   LargePage* current = first_page_;
   IncrementalMarking::NonAtomicMarkingState* marking_state =
       heap()->incremental_marking()->non_atomic_marking_state();
+  objects_size_ = 0;
   while (current != nullptr) {
     HeapObject* object = current->GetObject();
     DCHECK(!marking_state->IsGrey(object));
     if (marking_state->IsBlack(object)) {
       Address free_start;
-      if ((free_start = current->GetAddressToShrink()) != 0) {
+      size_t size = static_cast<size_t>(object->Size());
+      objects_size_ += size;
+      if ((free_start = current->GetAddressToShrink(object->address(), size)) !=
+          0) {
         DCHECK(!current->IsFlagSet(Page::IS_EXECUTABLE));
         current->ClearOutOfLiveRangeSlots(free_start);
         RemoveChunkMapEntries(current, free_start);
@@ -3450,7 +3461,6 @@ void LargeObjectSpace::FreeUnmarkedObjects() {
       // Free the chunk.
       size_ -= static_cast<int>(page->size());
       AccountUncommitted(page->size());
-      objects_size_ -= object->Size();
       page_count_--;
 
       RemoveChunkMapEntries(page);

src/heap/spaces.h

@@ -861,7 +861,7 @@ class LargePage : public MemoryChunk {
 
   // Uncommit memory that is not in use anymore by the object. If the object
   // cannot be shrunk 0 is returned.
-  Address GetAddressToShrink();
+  Address GetAddressToShrink(Address object_address, size_t object_size);
 
   void ClearOutOfLiveRangeSlots(Address free_start);
 
@@ -2923,8 +2923,6 @@ class LargeObjectSpace : public Space {
   // Checks whether the space is empty.
   bool IsEmpty() { return first_page_ == NULL; }
 
-  void AdjustLiveBytes(int by) { objects_size_ += by; }
-
   LargePage* first_page() { return first_page_; }
 
   // Collect code statistics.

src/objects.cc

@@ -2654,8 +2654,6 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
   ExternalTwoByteString* self = ExternalTwoByteString::cast(this);
   self->set_resource(resource);
   if (is_internalized) self->Hash();  // Force regeneration of the hash value.
-
-  heap->AdjustLiveBytes(this, new_size - size);
   return true;
 }
 
@@ -2725,8 +2723,6 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
   ExternalOneByteString* self = ExternalOneByteString::cast(this);
   self->set_resource(resource);
   if (is_internalized) self->Hash();  // Force regeneration of the hash value.
-
-  heap->AdjustLiveBytes(this, new_size - size);
   return true;
 }
 
@@ -4033,7 +4029,6 @@ void MigrateFastToFast(Handle<JSObject> object, Handle<Map> new_map) {
     Address address = object->address();
     heap->CreateFillerObjectAt(address + new_instance_size, instance_size_delta,
                                ClearRecordedSlots::kYes);
-    heap->AdjustLiveBytes(*object, -instance_size_delta);
   }
 
   // We are storing the new map using release store after creating a filler for
@@ -4116,7 +4111,6 @@ void MigrateFastToSlow(Handle<JSObject> object, Handle<Map> new_map,
   if (instance_size_delta > 0) {
     heap->CreateFillerObjectAt(object->address() + new_instance_size,
                                instance_size_delta, ClearRecordedSlots::kYes);
-    heap->AdjustLiveBytes(*object, -instance_size_delta);
   }
 
   // We are storing the new map using release store after creating a filler for
@@ -12120,8 +12114,6 @@ Handle<String> SeqString::Truncate(Handle<SeqString> string, int new_length) {
   // that are a multiple of pointer size.
   heap->CreateFillerObjectAt(start_of_string + new_size, delta,
                              ClearRecordedSlots::kNo);
-  heap->AdjustLiveBytes(*string, -delta);
-
   // We are storing the new length using release store after creating a filler
   // for the left-over space to avoid races with the sweeper thread.
   string->synchronized_set_length(new_length);
@@ -17134,7 +17126,6 @@ void MakeStringThin(String* string, String* internalized, Isolate* isolate) {
     if (size_delta != 0) {
       Heap* heap = isolate->heap();
       heap->CreateFillerObjectAt(thin_end, size_delta, ClearRecordedSlots::kNo);
-      heap->AdjustLiveBytes(thin, -size_delta);
     }
   }
 }

src/runtime/runtime-regexp.cc

@@ -777,7 +777,6 @@ MUST_USE_RESULT static Object* StringReplaceGlobalRegExpWithEmptyString(
   if (!heap->lo_space()->Contains(*answer)) {
     heap->CreateFillerObjectAt(end_of_string, delta, ClearRecordedSlots::kNo);
   }
-  heap->AdjustLiveBytes(*answer, -delta);
   return *answer;
 }
 

test/cctest/heap/test-heap.cc

@@ -5646,17 +5646,44 @@ TEST(Regress598319) {
   }
 }
 
+Handle<FixedArray> ShrinkArrayAndCheckSize(Heap* heap, int length) {
+  // Make sure there is no garbage and the compilation cache is empty.
+  for (int i = 0; i < 5; i++) {
+    CcTest::CollectAllGarbage();
+  }
+  heap->mark_compact_collector()->EnsureSweepingCompleted();
+  size_t size_before_allocation = heap->SizeOfObjects();
+  Handle<FixedArray> array =
+      heap->isolate()->factory()->NewFixedArray(length, TENURED);
+  size_t size_after_allocation = heap->SizeOfObjects();
+  CHECK_EQ(size_after_allocation, size_before_allocation + array->Size());
+  array->Shrink(1);
+  size_t size_after_shrinking = heap->SizeOfObjects();
+  // Shrinking does not change the space size immediately.
+  CHECK_EQ(size_after_allocation, size_after_shrinking);
+  // GC and sweeping updates the size to acccount for shrinking.
+  CcTest::CollectAllGarbage();
+  heap->mark_compact_collector()->EnsureSweepingCompleted();
+  intptr_t size_after_gc = heap->SizeOfObjects();
+  CHECK_EQ(size_after_gc, size_before_allocation + array->Size());
+  return array;
+}
+
 TEST(Regress609761) {
   CcTest::InitializeVM();
   v8::HandleScope scope(CcTest::isolate());
   Heap* heap = CcTest::heap();
-  Isolate* isolate = heap->isolate();
+  int length = kMaxRegularHeapObjectSize / kPointerSize + 1;
+  Handle<FixedArray> array = ShrinkArrayAndCheckSize(heap, length);
+  CHECK(heap->lo_space()->Contains(*array));
+}
 
-  intptr_t size_before = heap->SizeOfObjects();
-  Handle<FixedArray> array = isolate->factory()->NewFixedArray(200000);
-  array->Shrink(1);
-  intptr_t size_after = heap->SizeOfObjects();
-  CHECK_EQ(size_after, size_before + array->Size());
+TEST(LiveBytes) {
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+  Heap* heap = CcTest::heap();
+  Handle<FixedArray> array = ShrinkArrayAndCheckSize(heap, 2000);
+  CHECK(heap->old_space()->Contains(*array));
 }
 
 TEST(Regress615489) {