Make sure that we grow the slots buffer when we are in evacuation scope.

src/mark-compact.h

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 238 matching lines @@
   int bottom_;
   int mask_;
   bool overflowed_;
 
   DISALLOW_COPY_AND_ASSIGN(MarkingDeque);
 };
 
 
 class SlotsBufferAllocator {
  public:
-  SlotsBuffer* AllocateBuffer(SlotsBuffer* next_buffer);
+  SlotsBuffer* AllocateBuffer(Heap* heap, SlotsBuffer* next_buffer);
   void DeallocateBuffer(SlotsBuffer* buffer);
-
   void DeallocateChain(SlotsBuffer** buffer_address);
 };
 
 
 // SlotsBuffer records a sequence of slots that has to be updated
 // after live objects were relocated from evacuation candidates.
 // All slots are either untyped or typed:
 //    - Untyped slots are expected to contain a tagged object pointer.
 //      They are recorded by an address.
 //    - Typed slots are expected to contain an encoded pointer to a heap
 //      object where the way of encoding depends on the type of the slot.
 //      They are recorded as a pair (SlotType, slot address).
 // We assume that zero-page is never mapped this allows us to distinguish
 // untyped slots from typed slots during iteration by a simple comparison:
 // if element of slots buffer is less than NUMBER_OF_SLOT_TYPES then it
 // is the first element of typed slot's pair.
 class SlotsBuffer {
  public:
   typedef Object** ObjectSlot;
 
-  explicit SlotsBuffer(SlotsBuffer* next_buffer)
-      : idx_(0), chain_length_(1), next_(next_buffer) {
+  SlotsBuffer(Heap* heap, SlotsBuffer* next_buffer)
+      : idx_(0), chain_length_(1), heap_(heap), next_(next_buffer) {
     if (next_ != NULL) {
       chain_length_ = next_->chain_length_ + 1;
     }
   }
 
   ~SlotsBuffer() {
   }
 
   void Add(ObjectSlot slot) {
     ASSERT(0 <= idx_ && idx_ < kNumberOfElements);
@@ skipping 23 matching lines @@
       case DEBUG_TARGET_SLOT:
         return "DEBUG_TARGET_SLOT";
       case JS_RETURN_SLOT:
         return "JS_RETURN_SLOT";
       case NUMBER_OF_SLOT_TYPES:
         return "NUMBER_OF_SLOT_TYPES";
     }
     return "UNKNOWN SlotType";
   }
 
-  void UpdateSlots(Heap* heap);
+  void UpdateSlots();
 

[Michael Starzinger, 2014/03/06 10:47:25]

nit: These two should be made protected IMHO.

-  void UpdateSlotsWithFilter(Heap* heap);
+  void UpdateSlotsWithFilter();
 
   SlotsBuffer* next() { return next_; }
 
   static int SizeOfChain(SlotsBuffer* buffer) {
     if (buffer == NULL) return 0;
     return static_cast<int>(buffer->idx_ +
                             (buffer->chain_length_ - 1) * kNumberOfElements);
   }
 
   inline bool IsFull() {
     return idx_ == kNumberOfElements;
   }
 
   inline bool HasSpaceForTypedSlot() {
     return idx_ < kNumberOfElements - 1;
   }
 
-  static void UpdateSlotsRecordedIn(Heap* heap,
-                                    SlotsBuffer* buffer,
-                                    bool code_slots_filtering_required) {
+  inline void UpdateSlotsRecordedIn(bool code_slots_filtering_required) {

[Michael Starzinger, 2014/03/06 10:47:25]

nit: The naming is a little bit weird now, because it no longer takes an
argument to which the "in" applies. Can we rename the method to
"UpdateRecordedSlots" instead?

+    SlotsBuffer* buffer = this;
     while (buffer != NULL) {
       if (code_slots_filtering_required) {
-        buffer->UpdateSlotsWithFilter(heap);
+        buffer->UpdateSlotsWithFilter();
       } else {
-        buffer->UpdateSlots(heap);
+        buffer->UpdateSlots();
       }
       buffer = buffer->next();
     }
   }
 
   enum AdditionMode {
     FAIL_ON_OVERFLOW,
     IGNORE_OVERFLOW
   };
 
   static bool ChainLengthThresholdReached(SlotsBuffer* buffer) {
     return buffer != NULL && buffer->chain_length_ >= kChainLengthThreshold;
   }
 
-  INLINE(static bool AddTo(SlotsBufferAllocator* allocator,
+  INLINE(static bool AddTo(Heap* heap,
+                           SlotsBufferAllocator* allocator,
                            SlotsBuffer** buffer_address,
                            ObjectSlot slot,
-                           AdditionMode mode)) {
-    SlotsBuffer* buffer = *buffer_address;
-    if (buffer == NULL || buffer->IsFull()) {
-      if (mode == FAIL_ON_OVERFLOW && ChainLengthThresholdReached(buffer)) {
-        allocator->DeallocateChain(buffer_address);
-        return false;
-      }
-      buffer = allocator->AllocateBuffer(buffer);
-      *buffer_address = buffer;
-    }
-    buffer->Add(slot);
-    return true;
-  }
+                           AdditionMode mode));
+
+  INLINE(static bool AddTo(Heap* heap,
+                           SlotsBufferAllocator* allocator,
+                           SlotsBuffer** buffer_address,
+                           SlotType type,
+                           Address addr,
+                           AdditionMode mode));
+
+  Heap* heap() const { return heap_; }
 
   static bool IsTypedSlot(ObjectSlot slot);
 
-  static bool AddTo(SlotsBufferAllocator* allocator,
-                    SlotsBuffer** buffer_address,
-                    SlotType type,
-                    Address addr,
-                    AdditionMode mode);
-
-  static const int kNumberOfElements = 1021;
+  static const int kNumberOfElements = 1020;

[ulan, 2014/03/05 15:40:18]

Why is this change to 1020 necessary?

[Hannes Payer (out of office), 2014/03/05 20:39:29]

The slots buffer should fit on a page. After adding one word (heap pointer), it
would overflow the page.

 
  private:
   static const int kChainLengthThreshold = 15;
 
   intptr_t idx_;
   intptr_t chain_length_;
+  Heap* heap_;
   SlotsBuffer* next_;
   ObjectSlot slots_[kNumberOfElements];
 };
 
 
 // CodeFlusher collects candidates for code flushing during marking and
 // processes those candidates after marking has completed in order to
 // reset those functions referencing code objects that would otherwise
 // be unreachable. Code objects can be referenced in three ways:
 //    - SharedFunctionInfo references unoptimized code.
@@ skipping 329 matching lines @@
   }
 
   // Mark the global table which maps weak objects to dependent code without
   // marking its contents.
   void MarkWeakObjectToCodeTable();
 
   // Special case for processing weak references in a full collection. We need
   // to artifically keep AllocationSites alive for a time.
   void MarkAllocationSite(AllocationSite* site);
 
+  void IncrementEvacuationScope() {
+    evacuation_scope_++;
+    ASSERT(evacuation_scope_ <= 1);

[Michael Starzinger, 2014/03/06 10:47:25]

These two ASSERTS combined basically enforce ...

ASSERT(evacuation_scope_ == 1);  // in IncrementEvacuationScope
ASSERT(evacuation_scope_ == 0);  // in DecrementEvacuationScope

Can we change it to this instead? That would be much clearer.

+  }
+
+  void DecrementEvacuationScope() {
+    evacuation_scope_--;
+    ASSERT(evacuation_scope_ >= 0);
+  }
+
+  bool IsInsideEvacuationScope() {
+    return evacuation_scope_ > 0;
+  }
+
  private:
   class SweeperTask;
 
   explicit MarkCompactCollector(Heap* heap);
   ~MarkCompactCollector();
 
   bool MarkInvalidatedCode();
   bool WillBeDeoptimized(Code* code);
   void RemoveDeadInvalidatedCode();
   void ProcessInvalidatedCode(ObjectVisitor* visitor);
@@ skipping 207 matching lines @@
   CodeFlusher* code_flusher_;
   Object* encountered_weak_collections_;
   bool have_code_to_deoptimize_;
 
   List<Page*> evacuation_candidates_;
   List<Code*> invalidated_code_;
 
   SmartPointer<FreeList> free_list_old_data_space_;
   SmartPointer<FreeList> free_list_old_pointer_space_;
 
+  int evacuation_scope_;
+
   friend class Heap;
 };
 
 
 class MarkBitCellIterator BASE_EMBEDDED {
  public:
   explicit MarkBitCellIterator(MemoryChunk* chunk)
       : chunk_(chunk) {
     last_cell_index_ = Bitmap::IndexToCell(
         Bitmap::CellAlignIndex(
@@ skipping 44 matching lines @@
 
   ~SequentialSweepingScope() {
     collector_->set_sequential_sweeping(false);
   }
 
  private:
   MarkCompactCollector* collector_;
 };
 
 
+class EvacuationScope V8_FINAL {

[Michael Starzinger, 2014/03/06 10:47:25]

High-level idea: This scope pretty much coincides with "state_ == SWEEP_SPACES",
would it make sense to use the state variable instead, or do you think a
separate scope brings additional advantages?

+ public:
+  explicit EvacuationScope(MarkCompactCollector* mark_compact) :
+      mark_compact_(mark_compact) {
+    mark_compact_->IncrementEvacuationScope();
+  }
+  ~EvacuationScope() {
+    mark_compact_->DecrementEvacuationScope();
+  }
+
+ private:
+  MarkCompactCollector* mark_compact_;
+};
+
+
 const char* AllocationSpaceName(AllocationSpace space);
 
 } }  // namespace v8::internal
 
 #endif  // V8_MARK_COMPACT_H_