[heap] refactor SpaceWithLinearArea

* NewSpace::UpdateInlineAllocationInfo and PagedSpace::ComputeLimit were
closely related methods. Refactor these into a shared method in the
super class.
* refactor UpdateInlineAllocationInfo into SpaceWithLinearArea
* refactor StartNextInlineAllocationStep
* refactor PauseAllocationObservers

Bug: 
Change-Id: I898906d6228ff48e427367ef74e6dc77fb7a1837
Reviewed-on: https://chromium-review.googlesource.com/825591
Commit-Queue: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50247}

src/heap/spaces.cc

@@ -1624,33 +1624,34 @@ void PagedSpace::DecreaseLimit(Address new_limit) {
   }
 }
 
-Address PagedSpace::ComputeLimit(Address start, Address end,
-                                 size_t size_in_bytes) {
-  DCHECK_GE(end - start, size_in_bytes);
+Address SpaceWithLinearArea::ComputeLimit(Address start, Address end,
+                                          size_t min_size) {
+  DCHECK_GE(end - start, min_size);
 
   if (heap()->inline_allocation_disabled()) {
-    // Keep the linear allocation area to fit exactly the requested size.
-    return start + size_in_bytes;
+    // Fit the requested area exactly.
+    return start + min_size;
   } else if (SupportsInlineAllocation() && AllocationObserversActive()) {
-    // Generated code may allocate inline from the linear allocation area for
-    // Old Space. To make sure we can observe these allocations, we use a lower
-    // limit.
-    size_t step = RoundSizeDownToObjectAlignment(
-        static_cast<int>(GetNextInlineAllocationStepSize()));
-    return Min(start + size_in_bytes + step, end);
+    // Generated code may allocate inline from the linear allocation area for.
+    // To make sure we can observe these allocations, we use a lower limit.
+    size_t step = GetNextInlineAllocationStepSize();
+
+    // TODO(ofrobots): there is subtle difference between old space and new
+    // space here. Any way to avoid it? `step - 1` makes more sense as we would
+    // like to sample the object that straddles the `start + step` boundary.
+    // Rounding down further would introduce a small statistical error in
+    // sampling. However, presently PagedSpace requires limit to be aligned.
+    size_t rounded_step;
+    if (identity() == NEW_SPACE) {
+      DCHECK_GE(step, 1);
+      rounded_step = step - 1;
     } else {
-    // The entire node can be used as the linear allocation area.
-    return end;
+      rounded_step = RoundSizeDownToObjectAlignment(static_cast<int>(step));
     }
-}
-
-// TODO(ofrobots): refactor this code into SpaceWithLinearArea
-void PagedSpace::StartNextInlineAllocationStep() {
-  if (SupportsInlineAllocation() && AllocationObserversActive()) {
-    top_on_previous_step_ = top();
-    DecreaseLimit(ComputeLimit(top(), limit(), 0));
+    return Min(start + min_size + rounded_step, end);
   } else {
-    DCHECK_NULL(top_on_previous_step_);
+    // The entire node can be used as the linear allocation area.
+    return end;
   }
 }
 
@@ -2081,27 +2082,17 @@ void NewSpace::ResetAllocationInfo() {
   }
 }
 
-
-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 (!AllocationObserversActive()) {
-    DCHECK_NULL(top_on_previous_step_);
-    // Normal limit is the end of the current page.
-    allocation_info_.set_limit(to_space_.page_high());
-  } else {
-    // 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 + GetNextInlineAllocationStepSize() - 1;
-    allocation_info_.set_limit(Min(new_limit, high));
-  }
+void NewSpace::UpdateInlineAllocationLimit(size_t min_size) {
+  Address new_limit = ComputeLimit(top(), to_space_.page_high(), min_size);
+  allocation_info_.set_limit(new_limit);
   DCHECK_SEMISPACE_ALLOCATION_INFO(allocation_info_, to_space_);
 }
 
+void PagedSpace::UpdateInlineAllocationLimit(size_t min_size) {
+  Address new_limit = ComputeLimit(top(), limit(), min_size);
+  DCHECK_LE(new_limit, limit());
+  DecreaseLimit(new_limit);
+}
 
 bool NewSpace::AddFreshPage() {
   Address top = allocation_info_.top();
@@ -2160,8 +2151,7 @@ bool NewSpace::EnsureAllocation(int size_in_bytes,
   return true;
 }
 
-// TODO(ofrobots): refactor this code into SpaceWithLinearArea
-void NewSpace::StartNextInlineAllocationStep() {
+void SpaceWithLinearArea::StartNextInlineAllocationStep() {
   if (AllocationObserversActive()) {
     top_on_previous_step_ = top();
     UpdateInlineAllocationLimit(0);
@@ -2185,7 +2175,7 @@ void SpaceWithLinearArea::RemoveAllocationObserver(
   DCHECK_IMPLIES(top_on_previous_step_, AllocationObserversActive());
 }
 
-void NewSpace::PauseAllocationObservers() {
+void SpaceWithLinearArea::PauseAllocationObservers() {
   // Do a step to account for memory allocated so far.
   InlineAllocationStep(top(), nullptr, nullptr, 0);
   Space::PauseAllocationObservers();
@@ -2193,15 +2183,6 @@ void NewSpace::PauseAllocationObservers() {
   UpdateInlineAllocationLimit(0);
 }
 
-void PagedSpace::PauseAllocationObservers() {
-  // Do a step to account for memory allocated so far.
-  // TODO(ofrobots): Refactor into SpaceWithLinearArea. Note subtle difference
-  // from NewSpace version.
-  InlineAllocationStep(top(), nullptr, nullptr, 0);
-  Space::PauseAllocationObservers();
-  DCHECK_NULL(top_on_previous_step_);
-}
-
 void SpaceWithLinearArea::ResumeAllocationObservers() {
   DCHECK_NULL(top_on_previous_step_);
   Space::ResumeAllocationObservers();

src/heap/spaces.h

@@ -1988,6 +1988,23 @@ class SpaceWithLinearArea : public Space {
     return allocation_info_.limit_address();
   }
 
+  V8_EXPORT_PRIVATE void AddAllocationObserver(
+      AllocationObserver* observer) override;
+  V8_EXPORT_PRIVATE void RemoveAllocationObserver(
+      AllocationObserver* observer) override;
+  V8_EXPORT_PRIVATE void ResumeAllocationObservers() override;
+  V8_EXPORT_PRIVATE void PauseAllocationObservers() override;
+
+  // When allocation observers are active we may use a lower limit to allow the
+  // observers to 'interrupt' earlier than the natural limit. Given a linear
+  // area bounded by [start, end), this function computes the limit to use to
+  // allow proper observation based on existing observers. min_size specifies
+  // the minimum size that the limited area should have.
+  Address ComputeLimit(Address start, Address end, size_t min_size);
+  V8_EXPORT_PRIVATE virtual void UpdateInlineAllocationLimit(
+      size_t min_size) = 0;
+
+ protected:
   // If we are doing inline allocation in steps, this method performs the 'step'
   // operation. top is the memory address of the bump pointer at the last
   // inline allocation (i.e. it determines the numbers of bytes actually
@@ -1999,14 +2016,8 @@ class SpaceWithLinearArea : public Space {
   // Space::AllocationStep.
   void InlineAllocationStep(Address top, Address top_for_next_step,
                             Address soon_object, size_t size);
+  V8_EXPORT_PRIVATE void StartNextInlineAllocationStep() override;
 
-  V8_EXPORT_PRIVATE void AddAllocationObserver(
-      AllocationObserver* observer) override;
-  V8_EXPORT_PRIVATE void RemoveAllocationObserver(
-      AllocationObserver* observer) override;
-  V8_EXPORT_PRIVATE void ResumeAllocationObservers() override;
-
- protected:
   // TODO(ofrobots): make these private after refactoring is complete.
   AllocationInfo allocation_info_;
   Address top_on_previous_step_;
@@ -2125,8 +2136,6 @@ class V8_EXPORT_PRIVATE PagedSpace
 
   void ResetFreeList();
 
-  void PauseAllocationObservers() override;
-
   // Empty space allocation info, returning unused area to free list.
   void EmptyAllocationInfo();
 
@@ -2228,7 +2237,6 @@ class V8_EXPORT_PRIVATE PagedSpace
 
   std::unique_ptr<ObjectIterator> GetObjectIterator() override;
 
-  Address ComputeLimit(Address start, Address end, size_t size_in_bytes);
   void SetAllocationInfo(Address top, Address limit);
 
  private:
@@ -2240,7 +2248,7 @@ class V8_EXPORT_PRIVATE PagedSpace
     allocation_info_.Reset(top, limit);
   }
   void DecreaseLimit(Address new_limit);
-  void StartNextInlineAllocationStep() override;
+  void UpdateInlineAllocationLimit(size_t min_size) override;
   bool SupportsInlineAllocation() override {
     return identity() == OLD_SPACE && !is_local();
   }
@@ -2680,7 +2688,7 @@ class NewSpace : public SpaceWithLinearArea {
   // inline allocation every 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.
-  void UpdateInlineAllocationLimit(int size_in_bytes);
+  void UpdateInlineAllocationLimit(size_t size_in_bytes) override;
 
   // Get the extent of the inactive semispace (for use as a marking stack,
   // or to zap it). Notice: space-addresses are not necessarily on the
@@ -2737,8 +2745,6 @@ class NewSpace : public SpaceWithLinearArea {
 
   SemiSpace* active_space() { return &to_space_; }
 
-  void PauseAllocationObservers() override;
-
   iterator begin() { return to_space_.begin(); }
   iterator end() { return to_space_.end(); }
 
@@ -2768,7 +2774,6 @@ class NewSpace : public SpaceWithLinearArea {
 
   bool EnsureAllocation(int size_in_bytes, AllocationAlignment alignment);
   bool SupportsInlineAllocation() override { return true; }
-  void StartNextInlineAllocationStep() override;
 
   friend class SemiSpaceIterator;
 };