[heap] make inline allocation step size dynamic

Presently the inline allocation step is a static value defined to be the minimum
of the step sizes over all the observers. The step occur every (approx.) step
byte. This is unfair to observers whose steps are not evenly divisible by the
min step size. For example, consider two observers with steps sizes of 512 and
576 bytes. Across 16kb allocated, you would expect the first observer to be hit
approximately 32 times, and the second observer to be hit approximately 28
times.

In reality, the observers get notified 30 and 15 times respectively. The reason
is that each step is 512 bytes, and since 576 is not evenly divisible by 512,
it gets notified much less frequently.

This CL fixes the problem by making the next step size be the minimum (over all
observers) of the remaining bytes to get to the step, making the steps fair.

BUG=
R=hpayer@chromium.org,ulan@chromium.org

Review URL: https://codereview.chromium.org/1427973006

Cr-Commit-Position: refs/heads/master@{#31948}

src/heap/heap.cc

@@ -1736,7 +1736,7 @@ void Heap::Scavenge() {
 
   // We start a new step without accounting the objects copied into to space
   // as those are not allocations.
-  new_space_.UpdateInlineAllocationLimitStep();
+  new_space_.StartNextInlineAllocationStep();
 
   array_buffer_tracker()->FreeDead(true);
 

src/heap/spaces.cc

@@ -1516,7 +1516,7 @@ void NewSpace::UpdateInlineAllocationLimit(int size_in_bytes) {
     // Lower limit during incremental marking.
     Address high = to_space_.page_high();
     Address new_top = allocation_info_.top() + size_in_bytes;
-    Address new_limit = new_top + inline_allocation_limit_step_;
+    Address new_limit = new_top + GetNextInlineAllocationStepSize();
     allocation_info_.set_limit(Min(new_limit, high));
   }
   DCHECK_SEMISPACE_ALLOCATION_INFO(allocation_info_, to_space_);
@@ -1601,21 +1601,28 @@ bool NewSpace::EnsureAllocation(int size_in_bytes,
 }
 
 
-void NewSpace::UpdateInlineAllocationLimitStep() {
-  intptr_t step = 0;
+void NewSpace::StartNextInlineAllocationStep() {
+  top_on_previous_step_ =
+      inline_allocation_observers_.length() ? allocation_info_.top() : 0;
+  UpdateInlineAllocationLimit(0);
+}
+
+
+intptr_t NewSpace::GetNextInlineAllocationStepSize() {
+  intptr_t next_step = 0;
   for (int i = 0; i < inline_allocation_observers_.length(); ++i) {
-    InlineAllocationObserver* observer = inline_allocation_observers_[i];
-    step = step ? Min(step, observer->step_size()) : observer->step_size();
+    InlineAllocationObserver* o = inline_allocation_observers_[i];
+    next_step = next_step ? Min(next_step, o->bytes_to_next_step())
+                          : o->bytes_to_next_step();
   }
-  inline_allocation_limit_step_ = step;
-  top_on_previous_step_ = step ? allocation_info_.top() : 0;
-  UpdateInlineAllocationLimit(0);
+  DCHECK(inline_allocation_observers_.length() == 0 || next_step != 0);
+  return next_step;
 }
 
 
 void NewSpace::AddInlineAllocationObserver(InlineAllocationObserver* observer) {
   inline_allocation_observers_.Add(observer);
-  UpdateInlineAllocationLimitStep();
+  StartNextInlineAllocationStep();
 }
 
 
@@ -1625,7 +1632,7 @@ void NewSpace::RemoveInlineAllocationObserver(
   // Only used in assertion. Suppress unused variable warning.
   static_cast<void>(removed);
   DCHECK(removed);
-  UpdateInlineAllocationLimitStep();
+  StartNextInlineAllocationStep();
 }
 
 

src/heap/spaces.h

@@ -2521,6 +2521,7 @@ class InlineAllocationObserver {
 
  private:
   intptr_t step_size() const { return step_size_; }
+  intptr_t bytes_to_next_step() const { return bytes_to_next_step_; }
 
   // Pure virtual method provided by the subclasses that gets called when more
   // than step_size byte have been allocated.
@@ -2561,7 +2562,6 @@ class NewSpace : public Space {
         to_space_(heap, kToSpace),
         from_space_(heap, kFromSpace),
         reservation_(),
-        inline_allocation_limit_step_(0),
         top_on_previous_step_(0) {}
 
   // Sets up the new space using the given chunk.
@@ -2735,7 +2735,7 @@ class NewSpace : public Space {
   void ResetAllocationInfo();
 
   void UpdateInlineAllocationLimit(int size_in_bytes);
-  void UpdateInlineAllocationLimitStep();
+  void StartNextInlineAllocationStep();
 
   // Allows observation of inline allocation. The observer->Step() method gets
   // called after every step_size bytes have been allocated (approximately).
@@ -2747,7 +2747,6 @@ class NewSpace : public Space {
   void RemoveInlineAllocationObserver(InlineAllocationObserver* observer);
 
   void DisableInlineAllocationSteps() {
-    inline_allocation_limit_step_ = 0;
     top_on_previous_step_ = 0;
     UpdateInlineAllocationLimit(0);
   }
@@ -2849,7 +2848,6 @@ class NewSpace : public Space {
   // once in a while. This is done by setting allocation_info_.limit to be lower
   // than the actual limit and and increasing it in steps to guarantee that the
   // observers are notified periodically.
-  intptr_t inline_allocation_limit_step_;
   List<InlineAllocationObserver*> inline_allocation_observers_;
 
   Address top_on_previous_step_;
@@ -2866,6 +2864,7 @@ class NewSpace : public Space {
   // where the next byte is going to be allocated from. top and new_top may be
   // different when we cross a page boundary or reset the space.
   void InlineAllocationStep(Address top, Address new_top);
+  intptr_t GetNextInlineAllocationStepSize();
 
   friend class SemiSpaceIterator;
 };

test/cctest/test-spaces.cc

@@ -885,5 +885,37 @@ UNINITIALIZED_TEST(InlineAllocationObserver) {
   isolate->Dispose();
 }
 
+
+UNINITIALIZED_TEST(InlineAllocationObserverCadence) {
+  v8::Isolate::CreateParams create_params;
+  create_params.array_buffer_allocator = CcTest::array_buffer_allocator();
+  v8::Isolate* isolate = v8::Isolate::New(create_params);
+  {
+    v8::Isolate::Scope isolate_scope(isolate);
+    v8::HandleScope handle_scope(isolate);
+    v8::Context::New(isolate)->Enter();
+
+    Isolate* i_isolate = reinterpret_cast<Isolate*>(isolate);
+
+    NewSpace* new_space = i_isolate->heap()->new_space();
+
+    Observer observer1(512);
+    new_space->AddInlineAllocationObserver(&observer1);
+    Observer observer2(576);
+    new_space->AddInlineAllocationObserver(&observer2);
+
+    for (int i = 0; i < 512; ++i) {
+      AllocateUnaligned(new_space, 32);
+    }
+
+    new_space->RemoveInlineAllocationObserver(&observer1);
+    new_space->RemoveInlineAllocationObserver(&observer2);
+
+    CHECK_EQ(observer1.count(), 30);
+    CHECK_EQ(observer2.count(), 26);
+  }
+  isolate->Dispose();
+}
+
 }  // namespace internal
 }  // namespace v8