Distinct block sizes for StoreBuffer/MarkingStack.

runtime/vm/store_buffer.h

Index: runtime/vm/store_buffer.h
diff --git a/runtime/vm/store_buffer.h b/runtime/vm/store_buffer.h
index d6fcb7d7678c3430c2368d4ceb8480513bda2433..d49d82e62a37a4ca79f2ea7e75d1cd80ef025846 100644
--- a/runtime/vm/store_buffer.h
+++ b/runtime/vm/store_buffer.h
@@ -16,17 +16,17 @@ class Mutex;
 class RawObject;
 
 // A set of RawObject*. Must be emptied before destruction (using Pop/Reset).
-class StoreBufferBlock {
+template<int Size>
+class PointerBlock {
  public:
-  // Each full block contains kSize pointers.
-  static const int32_t kSize = 1024;
+  enum { kSize = Size };
 
   void Reset() {
     top_ = 0;
     next_ = NULL;
   }
 
-  StoreBufferBlock* next() const { return next_; }
+  PointerBlock<Size>* next() const { return next_; }
 
   intptr_t Count() const { return top_; }
   bool IsFull() const { return Count() == kSize; }
@@ -53,73 +53,71 @@ class StoreBufferBlock {
   }
 #endif  // TESTING
 
-  static intptr_t top_offset() { return OFFSET_OF(StoreBufferBlock, top_); }
+  static intptr_t top_offset() { return OFFSET_OF(PointerBlock<Size>, top_); }
   static intptr_t pointers_offset() {
-    return OFFSET_OF(StoreBufferBlock, pointers_);
+    return OFFSET_OF(PointerBlock<Size>, pointers_);
   }
 
  private:
-  StoreBufferBlock() : next_(NULL), top_(0) {}
-  ~StoreBufferBlock() {
+  PointerBlock() : next_(NULL), top_(0) {}
+  ~PointerBlock() {
     ASSERT(IsEmpty());  // Guard against unintentionally discarding pointers.
   }
 
-  StoreBufferBlock* next_;
+  PointerBlock<Size>* next_;
   int32_t top_;
   RawObject* pointers_[kSize];
 
-  friend class StoreBuffer;
+  template<int> friend class BlockStack;
 
-  DISALLOW_COPY_AND_ASSIGN(StoreBufferBlock);
+  DISALLOW_COPY_AND_ASSIGN(PointerBlock);
 };
 
 
-class StoreBuffer {
+template<int BlockSize>
+class BlockStack {
  public:
-  StoreBuffer();
-  ~StoreBuffer();
+  typedef PointerBlock<BlockSize> Block;
+
+  BlockStack();
+  ~BlockStack();
   static void InitOnce();
   static void ShutDown();
 
-  // Interrupt when crossing this threshold of non-empty blocks in the buffer.
-  static const intptr_t kMaxNonEmpty = 100;
-
-  // Adds and transfers ownership of the block to the buffer.
-  void PushBlock(StoreBufferBlock* block, bool check_threshold = true);
   // Partially filled blocks can be reused, and there is an "inifite" supply
   // of empty blocks (reused or newly allocated). In any case, the caller
   // takes ownership of the returned block.
-  StoreBufferBlock* PopNonFullBlock();
-  StoreBufferBlock* PopEmptyBlock();
-  StoreBufferBlock* PopNonEmptyBlock();
+  Block* PopNonFullBlock();
+  Block* PopEmptyBlock();
+  Block* PopNonEmptyBlock();
 
   // Pops and returns all non-empty blocks as a linked list (owned by caller).
-  StoreBufferBlock* Blocks();
+  Block* Blocks();
 
-  // Discards the contents of this store buffer.
+  // Discards the contents of all non-empty blocks.
   void Reset();
 
-  // Check whether non-empty blocks have exceeded kMaxNonEmpty.
-  bool Overflowed();
-
   bool IsEmpty();
 
- private:
+ protected:
   class List {
    public:
     List() : head_(NULL), length_(0) {}
     ~List();
-    void Push(StoreBufferBlock* block);
-    StoreBufferBlock* Pop();
+    void Push(Block* block);
+    Block* Pop();
     intptr_t length() const { return length_; }
     bool IsEmpty() const { return head_ == NULL; }
-    StoreBufferBlock* PopAll();
+    Block* PopAll();
    private:
-    StoreBufferBlock* head_;
+    Block* head_;
     intptr_t length_;
     DISALLOW_COPY_AND_ASSIGN(List);
   };
 
+  // Adds and transfers ownership of the block to the buffer.
+  void PushBlockImpl(Block* block);
+
   // If needed, trims the the global cache of empty blocks.
   static void TrimGlobalEmpty();
 
@@ -131,9 +129,41 @@ class StoreBuffer {
   static List* global_empty_;
   static Mutex* global_mutex_;

[Cutch, 2015/09/17 18:04:06]

Will it always be true that blocks of the same size also share the same list and
mutex? That is, will we ever need two distinct lists of PointerBlock<100>? This
assumption should be documented somewhere too.

[koda, 2015/09/17 19:13:18]

Yes, the cache of empty blocks will be shared if we ever have two types with
matching block size, which I think is most likely what we want: being empty,
they are perfectly shareable, and if lock contention is an issue, we should
probably just add per-thread caches (still based on block size) rather than base
it on the type.

But a good idea to document it; added comments to class.

 
-  DISALLOW_COPY_AND_ASSIGN(StoreBuffer);
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BlockStack);
 };
 
+
+static const int kStoreBufferBlockSize = 1024;
+class StoreBuffer : public BlockStack<kStoreBufferBlockSize> {
+ public:
+  // Interrupt when crossing this threshold of non-empty blocks in the buffer.
+  static const intptr_t kMaxNonEmpty = 100;
+
+  // Adds and transfers ownership of the block to the buffer. Optionally
+  // checks the number of non-empty blocks for overflow, and schedules an
+  // interrupt on the current isolate if so.
+  void PushBlock(Block* block, bool check_threshold);

[Cutch, 2015/09/17 18:04:05]

Instead of bool check_threshold could we have something like:

enum {
  kCheckThreshold,
  kIgnoreThreshold
}

Makes the code passing ", false" easier to understand when you see ",
StoreBuffer::kIgnoreThreshold"

[koda, 2015/09/17 19:13:18]

Done.

+
+  // Check whether non-empty blocks have exceeded kMaxNonEmpty (but takes no
+  // action).
+  bool Overflowed();
+};
+
+
+typedef StoreBuffer::Block StoreBufferBlock;
+
+
+static const int kMarkingStackBlockSize = 64;
+class MarkingStack : public BlockStack<kMarkingStackBlockSize> {
+ public:
+  // Adds and transfers ownership of the block to the buffer.
+  void PushBlock(Block* block) {
+    BlockStack<Block::kSize>::PushBlockImpl(block);
+  }
+};
+
+
 }  // namespace dart
 
 #endif  // VM_STORE_BUFFER_H_