improve allocation accounting for incremental mark

src/heap/spaces.cc

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/v8.h"
 
 #include "src/base/bits.h"
 #include "src/base/platform/platform.h"
 #include "src/full-codegen/full-codegen.h"
 #include "src/heap/mark-compact.h"
@@ skipping 1378 matching lines @@
 void NewSpace::UpdateAllocationInfo() {
   MemoryChunk::UpdateHighWaterMark(allocation_info_.top());
   allocation_info_.set_top(to_space_.page_low());
   allocation_info_.set_limit(to_space_.page_high());
   UpdateInlineAllocationLimit(0);
   DCHECK_SEMISPACE_ALLOCATION_INFO(allocation_info_, to_space_);
 }
 
 
 void NewSpace::ResetAllocationInfo() {
+  Address old_top = allocation_info_.top();
   to_space_.Reset();
   UpdateAllocationInfo();
   pages_used_ = 0;
   // Clear all mark-bits in the to-space.
   NewSpacePageIterator it(&to_space_);
   while (it.has_next()) {
     Bitmap::Clear(it.next());
   }
+  if (top_on_previous_step_) {
+    int bytes_allocated = static_cast<int>(old_top - top_on_previous_step_);
+    heap()->incremental_marking()->Step(bytes_allocated,

[Hannes Payer (out of office), 2015/08/14 05:26:30]

Calling step is is a no-op in this case. ResetAllocationInfo is always called
within the GC. if (heap_->gc_state() != Heap::NOT_IN_GC ... will bail out
immediately.

[ofrobots, 2015/08/14 15:26:18]

The objective is to make accounting of the allocated bytes accurate. Today,
incremental_marking doesn't care about the bytes allocated during GC, but there
might be other consumers of the allocated bytes (such as a sampling heap
profiler) that do care about these bytes. It does make sense to do a
inline-allocation-step here, even if incremental_marking is going to ignore
them.

Once I refactor this into NewSpace::InlineAllocationStep, this would become more
obvious.

[Hannes Payer (out of office), 2015/08/17 17:56:16]

SGTM with the refactoring.

+                                        IncrementalMarking::GC_VIA_STACK_GUARD);
+    top_on_previous_step_ = allocation_info_.top();
+  }
 }
 
 
 void NewSpace::UpdateInlineAllocationLimit(int size_in_bytes) {
   if (heap()->inline_allocation_disabled()) {
     // Lowest limit when linear allocation was disabled.
     Address high = to_space_.page_high();
     Address new_top = allocation_info_.top() + size_in_bytes;
     allocation_info_.set_limit(Min(new_top, high));
   } else if (inline_allocation_limit_step() == 0) {
@@ skipping 58 matching lines @@
   Address high = to_space_.page_high();
   int filler_size = Heap::GetFillToAlign(old_top, alignment);
   int aligned_size_in_bytes = size_in_bytes + filler_size;
 
   if (old_top + aligned_size_in_bytes >= high) {
     // Not enough room in the page, try to allocate a new one.
     if (!AddFreshPage()) {
       return false;
     }
 
-    // Do a step for the bytes allocated on the last page.
-    int bytes_allocated = static_cast<int>(old_top - top_on_previous_step_);
-    heap()->incremental_marking()->Step(bytes_allocated,
-                                        IncrementalMarking::GC_VIA_STACK_GUARD);
+    if (top_on_previous_step_) {
+      // Do a step for the bytes allocated on the last page.
+      int bytes_allocated = static_cast<int>(old_top - top_on_previous_step_);
+      heap()->incremental_marking()->Step(
+          bytes_allocated, IncrementalMarking::GC_VIA_STACK_GUARD);
+      top_on_previous_step_ = allocation_info_.top();
+    }
+
     old_top = allocation_info_.top();
-    top_on_previous_step_ = old_top;
-
     high = to_space_.page_high();
     filler_size = Heap::GetFillToAlign(old_top, alignment);
     aligned_size_in_bytes = size_in_bytes + filler_size;
   }
 
   DCHECK(old_top + aligned_size_in_bytes < high);
 
   if (allocation_info_.limit() < high) {
     // Either the limit has been lowered because linear allocation was disabled
     // or because incremental marking wants to get a chance to do a step. Set
     // the new limit accordingly.
-    Address new_top = old_top + aligned_size_in_bytes;
-    int bytes_allocated = static_cast<int>(new_top - top_on_previous_step_);
-    heap()->incremental_marking()->Step(bytes_allocated,
-                                        IncrementalMarking::GC_VIA_STACK_GUARD);
+    if (top_on_previous_step_) {
+      Address new_top = old_top + aligned_size_in_bytes;
+      int bytes_allocated = static_cast<int>(new_top - top_on_previous_step_);
+      heap()->incremental_marking()->Step(
+          bytes_allocated, IncrementalMarking::GC_VIA_STACK_GUARD);
+      top_on_previous_step_ = new_top;

[ofrobots, 2015/08/04 19:37:11]

I think it might be good to refactor this into a new
NewSpace::InlineAllocationStep method. I can do that. I wanted to keep the
initial CL minimal.

[Hannes Payer (out of office), 2015/08/14 05:26:30]

Yes, good idea.

+    }
     UpdateInlineAllocationLimit(aligned_size_in_bytes);
-    top_on_previous_step_ = new_top;
   }
   return true;
 }
 
 
 #ifdef VERIFY_HEAP
 // We do not use the SemiSpaceIterator because verification doesn't assume
 // that it works (it depends on the invariants we are checking).
 void NewSpace::Verify() {
   // The allocation pointer should be in the space or at the very end.
@@ skipping 1633 matching lines @@
     object->ShortPrint();
     PrintF("\n");
   }
   printf(" --------------------------------------\n");
   printf(" Marked: %x, LiveCount: %x\n", mark_size, LiveBytes());
 }
 
 #endif  // DEBUG
 }  // namespace internal
 }  // namespace v8