From ff58089c9f8520d539d4de7438838cfdfa4a081d Mon Sep 17 00:00:00 2001 From: "epenner@chromium.org" Date: Thu, 27 Mar 2014 03:06:14 +0000 Subject: [PATCH] GPU:'Pass' SharedMemory when possible. This avoids some duplication of shared memory handles just for the purposes of passing them around. Instead we Map immediately when the memory arrives in the process, and then 'Pass' the shared memory from there. BUG=177063, 353822 Review URL: https://codereview.chromium.org/211703003 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@259772 0039d316-1c4b-4281-b951-d872f2087c98 --- content/common/gpu/gpu_command_buffer_stub.cc | 13 ++- gpu/command_buffer/common/buffer.cc | 6 +- gpu/command_buffer/common/buffer.h | 2 +- .../service/command_buffer_service.cc | 13 ++- .../service/command_buffer_service.h | 2 +- .../service/transfer_buffer_manager.cc | 23 +---- .../service/transfer_buffer_manager.h | 14 +-- .../transfer_buffer_manager_unittest.cc | 85 +++++++++---------- mojo/services/gles2/command_buffer_impl.cc | 13 ++- 9 files changed, 85 insertions(+), 86 deletions(-) diff --git a/content/common/gpu/gpu_command_buffer_stub.cc b/content/common/gpu/gpu_command_buffer_stub.cc index 8bccd2abd2f667..ef6a45a2e1916e 100644 --- a/content/common/gpu/gpu_command_buffer_stub.cc +++ b/content/common/gpu/gpu_command_buffer_stub.cc @@ -686,9 +686,18 @@ void GpuCommandBufferStub::OnRegisterTransferBuffer( base::SharedMemoryHandle transfer_buffer, uint32 size) { TRACE_EVENT0("gpu", "GpuCommandBufferStub::OnRegisterTransferBuffer"); - base::SharedMemory shared_memory(transfer_buffer, false); + + // Take ownership of the memory and map it into this process. + // This validates the size. + scoped_ptr shared_memory( + new base::SharedMemory(transfer_buffer, false)); + if (!shared_memory->Map(size)) { + DVLOG(0) << "Failed to map shared memory."; + return; + } + if (command_buffer_) - command_buffer_->RegisterTransferBuffer(id, &shared_memory, size); + command_buffer_->RegisterTransferBuffer(id, shared_memory.Pass(), size); } void GpuCommandBufferStub::OnDestroyTransferBuffer(int32 id) { diff --git a/gpu/command_buffer/common/buffer.cc b/gpu/command_buffer/common/buffer.cc index f0ddbd0c873e73..5ba4e51ec01d0d 100644 --- a/gpu/command_buffer/common/buffer.cc +++ b/gpu/command_buffer/common/buffer.cc @@ -4,12 +4,16 @@ #include "gpu/command_buffer/common/buffer.h" +#include "base/logging.h" + namespace gpu { Buffer::Buffer(scoped_ptr shared_memory, size_t size) : shared_memory_(shared_memory.Pass()), memory_(shared_memory_->memory()), - size_(size) {} + size_(size) { + DCHECK(memory_) << "The memory must be mapped to create a Buffer"; +} Buffer::~Buffer() {} diff --git a/gpu/command_buffer/common/buffer.h b/gpu/command_buffer/common/buffer.h index af8073e1fe4c36..74d9705b555e60 100644 --- a/gpu/command_buffer/common/buffer.h +++ b/gpu/command_buffer/common/buffer.h @@ -17,7 +17,7 @@ namespace base { namespace gpu { -// Buffer/ThreadSafeBuffer own a piece of shared-memory of a certain size. +// Buffer owns a piece of shared-memory of a certain size. class GPU_EXPORT Buffer : public base::RefCountedThreadSafe { public: Buffer(scoped_ptr shared_memory, size_t size); diff --git a/gpu/command_buffer/service/command_buffer_service.cc b/gpu/command_buffer/service/command_buffer_service.cc index ec538c974d9036..58b96ac21d648e 100644 --- a/gpu/command_buffer/service/command_buffer_service.cc +++ b/gpu/command_buffer/service/command_buffer_service.cc @@ -123,14 +123,14 @@ scoped_refptr CommandBufferService::CreateTransferBuffer(size_t size, int32* id) { *id = -1; - SharedMemory buffer; - if (!buffer.CreateAnonymous(size)) + scoped_ptr shared_memory(new SharedMemory()); + if (!shared_memory->CreateAndMapAnonymous(size)) return NULL; static int32 next_id = 1; *id = next_id++; - if (!RegisterTransferBuffer(*id, &buffer, size)) { + if (!RegisterTransferBuffer(*id, shared_memory.Pass(), size)) { *id = -1; return NULL; } @@ -155,11 +155,10 @@ scoped_refptr CommandBufferService::GetTransferBuffer(int32 id) { bool CommandBufferService::RegisterTransferBuffer( int32 id, - base::SharedMemory* shared_memory, + scoped_ptr shared_memory, size_t size) { - return transfer_buffer_manager_->RegisterTransferBuffer(id, - shared_memory, - size); + return transfer_buffer_manager_->RegisterTransferBuffer( + id, shared_memory.Pass(), size); } void CommandBufferService::SetToken(int32 token) { diff --git a/gpu/command_buffer/service/command_buffer_service.h b/gpu/command_buffer/service/command_buffer_service.h index 07713fd2e0c02a..15c38dd4b5a288 100644 --- a/gpu/command_buffer/service/command_buffer_service.h +++ b/gpu/command_buffer/service/command_buffer_service.h @@ -65,7 +65,7 @@ class GPU_EXPORT CommandBufferService : public CommandBuffer { // to identify it in the command buffer. Callee dups the handle until // DestroyTransferBuffer is called. bool RegisterTransferBuffer(int32 id, - base::SharedMemory* shared_memory, + scoped_ptr shared_memory, size_t size); private: diff --git a/gpu/command_buffer/service/transfer_buffer_manager.cc b/gpu/command_buffer/service/transfer_buffer_manager.cc index 7130cb135a447f..7f81115b85b5c6 100644 --- a/gpu/command_buffer/service/transfer_buffer_manager.cc +++ b/gpu/command_buffer/service/transfer_buffer_manager.cc @@ -40,7 +40,7 @@ bool TransferBufferManager::Initialize() { bool TransferBufferManager::RegisterTransferBuffer( int32 id, - base::SharedMemory* shared_memory, + scoped_ptr shared_memory, size_t size) { if (id <= 0) { DVLOG(0) << "Cannot register transfer buffer with non-positive ID."; @@ -53,25 +53,8 @@ bool TransferBufferManager::RegisterTransferBuffer( return false; } - // Duplicate the handle. - base::SharedMemoryHandle duped_shared_memory_handle; - if (!shared_memory->ShareToProcess(base::GetCurrentProcessHandle(), - &duped_shared_memory_handle)) { - DVLOG(0) << "Failed to duplicate shared memory handle."; - return false; - } - scoped_ptr duped_shared_memory( - new SharedMemory(duped_shared_memory_handle, false)); - - // Map the shared memory into this process. This validates the size. - if (!duped_shared_memory->Map(size)) { - DVLOG(0) << "Failed to map shared memory."; - return false; - } - - // If it could be mapped register the shared memory with the ID. - scoped_refptr buffer = - make_scoped_refptr(new gpu::Buffer(duped_shared_memory.Pass(), size)); + // Register the shared memory with the ID. + scoped_refptr buffer = new gpu::Buffer(shared_memory.Pass(), size); // Check buffer alignment is sane. DCHECK(!(reinterpret_cast(buffer->memory()) & diff --git a/gpu/command_buffer/service/transfer_buffer_manager.h b/gpu/command_buffer/service/transfer_buffer_manager.h index b96cad4cf8ed5e..5f8f94679111cd 100644 --- a/gpu/command_buffer/service/transfer_buffer_manager.h +++ b/gpu/command_buffer/service/transfer_buffer_manager.h @@ -20,9 +20,10 @@ class GPU_EXPORT TransferBufferManagerInterface { public: virtual ~TransferBufferManagerInterface(); - virtual bool RegisterTransferBuffer(int32 id, - base::SharedMemory* shared_memory, - size_t size) = 0; + virtual bool RegisterTransferBuffer( + int32 id, + scoped_ptr shared_memory, + size_t size) = 0; virtual void DestroyTransferBuffer(int32 id) = 0; virtual scoped_refptr GetTransferBuffer(int32 id) = 0; }; @@ -33,9 +34,10 @@ class GPU_EXPORT TransferBufferManager TransferBufferManager(); bool Initialize(); - virtual bool RegisterTransferBuffer(int32 id, - base::SharedMemory* shared_memory, - size_t size) OVERRIDE; + virtual bool RegisterTransferBuffer( + int32 id, + scoped_ptr shared_memory, + size_t size) OVERRIDE; virtual void DestroyTransferBuffer(int32 id) OVERRIDE; virtual scoped_refptr GetTransferBuffer(int32 id) OVERRIDE; diff --git a/gpu/command_buffer/service/transfer_buffer_manager_unittest.cc b/gpu/command_buffer/service/transfer_buffer_manager_unittest.cc index 3f965e54b3c7d0..c7dc04a837e24c 100644 --- a/gpu/command_buffer/service/transfer_buffer_manager_unittest.cc +++ b/gpu/command_buffer/service/transfer_buffer_manager_unittest.cc @@ -17,17 +17,11 @@ const static size_t kBufferSize = 1024; class TransferBufferManagerTest : public testing::Test { protected: virtual void SetUp() { - for (size_t i = 0; i < arraysize(buffers_); ++i) { - buffers_[i].CreateAnonymous(kBufferSize); - buffers_[i].Map(kBufferSize); - } - TransferBufferManager* manager = new TransferBufferManager(); transfer_buffer_manager_.reset(manager); ASSERT_TRUE(manager->Initialize()); } - base::SharedMemory buffers_[3]; scoped_ptr transfer_buffer_manager_; }; @@ -44,30 +38,23 @@ TEST_F(TransferBufferManagerTest, OutOfRangeHandleMapsToNull) { } TEST_F(TransferBufferManagerTest, CanRegisterTransferBuffer) { - EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer(1, - &buffers_[0], - kBufferSize)); + scoped_ptr shm(new base::SharedMemory()); + shm->CreateAndMapAnonymous(kBufferSize); + base::SharedMemory* shm_raw_pointer = shm.get(); + EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer( + 1, shm.Pass(), kBufferSize)); scoped_refptr registered = transfer_buffer_manager_->GetTransferBuffer(1); - // Distinct memory range and shared memory handle from that originally - // registered. - scoped_refptr null_buffer; - EXPECT_NE(null_buffer, registered); - EXPECT_NE(buffers_[0].memory(), registered->memory()); - EXPECT_EQ(kBufferSize, registered->size()); - EXPECT_NE(&buffers_[0], registered->shared_memory()); - - // But maps to the same physical memory. - *static_cast(registered->memory()) = 7; - *static_cast(buffers_[0].memory()) = 8; - EXPECT_EQ(8, *static_cast(registered->memory())); + // Shared-memory ownership is transfered. It should be the same memory. + EXPECT_EQ(shm_raw_pointer, registered->shared_memory()); } TEST_F(TransferBufferManagerTest, CanDestroyTransferBuffer) { - EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer(1, - &buffers_[0], - kBufferSize)); + scoped_ptr shm(new base::SharedMemory()); + shm->CreateAndMapAnonymous(kBufferSize); + EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer( + 1, shm.Pass(), kBufferSize)); transfer_buffer_manager_->DestroyTransferBuffer(1); scoped_refptr registered = transfer_buffer_manager_->GetTransferBuffer(1); @@ -77,25 +64,31 @@ TEST_F(TransferBufferManagerTest, CanDestroyTransferBuffer) { } TEST_F(TransferBufferManagerTest, CannotRegregisterTransferBufferId) { - EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer(1, - &buffers_[0], - kBufferSize)); - EXPECT_FALSE(transfer_buffer_manager_->RegisterTransferBuffer(1, - &buffers_[0], - kBufferSize)); - EXPECT_FALSE(transfer_buffer_manager_->RegisterTransferBuffer(1, - &buffers_[1], - kBufferSize)); + scoped_ptr shm1(new base::SharedMemory()); + scoped_ptr shm2(new base::SharedMemory()); + scoped_ptr shm3(new base::SharedMemory()); + shm1->CreateAndMapAnonymous(kBufferSize); + shm2->CreateAndMapAnonymous(kBufferSize); + shm3->CreateAndMapAnonymous(kBufferSize); + + EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer( + 1, shm1.Pass(), kBufferSize)); + EXPECT_FALSE(transfer_buffer_manager_->RegisterTransferBuffer( + 1, shm2.Pass(), kBufferSize)); + EXPECT_FALSE(transfer_buffer_manager_->RegisterTransferBuffer( + 1, shm3.Pass(), kBufferSize)); } TEST_F(TransferBufferManagerTest, CanReuseTransferBufferIdAfterDestroying) { - EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer(1, - &buffers_[0], - kBufferSize)); + scoped_ptr shm1(new base::SharedMemory()); + scoped_ptr shm2(new base::SharedMemory()); + shm1->CreateAndMapAnonymous(kBufferSize); + shm2->CreateAndMapAnonymous(kBufferSize); + EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer( + 1, shm1.Pass(), kBufferSize)); transfer_buffer_manager_->DestroyTransferBuffer(1); - EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer(1, - &buffers_[1], - kBufferSize)); + EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer( + 1, shm2.Pass(), kBufferSize)); } TEST_F(TransferBufferManagerTest, DestroyUnusedTransferBufferIdDoesNotCrash) { @@ -103,15 +96,17 @@ TEST_F(TransferBufferManagerTest, DestroyUnusedTransferBufferIdDoesNotCrash) { } TEST_F(TransferBufferManagerTest, CannotRegisterNullTransferBuffer) { - EXPECT_FALSE(transfer_buffer_manager_->RegisterTransferBuffer(0, - &buffers_[0], - kBufferSize)); + scoped_ptr shm(new base::SharedMemory()); + shm->CreateAndMapAnonymous(kBufferSize); + EXPECT_FALSE(transfer_buffer_manager_->RegisterTransferBuffer( + 0, shm.Pass(), kBufferSize)); } TEST_F(TransferBufferManagerTest, CannotRegisterNegativeTransferBufferId) { - EXPECT_FALSE(transfer_buffer_manager_->RegisterTransferBuffer(-1, - &buffers_[0], - kBufferSize)); + scoped_ptr shm(new base::SharedMemory()); + shm->CreateAndMapAnonymous(kBufferSize); + EXPECT_FALSE(transfer_buffer_manager_->RegisterTransferBuffer( + -1, shm.Pass(), kBufferSize)); } } // namespace gpu diff --git a/mojo/services/gles2/command_buffer_impl.cc b/mojo/services/gles2/command_buffer_impl.cc index 71df2c5e7a9d9e..3fb9881a7a6580 100644 --- a/mojo/services/gles2/command_buffer_impl.cc +++ b/mojo/services/gles2/command_buffer_impl.cc @@ -143,9 +143,16 @@ void CommandBufferImpl::MakeProgress(int32_t last_get_offset) { void CommandBufferImpl::RegisterTransferBuffer(int32_t id, const ShmHandle& transfer_buffer, uint32_t size) { - bool read_only = false; - base::SharedMemory shared_memory(transfer_buffer, read_only); - command_buffer_->RegisterTransferBuffer(id, &shared_memory, size); + // Take ownership of the memory and map it into this process. + // This validates the size. + scoped_ptr shared_memory( + new base::SharedMemory(transfer_buffer, false)); + if (!shared_memory->Map(size)) { + DVLOG(0) << "Failed to map shared memory."; + return; + } + + command_buffer_->RegisterTransferBuffer(id, shared_memory.Pass(), size); } void CommandBufferImpl::DestroyTransferBuffer(int32_t id) {