Fix logic for doing incremental marking steps on tenured allocation.

src/heap/spaces.cc

Index: src/heap/spaces.cc
diff --git a/src/heap/spaces.cc b/src/heap/spaces.cc
index d372083a4a63aee1e48dbc4a18950a9ffa79e39e..06fde3c7b09e2b7acb91f82500b22c1fbb66e193 100644
--- a/src/heap/spaces.cc
+++ b/src/heap/spaces.cc
@@ -2358,6 +2358,15 @@ FreeSpace* FreeList::FindNodeFor(int size_in_bytes, int* node_size) {
 }
 
 
+void PagedSpace::SetTopAndLimit(Address top, Address limit) {
+  DCHECK(top == limit ||
+         Page::FromAddress(top) == Page::FromAddress(limit - 1));
+  MemoryChunk::UpdateHighWaterMark(allocation_info_.top());
+  allocation_info_.set_top(top);
+  allocation_info_.set_limit(limit);
+}
+
+
 // Allocation on the old space free list.  If it succeeds then a new linear
 // allocation space has been set up with the top and limit of the space.  If
 // the allocation fails then NULL is returned, and the caller can perform a GC
@@ -2375,9 +2384,6 @@ HeapObject* FreeList::Allocate(int size_in_bytes) {
   // if it is big enough.
   owner_->Free(owner_->top(), old_linear_size);
 
-  owner_->heap()->incremental_marking()->OldSpaceStep(size_in_bytes -
-                                                      old_linear_size);

[Erik Corry Chromium.org, 2015/03/31 13:11:32]

This was basically wrong, basing the step on the size of one allocation plus
what was left of the old linear allocation area, but ignoring the rest of the
new linear allocation area.

-
   int new_node_size = 0;
   FreeSpace* new_node = FindNodeFor(size_in_bytes, &new_node_size);
   if (new_node == NULL) {
@@ -2400,7 +2406,11 @@ HeapObject* FreeList::Allocate(int size_in_bytes) {
   // candidate.
   DCHECK(!MarkCompactCollector::IsOnEvacuationCandidate(new_node));
 
-  const int kThreshold = IncrementalMarking::kAllocatedThreshold;
+  // An old-space step will mark more data per byte allocated, because old space
+  // allocation is more serious.  We don't want the pause to be bigger, so we
+  // do marking after a smaller amount of allocation.
+  const int kThreshold = IncrementalMarking::kAllocatedThreshold *
+                         IncrementalMarking::kOldSpaceAllocationMarkingFactor;
 
   // Memory in the linear allocation area is counted as allocated.  We may free
   // a little of this again immediately - see below.
@@ -2412,9 +2422,9 @@ HeapObject* FreeList::Allocate(int size_in_bytes) {
     owner_->Free(new_node->address() + size_in_bytes, bytes_left);
     DCHECK(owner_->top() == NULL && owner_->limit() == NULL);
   } else if (bytes_left > kThreshold &&
-             owner_->heap()->incremental_marking()->IsMarkingIncomplete() &&
-             FLAG_incremental_marking_steps) {
+             owner_->heap()->incremental_marking()->CanDoSteps()) {
     int linear_size = owner_->RoundSizeDownToObjectAlignment(kThreshold);
+
     // We don't want to give too large linear areas to the allocator while
     // incremental marking is going on, because we won't check again whether
     // we want to do another increment until the linear area is used up.
@@ -2422,15 +2432,26 @@ HeapObject* FreeList::Allocate(int size_in_bytes) {
                  new_node_size - size_in_bytes - linear_size);
     owner_->SetTopAndLimit(new_node->address() + size_in_bytes,
                            new_node->address() + size_in_bytes + linear_size);
-  } else if (bytes_left > 0) {
-    // Normally we give the rest of the node to the allocator as its new
-    // linear allocation area.
-    owner_->SetTopAndLimit(new_node->address() + size_in_bytes,
-                           new_node->address() + new_node_size);
+    owner_->heap()->incremental_marking()->OldSpaceStep(size_in_bytes +
+                                                        linear_size);
   } else {
-    // TODO(gc) Try not freeing linear allocation region when bytes_left
-    // are zero.
-    owner_->SetTopAndLimit(NULL, NULL);
+    if (owner_->heap()->incremental_marking()->CanDoSteps()) {
+      owner_->heap()->incremental_marking()->OldSpaceStep(new_node_size);
+    } else if (new_node_size > kThreshold) {
+      // When we give big chunks to the old space allocator we do a little
+      // step in case the incremental marker wants to start.
+      owner_->heap()->incremental_marking()->OldSpaceStep(kThreshold);
+    }
+    if (bytes_left > 0) {
+      // Normally we give the rest of the node to the allocator as its new
+      // linear allocation area.
+      owner_->SetTopAndLimit(new_node->address() + size_in_bytes,
+                             new_node->address() + new_node_size);
+    } else {
+      // TODO(gc) Try not freeing linear allocation region when bytes_left
+      // are zero.
+      owner_->SetTopAndLimit(NULL, NULL);
+    }
   }
 
   return new_node;
@@ -3147,5 +3168,6 @@ void Page::Print() {
 }
 
 #endif  // DEBUG
+
 }
 }  // namespace v8::internal