Rework RingBuffer to assume the functionality of AlignedRingBuffer

AlignedRingBuffer used to be the only user of the RingBuffer class.

Merge the functionality of AlignedRingBuffer into RingBuffer, also
fixing the problem of introducing memory misalignment on a zero-byte
allocation.

Add test case for the zero-byte allocation case.

BUG=

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@267857 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 82 insertions, 248 deletions

diff --git a/gpu/command_buffer/client/ring_buffer.cc b/gpu/command_buffer/client/ring_buffer.cc
index 25f6342..813bb34 100644
--- a/gpu/command_buffer/client/ring_buffer.cc
+++ b/gpu/command_buffer/client/ring_buffer.cc
@@ -13,13 +13,16 @@
 
 namespace gpu {
 
-RingBuffer::RingBuffer(
-    Offset base_offset, unsigned int size, CommandBufferHelper* helper)
+RingBuffer::RingBuffer(unsigned int alignment, Offset base_offset,
+                       unsigned int size, CommandBufferHelper* helper,
+                       void* base)
     : helper_(helper),
       base_offset_(base_offset),
       size_(size),
       free_offset_(0),
-      in_use_offset_(0) {
+      in_use_offset_(0),
+      alignment_(alignment),
+      base_(static_cast<int8*>(base) - base_offset) {
 }
 
 RingBuffer::~RingBuffer() {
@@ -49,13 +52,16 @@ void RingBuffer::FreeOldestBlock() {
   blocks_.pop_front();
 }
 
-RingBuffer::Offset RingBuffer::Alloc(unsigned int size) {
+void* RingBuffer::Alloc(unsigned int size) {
   DCHECK_LE(size, size_) << "attempt to allocate more than maximum memory";
   DCHECK(blocks_.empty() || blocks_.back().state != IN_USE)
       << "Attempt to alloc another block before freeing the previous.";
   // Similarly to malloc, an allocation of 0 allocates at least 1 byte, to
   // return different pointers every time.
   if (size == 0) size = 1;
+  // Allocate rounded to alignment size so that the offsets are always
+  // memory-aligned.
+  size = RoundToAlignment(size);
 
   // Wait until there is enough room.
   while (size > GetLargestFreeSizeNoWaiting()) {
@@ -74,11 +80,12 @@ RingBuffer::Offset RingBuffer::Alloc(unsigned int size) {
   if (free_offset_ == size_) {
     free_offset_ = 0;
   }
-  return offset + base_offset_;
+  return GetPointer(offset + base_offset_);
 }
 
-void RingBuffer::FreePendingToken(RingBuffer::Offset offset,
+void RingBuffer::FreePendingToken(void* pointer,
                                   unsigned int token) {
+  Offset offset = GetOffset(pointer);
   offset -= base_offset_;
   DCHECK(!blocks_.empty()) << "no allocations to free";
   for (Container::reverse_iterator it = blocks_.rbegin();
diff --git a/gpu/command_buffer/client/ring_buffer.h b/gpu/command_buffer/client/ring_buffer.h
index 81f1cdd..3683707 100644
--- a/gpu/command_buffer/client/ring_buffer.h
+++ b/gpu/command_buffer/client/ring_buffer.h
@@ -25,11 +25,13 @@ class GPU_EXPORT RingBuffer {
 
   // Creates a RingBuffer.
   // Parameters:
+  //   alignment: Alignment for allocations.
   //   base_offset: The offset of the start of the buffer.
   //   size: The size of the buffer in bytes.
   //   helper: A CommandBufferHelper for dealing with tokens.
-  RingBuffer(
-      Offset base_offset, unsigned int size, CommandBufferHelper* helper);
+  //   base: The physical address that corresponds to base_offset.
+  RingBuffer(unsigned int alignment, Offset base_offset,
+             unsigned int size, CommandBufferHelper* helper, void* base);
 
   ~RingBuffer();
 
@@ -41,16 +43,16 @@ class GPU_EXPORT RingBuffer {
   //   size: the size of the memory block to allocate.
   //
   // Returns:
-  //   the offset of the allocated memory block.
-  Offset Alloc(unsigned int size);
+  //   the pointer to the allocated memory block.
+  void* Alloc(unsigned int size);
 
   // Frees a block of memory, pending the passage of a token. That memory won't
   // be re-allocated until the token has passed through the command stream.
   //
   // Parameters:
-  //   offset: the offset of the memory block to free.
+  //   pointer: the pointer to the memory block to free.
   //   token: the token value to wait for before re-using the memory.
-  void FreePendingToken(Offset offset, unsigned int token);
+  void FreePendingToken(void* pointer, unsigned int token);
 
   // Gets the size of the largest free block that is available without waiting.
   unsigned int GetLargestFreeSizeNoWaiting();
@@ -62,6 +64,22 @@ class GPU_EXPORT RingBuffer {
     return size_;
   }
 
+  // Gets a pointer to a memory block given the base memory and the offset.
+  void* GetPointer(RingBuffer::Offset offset) const {
+    return static_cast<int8*>(base_) + offset;
+  }
+
+  // Gets the offset to a memory block given the base memory and the address.
+  RingBuffer::Offset GetOffset(void* pointer) const {
+    return static_cast<int8*>(pointer) - static_cast<int8*>(base_);
+  }
+
+  // Rounds the given size to the alignment in use.
+  unsigned int RoundToAlignment(unsigned int size) {
+    return (size + alignment_ - 1) & ~(alignment_ - 1);
+  }
+
+
  private:
   enum State {
     IN_USE,
@@ -105,92 +123,13 @@ class GPU_EXPORT RingBuffer {
   // Range between in_use_mark and free_mark is in use.
   Offset in_use_offset_;
 
-  DISALLOW_IMPLICIT_CONSTRUCTORS(RingBuffer);
-};
-
-// This class functions just like RingBuffer, but its API uses pointers
-// instead of offsets.
-class RingBufferWrapper {
- public:
-  // Parameters:
-  //   base_offset: The offset to the start of the buffer
-  //   size: The size of the buffer in bytes.
-  //   helper: A CommandBufferHelper for dealing with tokens.
-  //   base: The physical address that corresponds to base_offset.
-  RingBufferWrapper(RingBuffer::Offset base_offset,
-                    unsigned int size,
-                    CommandBufferHelper* helper,
-                    void* base)
-      : allocator_(base_offset, size, helper),
-        base_(static_cast<int8*>(base) - base_offset) {
-  }
-
-  // Allocates a block of memory. If the buffer is out of directly available
-  // memory, this function may wait until memory that was freed "pending a
-  // token" can be re-used.
-  //
-  // Parameters:
-  //   size: the size of the memory block to allocate.
-  //
-  // Returns:
-  //   the pointer to the allocated memory block, or NULL if out of
-  //   memory.
-  void* Alloc(unsigned int size) {
-    RingBuffer::Offset offset = allocator_.Alloc(size);
-    return GetPointer(offset);
-  }
+  // Alignment for allocations.
+  unsigned int alignment_;
 
-  // Allocates a block of memory. If the buffer is out of directly available
-  // memory, this function may wait until memory that was freed "pending a
-  // token" can be re-used.
-  // This is a type-safe version of Alloc, returning a typed pointer.
-  //
-  // Parameters:
-  //   count: the number of elements to allocate.
-  //
-  // Returns:
-  //   the pointer to the allocated memory block, or NULL if out of
-  //   memory.
-  template <typename T> T* AllocTyped(unsigned int count) {
-    return static_cast<T*>(Alloc(count * sizeof(T)));
-  }
-
-  // Frees a block of memory, pending the passage of a token. That memory won't
-  // be re-allocated until the token has passed through the command stream.
-  //
-  // Parameters:
-  //   pointer: the pointer to the memory block to free.
-  //   token: the token value to wait for before re-using the memory.
-  void FreePendingToken(void* pointer, unsigned int token) {
-    DCHECK(pointer);
-    allocator_.FreePendingToken(GetOffset(pointer), token);
-  }
-
-  // Gets a pointer to a memory block given the base memory and the offset.
-  void* GetPointer(RingBuffer::Offset offset) const {
-    return static_cast<int8*>(base_) + offset;
-  }
-
-  // Gets the offset to a memory block given the base memory and the address.
-  RingBuffer::Offset GetOffset(void* pointer) const {
-    return static_cast<int8*>(pointer) - static_cast<int8*>(base_);
-  }
-
-  // Gets the size of the largest free block that is available without waiting.
-  unsigned int GetLargestFreeSizeNoWaiting() {
-    return allocator_.GetLargestFreeSizeNoWaiting();
-  }
-
-  // Gets the size of the largest free block that can be allocated if the
-  // caller can wait.
-  unsigned int GetLargestFreeOrPendingSize() {
-    return allocator_.GetLargestFreeOrPendingSize();
-  }
-
- private:
-  RingBuffer allocator_;
+  // The physical address that corresponds to base_offset.
   void* base_;
-  DISALLOW_IMPLICIT_CONSTRUCTORS(RingBufferWrapper);
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(RingBuffer);
 };
 
 }  // namespace gpu
diff --git a/gpu/command_buffer/client/ring_buffer_test.cc b/gpu/command_buffer/client/ring_buffer_test.cc
index e46ec43c..0b58e3a 100644
--- a/gpu/command_buffer/client/ring_buffer_test.cc
+++ b/gpu/command_buffer/client/ring_buffer_test.cc
@@ -35,6 +35,7 @@ class BaseRingBufferTest : public testing::Test {
  protected:
   static const unsigned int kBaseOffset = 128;
   static const unsigned int kBufferSize = 1024;
+  static const unsigned int kAlignment = 4;
 
   void RunPendingSetToken() {
     for (std::vector<const void*>::iterator it = set_token_arguments_.begin();
@@ -107,6 +108,8 @@ class BaseRingBufferTest : public testing::Test {
   std::vector<const void*> set_token_arguments_;
   bool delay_set_token_;
 
+  scoped_ptr<int8[]> buffer_;
+  int8* buffer_start_;
 };
 
 #ifndef _MSC_VER
@@ -122,7 +125,11 @@ class RingBufferTest : public BaseRingBufferTest {
  protected:
   virtual void SetUp() {
     BaseRingBufferTest::SetUp();
-    allocator_.reset(new RingBuffer(kBaseOffset, kBufferSize, helper_.get()));
+
+    buffer_.reset(new int8[kBufferSize + kBaseOffset]);
+    buffer_start_ = buffer_.get() + kBaseOffset;
+    allocator_.reset(new RingBuffer(kAlignment, kBaseOffset, kBufferSize,
+                                    helper_.get(), buffer_start_));
   }
 
   virtual void TearDown() {
@@ -140,12 +147,12 @@ TEST_F(RingBufferTest, TestBasic) {
   const unsigned int kSize = 16;
   EXPECT_EQ(kBufferSize, allocator_->GetLargestFreeOrPendingSize());
   EXPECT_EQ(kBufferSize, allocator_->GetLargestFreeSizeNoWaiting());
-  RingBuffer::Offset offset = allocator_->Alloc(kSize);
-  EXPECT_GE(kBufferSize, offset - kBaseOffset + kSize);
+  void* pointer = allocator_->Alloc(kSize);
+  EXPECT_GE(kBufferSize, allocator_->GetOffset(pointer) - kBaseOffset + kSize);
   EXPECT_EQ(kBufferSize, allocator_->GetLargestFreeOrPendingSize());
   EXPECT_EQ(kBufferSize - kSize, allocator_->GetLargestFreeSizeNoWaiting());
   int32 token = helper_->InsertToken();
-  allocator_->FreePendingToken(offset, token);
+  allocator_->FreePendingToken(pointer, token);
 }
 
 // Checks the free-pending-token mechanism.
@@ -158,10 +165,11 @@ TEST_F(RingBufferTest, TestFreePendingToken) {
   // Allocate several buffers to fill in the memory.
   int32 tokens[kAllocCount];
   for (unsigned int ii = 0; ii < kAllocCount; ++ii) {
-    RingBuffer::Offset offset = allocator_->Alloc(kSize);
-    EXPECT_GE(kBufferSize, offset - kBaseOffset + kSize);
+    void* pointer = allocator_->Alloc(kSize);
+    EXPECT_GE(kBufferSize,
+              allocator_->GetOffset(pointer) - kBaseOffset + kSize);
     tokens[ii] = helper_->InsertToken();
-    allocator_->FreePendingToken(offset, tokens[ii]);
+    allocator_->FreePendingToken(pointer, tokens[ii]);
   }
 
   EXPECT_EQ(kBufferSize - (kSize * kAllocCount),
@@ -171,131 +179,38 @@ TEST_F(RingBufferTest, TestFreePendingToken) {
 
   // This allocation will need to reclaim the space freed above, so that should
   // process the commands until a token is passed.
-  RingBuffer::Offset offset1 = allocator_->Alloc(kSize);
-  EXPECT_EQ(kBaseOffset, offset1);
+  void* pointer1 = allocator_->Alloc(kSize);
+  EXPECT_EQ(kBaseOffset, allocator_->GetOffset(pointer1));
 
   // Check that the token has indeed passed.
   EXPECT_LE(tokens[0], GetToken());
 
-  allocator_->FreePendingToken(offset1, helper_->InsertToken());
+  allocator_->FreePendingToken(pointer1, helper_->InsertToken());
 }
 
 // Tests GetLargestFreeSizeNoWaiting
 TEST_F(RingBufferTest, TestGetLargestFreeSizeNoWaiting) {
   EXPECT_EQ(kBufferSize, allocator_->GetLargestFreeSizeNoWaiting());
 
-  RingBuffer::Offset offset = allocator_->Alloc(kBufferSize);
+  void* pointer = allocator_->Alloc(kBufferSize);
   EXPECT_EQ(0u, allocator_->GetLargestFreeSizeNoWaiting());
-  allocator_->FreePendingToken(offset, helper_->InsertToken());
+  allocator_->FreePendingToken(pointer, helper_->InsertToken());
 }
 
 TEST_F(RingBufferTest, TestFreeBug) {
   // The first and second allocations must not match.
-  const unsigned int kAlloc1 = 10;
+  const unsigned int kAlloc1 = 3*kAlignment;
   const unsigned int kAlloc2 = 20;
-  RingBuffer::Offset offset = allocator_->Alloc(kAlloc1);
+  void* pointer = allocator_->Alloc(kAlloc1);
   EXPECT_EQ(kBufferSize - kAlloc1, allocator_->GetLargestFreeSizeNoWaiting());
-  allocator_->FreePendingToken(offset, helper_.get()->InsertToken());
-  offset = allocator_->Alloc(kAlloc2);
+  allocator_->FreePendingToken(pointer, helper_.get()->InsertToken());
+  pointer = allocator_->Alloc(kAlloc2);
   EXPECT_EQ(kBufferSize - kAlloc1 - kAlloc2,
             allocator_->GetLargestFreeSizeNoWaiting());
-  allocator_->FreePendingToken(offset, helper_.get()->InsertToken());
-  offset = allocator_->Alloc(kBufferSize);
+  allocator_->FreePendingToken(pointer, helper_.get()->InsertToken());
+  pointer = allocator_->Alloc(kBufferSize);
   EXPECT_EQ(0u, allocator_->GetLargestFreeSizeNoWaiting());
-  allocator_->FreePendingToken(offset, helper_.get()->InsertToken());
-}
-
-// Test fixture for RingBufferWrapper test - Creates a
-// RingBufferWrapper, using a CommandBufferHelper with a mock
-// AsyncAPIInterface for its interface (calling it directly, not through the
-// RPC mechanism), making sure Noops are ignored and SetToken are properly
-// forwarded to the engine.
-class RingBufferWrapperTest : public BaseRingBufferTest {
- protected:
-  virtual void SetUp() {
-    BaseRingBufferTest::SetUp();
-
-    // Though allocating this buffer isn't strictly necessary, it makes
-    // allocations point to valid addresses, so they could be used for
-    // something.
-    buffer_.reset(new int8[kBufferSize + kBaseOffset]);
-    buffer_start_ = buffer_.get() + kBaseOffset;
-    allocator_.reset(new RingBufferWrapper(
-        kBaseOffset, kBufferSize, helper_.get(), buffer_start_));
-  }
-
-  virtual void TearDown() {
-    // If the GpuScheduler posts any tasks, this forces them to run.
-    base::MessageLoop::current()->RunUntilIdle();
-
-    BaseRingBufferTest::TearDown();
-  }
-
-  scoped_ptr<RingBufferWrapper> allocator_;
-  scoped_ptr<int8[]> buffer_;
-  int8* buffer_start_;
-};
-
-// Checks basic alloc and free.
-TEST_F(RingBufferWrapperTest, TestBasic) {
-  const unsigned int kSize = 16;
-  void* pointer = allocator_->Alloc(kSize);
-  ASSERT_TRUE(pointer);
-  EXPECT_LE(buffer_start_, static_cast<int8*>(pointer));
-  EXPECT_GE(kBufferSize, static_cast<int8*>(pointer) - buffer_start_ + kSize);
-
-  allocator_->FreePendingToken(pointer, helper_->InsertToken());
-
-  int8* pointer_int8 = allocator_->AllocTyped<int8>(kSize);
-  ASSERT_TRUE(pointer_int8);
-  EXPECT_LE(buffer_start_, pointer_int8);
-  EXPECT_GE(buffer_start_ + kBufferSize, pointer_int8 + kSize);
-  allocator_->FreePendingToken(pointer_int8, helper_->InsertToken());
-
-  unsigned int* pointer_uint = allocator_->AllocTyped<unsigned int>(kSize);
-  ASSERT_TRUE(pointer_uint);
-  EXPECT_LE(buffer_start_, reinterpret_cast<int8*>(pointer_uint));
-  EXPECT_GE(buffer_start_ + kBufferSize,
-            reinterpret_cast<int8* >(pointer_uint + kSize));
-
-  // Check that it did allocate kSize * sizeof(unsigned int). We can't tell
-  // directly, except from the remaining size.
-  EXPECT_EQ(kBufferSize - kSize - kSize - kSize * sizeof(*pointer_uint),
-            allocator_->GetLargestFreeSizeNoWaiting());
-  allocator_->FreePendingToken(pointer_uint, helper_->InsertToken());
-}
-
-// Checks the free-pending-token mechanism.
-TEST_F(RingBufferWrapperTest, TestFreePendingToken) {
-  const unsigned int kSize = 16;
-  const unsigned int kAllocCount = kBufferSize / kSize;
-  CHECK(kAllocCount * kSize == kBufferSize);
-
-  delay_set_token_ = true;
-  // Allocate several buffers to fill in the memory.
-  int32 tokens[kAllocCount];
-  for (unsigned int ii = 0; ii < kAllocCount; ++ii) {
-    void* pointer = allocator_->Alloc(kSize);
-    EXPECT_TRUE(pointer != NULL);
-    tokens[ii] = helper_->InsertToken();
-    allocator_->FreePendingToken(pointer, helper_->InsertToken());
-  }
-
-  EXPECT_EQ(kBufferSize - (kSize * kAllocCount),
-            allocator_->GetLargestFreeSizeNoWaiting());
-
-  RunPendingSetToken();
-
-  // This allocation will need to reclaim the space freed above, so that should
-  // process the commands until the token is passed.
-  void* pointer1 = allocator_->Alloc(kSize);
-  EXPECT_EQ(buffer_start_, static_cast<int8*>(pointer1));
-
-  // Check that the token has indeed passed.
-  EXPECT_LE(tokens[0], GetToken());
-
-  allocator_->FreePendingToken(pointer1, helper_->InsertToken());
-  EXPECT_LE(command_buffer_->GetLastState().token, helper_->InsertToken());
+  allocator_->FreePendingToken(pointer, helper_.get()->InsertToken());
 }
 
 }  // namespace gpu
diff --git a/gpu/command_buffer/client/transfer_buffer.cc b/gpu/command_buffer/client/transfer_buffer.cc
index 70c4bf0..c5ce3d5 100644
--- a/gpu/command_buffer/client/transfer_buffer.cc
+++ b/gpu/command_buffer/client/transfer_buffer.cc
@@ -13,9 +13,6 @@
 
 namespace gpu {
 
-AlignedRingBuffer::~AlignedRingBuffer() {
-}
-
 TransferBuffer::TransferBuffer(
     CommandBufferHelper* helper)
     : helper_(helper),
@@ -92,9 +89,8 @@ void TransferBuffer::AllocateRingBuffer(unsigned int size) {
     if (id != -1) {
       DCHECK(buffer);
       buffer_ = buffer;
-      ring_buffer_.reset(new AlignedRingBuffer(
+      ring_buffer_.reset(new RingBuffer(
           alignment_,
-          id,
           result_size_,
           buffer_->size() - result_size_,
           helper_,
diff --git a/gpu/command_buffer/client/transfer_buffer.h b/gpu/command_buffer/client/transfer_buffer.h
index 28971d7..348ad32 100644
--- a/gpu/command_buffer/client/transfer_buffer.h
+++ b/gpu/command_buffer/client/transfer_buffer.h
@@ -16,40 +16,6 @@ namespace gpu {
 
 class CommandBufferHelper;
 
-// Wraps RingBufferWrapper to provide aligned allocations.
-class AlignedRingBuffer : public RingBufferWrapper {
- public:
-  AlignedRingBuffer(
-      unsigned int alignment,
-      int32 shm_id,
-      RingBuffer::Offset base_offset,
-      unsigned int size,
-      CommandBufferHelper* helper,
-      void* base)
-      : RingBufferWrapper(base_offset, size, helper, base),
-        alignment_(alignment),
-        shm_id_(shm_id) {
-  }
-  ~AlignedRingBuffer();
-
-  // Hiding Alloc from RingBufferWrapper
-  void* Alloc(unsigned int size) {
-    return RingBufferWrapper::Alloc(RoundToAlignment(size));
-  }
-
-  int32 GetShmId() const {
-    return shm_id_;
-  }
-
- private:
-  unsigned int RoundToAlignment(unsigned int size) {
-    return (size + alignment_ - 1) & ~(alignment_ - 1);
-  }
-
-  unsigned int alignment_;
-  int32 shm_id_;
-};
-
 // Interface for managing the transfer buffer.
 class GPU_EXPORT TransferBufferInterface {
  public:
@@ -120,7 +86,7 @@ class GPU_EXPORT TransferBuffer : public TransferBufferInterface {
   void AllocateRingBuffer(unsigned int size);
 
   CommandBufferHelper* helper_;
-  scoped_ptr<AlignedRingBuffer> ring_buffer_;
+  scoped_ptr<RingBuffer> ring_buffer_;
 
   // size reserved for results
   unsigned int result_size_;
diff --git a/gpu/command_buffer/client/transfer_buffer_unittest.cc b/gpu/command_buffer/client/transfer_buffer_unittest.cc
index 7201477..9f3b51e 100644
--- a/gpu/command_buffer/client/transfer_buffer_unittest.cc
+++ b/gpu/command_buffer/client/transfer_buffer_unittest.cc
@@ -181,6 +181,17 @@ TEST_F(TransferBufferTest, TooLargeAllocation) {
   transfer_buffer_->FreePendingToken(ptr, 1);
 }
 
+TEST_F(TransferBufferTest, MemoryAlignmentAfterZeroAllocation) {
+  Initialize(32u);
+  void* ptr = transfer_buffer_->Alloc(0);
+  EXPECT_EQ((reinterpret_cast<uintptr_t>(ptr) & (kAlignment - 1)), 0u);
+  transfer_buffer_->FreePendingToken(ptr, -1);
+  // Check that the pointer is aligned on the following allocation.
+  ptr = transfer_buffer_->Alloc(4);
+  EXPECT_EQ((reinterpret_cast<uintptr_t>(ptr) & (kAlignment - 1)), 0u);
+  transfer_buffer_->FreePendingToken(ptr, 1);
+}
+
 TEST_F(TransferBufferTest, Flush) {
   Initialize(16u);
   unsigned int size_allocated = 0;