Fix GpuMemoryBuffer memory traces

InProcessGpuMemoryBufferManager wasn't providing memory dumps for
GpuMemoryBuffers it allocated. Any GMBs allocated for the GPU process
weren't accounted for in memory traces. Share the implementation with
HostGpuMemoryBufferManager by refactoring it into AllocateBufferInfo
class.

Bug: none
Change-Id: Ieaf1597106bee2562b0fc99a47243f55af7faf5d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1894457
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Jonathan Backer <backer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713172}

components/viz/host/host_gpu_memory_buffer_manager.cc

@@ -8,7 +8,6 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
-#include "base/strings/stringprintf.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "gpu/ipc/common/gpu_memory_buffer_impl.h"
@@ -37,11 +36,6 @@ HostGpuMemoryBufferManager::PendingBufferInfo::PendingBufferInfo(
     PendingBufferInfo&&) = default;
 HostGpuMemoryBufferManager::PendingBufferInfo::~PendingBufferInfo() = default;
 
-HostGpuMemoryBufferManager::AllocatedBufferInfo::AllocatedBufferInfo() =
-    default;
-HostGpuMemoryBufferManager::AllocatedBufferInfo::~AllocatedBufferInfo() =
-    default;
-
 HostGpuMemoryBufferManager::HostGpuMemoryBufferManager(
     GpuServiceProvider gpu_service_provider,
     int client_id,
@@ -75,8 +69,8 @@ void HostGpuMemoryBufferManager::DestroyGpuMemoryBuffer(
   auto buffer_iter = buffers.find(id);
   if (buffer_iter == buffers.end())
     return;
-  DCHECK_NE(gfx::EMPTY_BUFFER, buffer_iter->second.type);
-  if (buffer_iter->second.type != gfx::SHARED_MEMORY_BUFFER) {
+  DCHECK_NE(gfx::EMPTY_BUFFER, buffer_iter->second.type());
+  if (buffer_iter->second.type() != gfx::SHARED_MEMORY_BUFFER) {
     auto* gpu_service = GetGpuService();
     DCHECK(gpu_service);
     gpu_service->DestroyGpuMemoryBuffer(id, client_id, sync_token);
@@ -91,8 +85,8 @@ void HostGpuMemoryBufferManager::DestroyAllGpuMemoryBufferForClient(
   if (client_iter != allocated_buffers_.end()) {
     auto& buffers = client_iter->second;
     for (const auto& pair : buffers) {
-      DCHECK_NE(gfx::EMPTY_BUFFER, pair.second.type);
-      if (pair.second.type != gfx::SHARED_MEMORY_BUFFER) {
+      DCHECK_NE(gfx::EMPTY_BUFFER, pair.second.type());
+      if (pair.second.type() != gfx::SHARED_MEMORY_BUFFER) {
         auto* gpu_service = GetGpuService();
         DCHECK(gpu_service);
         gpu_service->DestroyGpuMemoryBuffer(pair.first, client_id,
@@ -153,12 +147,8 @@ void HostGpuMemoryBufferManager::AllocateGpuMemoryBuffer(
                                                                  format)) {
     buffer_handle = gpu::GpuMemoryBufferImplSharedMemory::CreateGpuMemoryBuffer(
         id, size, format, usage);
-    AllocatedBufferInfo buffer_info;
     DCHECK_EQ(gfx::SHARED_MEMORY_BUFFER, buffer_handle.type);
-    buffer_info.type = gfx::SHARED_MEMORY_BUFFER;
-    buffer_info.buffer_size_in_bytes =
-        gfx::BufferSizeForBufferFormat(size, format);
-    buffer_info.shared_memory_guid = buffer_handle.region.GetGUID();
+    AllocatedBufferInfo buffer_info(buffer_handle, size, format);
     allocated_buffers_[client_id].insert(
         std::make_pair(buffer_handle.id, buffer_info));
   }
@@ -230,33 +220,11 @@ bool HostGpuMemoryBufferManager::OnMemoryDump(
   DCHECK(task_runner_->BelongsToCurrentThread());
   for (const auto& pair : allocated_buffers_) {
     int client_id = pair.first;
+    uint64_t client_tracing_process_id = ClientIdToTracingId(client_id);
     for (const auto& buffer_pair : pair.second) {
-      gfx::GpuMemoryBufferId buffer_id = buffer_pair.first;
-      const AllocatedBufferInfo& buffer_info = buffer_pair.second;
-      base::trace_event::MemoryAllocatorDump* dump =
-          pmd->CreateAllocatorDump(base::StringPrintf(
-              "gpumemorybuffer/client_%d/buffer_%d", client_id, buffer_id.id));
-      if (!dump)
+      auto& buffer_info = buffer_pair.second;
+      if (!buffer_info.OnMemoryDump(pmd, client_id, client_tracing_process_id))
         return false;
-      dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
-                      base::trace_event::MemoryAllocatorDump::kUnitsBytes,
-                      buffer_info.buffer_size_in_bytes);
-
-      // Create the cross-process ownership edge. If the client creates a
-      // corresponding dump for the same buffer, this will avoid to
-      // double-count them in tracing. If, instead, no other process will emit a
-      // dump with the same guid, the segment will be accounted to the browser.
-      uint64_t client_tracing_process_id = ClientIdToTracingId(client_id);
-
-      if (buffer_info.type == gfx::SHARED_MEMORY_BUFFER) {
-        pmd->CreateSharedMemoryOwnershipEdge(
-            dump->guid(), buffer_info.shared_memory_guid, 0 /* importance */);
-      } else {
-        auto shared_buffer_guid = gfx::GetGenericSharedGpuMemoryGUIDForTracing(
-            client_tracing_process_id, buffer_id);
-        pmd->CreateSharedGlobalAllocatorDump(shared_buffer_guid);
-        pmd->AddOwnershipEdge(dump->guid(), shared_buffer_guid);
-      }
     }
   }
   return true;
@@ -348,10 +316,8 @@ void HostGpuMemoryBufferManager::OnGpuMemoryBufferAllocated(
   if (!handle.is_null()) {
     DCHECK(handle.id == id);
 
-    AllocatedBufferInfo buffer_info;
-    buffer_info.type = handle.type;
-    buffer_info.buffer_size_in_bytes = gfx::BufferSizeForBufferFormat(
-        pending_buffer.size, pending_buffer.format);
+    AllocatedBufferInfo buffer_info(handle, pending_buffer.size,
+                                    pending_buffer.format);
     allocated_buffers_[client_id].insert(std::make_pair(id, buffer_info));
   }
   std::move(pending_buffer.callback).Run(std::move(handle));

components/viz/host/host_gpu_memory_buffer_manager.h

@@ -99,14 +99,6 @@ class VIZ_HOST_EXPORT HostGpuMemoryBufferManager
                                             PendingBufferInfo,
                                             std::hash<gfx::GpuMemoryBufferId>>;
 
-  struct AllocatedBufferInfo {
-    AllocatedBufferInfo();
-    ~AllocatedBufferInfo();
-
-    gfx::GpuMemoryBufferType type = gfx::EMPTY_BUFFER;
-    size_t buffer_size_in_bytes = 0;
-    base::UnguessableToken shared_memory_guid;
-  };
   using AllocatedBuffers =
       std::unordered_map<gfx::GpuMemoryBufferId,
                          AllocatedBufferInfo,

components/viz/host/host_gpu_memory_buffer_manager_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/memory/unsafe_shared_memory_region.h"
 #include "base/run_loop.h"
 #include "base/threading/thread.h"
 #include "build/build_config.h"
@@ -74,6 +75,8 @@ class TestGpuService : public mojom::GpuService {
     gfx::GpuMemoryBufferHandle handle;
     handle.id = req.id;
     handle.type = gfx::SHARED_MEMORY_BUFFER;
+    constexpr size_t kBufferSizeBytes = 100;
+    handle.region = base::UnsafeSharedMemoryRegion::Create(kBufferSizeBytes);
 
     DCHECK(req.callback);
     std::move(req.callback).Run(std::move(handle));

components/viz/service/display_embedder/in_process_gpu_memory_buffer_manager.cc

@@ -4,8 +4,12 @@
 
 #include "components/viz/service/display_embedder/in_process_gpu_memory_buffer_manager.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/trace_event/memory_dump_manager.h"
+#include "gpu/command_buffer/client/gpu_memory_buffer_manager.h"
 #include "gpu/command_buffer/service/sync_point_manager.h"
 #include "gpu/ipc/common/gpu_client_ids.h"
 #include "gpu/ipc/common/gpu_memory_buffer_impl.h"
@@ -35,11 +39,16 @@ InProcessGpuMemoryBufferManager::InProcessGpuMemoryBufferManager(
       gpu_memory_buffer_factory_(gpu_memory_buffer_factory),
       sync_point_manager_(sync_point_manager),
       task_runner_(base::ThreadTaskRunnerHandle::Get()) {
+  base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
+      this, "InProcessGpuMemoryBufferManager", task_runner_);
+
   weak_ptr_ = weak_ptr_factory_.GetWeakPtr();
 }
 
 InProcessGpuMemoryBufferManager::~InProcessGpuMemoryBufferManager() {
   DCHECK(task_runner_->BelongsToCurrentThread());
+  base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
+      this);
 }
 
 std::unique_ptr<gfx::GpuMemoryBuffer>
@@ -53,12 +62,19 @@ InProcessGpuMemoryBufferManager::CreateGpuMemoryBuffer(
       gpu_memory_buffer_factory_->CreateGpuMemoryBuffer(
           id, size, format, usage, client_id_, surface_handle);
 
+  AllocatedBufferInfo buffer_info(buffer_handle, size, format);
+
   auto callback = base::BindOnce(
       &InProcessGpuMemoryBufferManager::ShouldDestroyGpuMemoryBuffer, weak_ptr_,
       id);
-  return gpu_memory_buffer_support_.CreateGpuMemoryBufferImplFromHandle(
+  auto gmb = gpu_memory_buffer_support_.CreateGpuMemoryBufferImplFromHandle(
       std::move(buffer_handle), size, format, usage,
       base::BindOnce(&DestroyOnThread, task_runner_, std::move(callback)));
+
+  if (gmb)
+    allocated_buffers_.insert(std::make_pair(id, buffer_info));
+
+  return gmb;
 }
 
 void InProcessGpuMemoryBufferManager::SetDestructionSyncToken(
@@ -68,6 +84,23 @@ void InProcessGpuMemoryBufferManager::SetDestructionSyncToken(
       sync_token);
 }
 
+bool InProcessGpuMemoryBufferManager::OnMemoryDump(
+    const base::trace_event::MemoryDumpArgs& args,
+    base::trace_event::ProcessMemoryDump* pmd) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  uint64_t client_tracing_process_id =
+      base::trace_event::MemoryDumpManager::GetInstance()
+          ->GetTracingProcessId();
+
+  for (auto& buffer_pair : allocated_buffers_) {
+    auto& buffer_info = buffer_pair.second;
+    if (!buffer_info.OnMemoryDump(pmd, client_id_, client_tracing_process_id))
+      return false;
+  }
+
+  return true;
+}
+
 void InProcessGpuMemoryBufferManager::ShouldDestroyGpuMemoryBuffer(
     gfx::GpuMemoryBufferId id,
     const gpu::SyncToken& sync_token) {
@@ -89,6 +122,7 @@ void InProcessGpuMemoryBufferManager::ShouldDestroyGpuMemoryBuffer(
 
 void InProcessGpuMemoryBufferManager::DestroyGpuMemoryBuffer(
     gfx::GpuMemoryBufferId id) {
+  allocated_buffers_.erase(id);
   gpu_memory_buffer_factory_->DestroyGpuMemoryBuffer(id, client_id_);
 }
 

components/viz/service/display_embedder/in_process_gpu_memory_buffer_manager.h

@@ -5,7 +5,11 @@
 #ifndef COMPONENTS_VIZ_SERVICE_DISPLAY_EMBEDDER_IN_PROCESS_GPU_MEMORY_BUFFER_MANAGER_H_
 #define COMPONENTS_VIZ_SERVICE_DISPLAY_EMBEDDER_IN_PROCESS_GPU_MEMORY_BUFFER_MANAGER_H_
 
+#include <memory>
+
+#include "base/containers/flat_map.h"
 #include "base/memory/weak_ptr.h"
+#include "base/trace_event/memory_dump_provider.h"
 #include "components/viz/service/viz_service_export.h"
 #include "gpu/command_buffer/client/gpu_memory_buffer_manager.h"
 #include "gpu/ipc/common/gpu_memory_buffer_support.h"
@@ -20,7 +24,8 @@ namespace viz {
 // GpuMemoryBufferManager implementation usable from any thread in the GPU
 // process. Must be created and destroyed on the same thread.
 class VIZ_SERVICE_EXPORT InProcessGpuMemoryBufferManager
-    : public gpu::GpuMemoryBufferManager {
+    : public gpu::GpuMemoryBufferManager,
+      public base::trace_event::MemoryDumpProvider {
  public:
   // |gpu_memory_buffer_factory| and |sync_point_manager| must outlive |this|.
   InProcessGpuMemoryBufferManager(
@@ -39,6 +44,10 @@ class VIZ_SERVICE_EXPORT InProcessGpuMemoryBufferManager
   void SetDestructionSyncToken(gfx::GpuMemoryBuffer* buffer,
                                const gpu::SyncToken& sync_token) override;
 
+  // base::trace_event::MemoryDumpProvider:
+  bool OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
+                    base::trace_event::ProcessMemoryDump* pmd) override;
+
  private:
   // Provided as callback when a GpuMemoryBuffer should be destroyed.
   void ShouldDestroyGpuMemoryBuffer(gfx::GpuMemoryBufferId id,
@@ -52,8 +61,11 @@ class VIZ_SERVICE_EXPORT InProcessGpuMemoryBufferManager
   gpu::GpuMemoryBufferFactory* const gpu_memory_buffer_factory_;
   gpu::SyncPointManager* const sync_point_manager_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
-  base::WeakPtr<InProcessGpuMemoryBufferManager> weak_ptr_;
 
+  base::flat_map<gfx::GpuMemoryBufferId, AllocatedBufferInfo>
+      allocated_buffers_;
+
+  base::WeakPtr<InProcessGpuMemoryBufferManager> weak_ptr_;
   base::WeakPtrFactory<InProcessGpuMemoryBufferManager> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(InProcessGpuMemoryBufferManager);

gpu/command_buffer/client/gpu_memory_buffer_manager.cc

@@ -4,10 +4,59 @@
 
 #include "gpu/command_buffer/client/gpu_memory_buffer_manager.h"
 
+#include <inttypes.h>
+
+#include "base/strings/stringprintf.h"
+#include "base/trace_event/process_memory_dump.h"
+#include "ui/gfx/buffer_format_util.h"
+
 namespace gpu {
 
 GpuMemoryBufferManager::GpuMemoryBufferManager() = default;
 
 GpuMemoryBufferManager::~GpuMemoryBufferManager() = default;
 
+GpuMemoryBufferManager::AllocatedBufferInfo::AllocatedBufferInfo(
+    const gfx::GpuMemoryBufferHandle& handle,
+    const gfx::Size& size,
+    gfx::BufferFormat format)
+    : buffer_id_(handle.id),
+      type_(handle.type),
+      size_in_bytes_(gfx::BufferSizeForBufferFormat(size, format)) {
+  DCHECK_NE(gfx::EMPTY_BUFFER, type_);
+
+  if (type_ == gfx::SHARED_MEMORY_BUFFER)
+    shared_memory_guid_ = handle.region.GetGUID();
+}
+
+GpuMemoryBufferManager::AllocatedBufferInfo::~AllocatedBufferInfo() = default;
+
+bool GpuMemoryBufferManager::AllocatedBufferInfo::OnMemoryDump(
+    base::trace_event::ProcessMemoryDump* pmd,
+    int client_id,
+    uint64_t client_tracing_process_id) const {
+  base::trace_event::MemoryAllocatorDump* dump = pmd->CreateAllocatorDump(
+      base::StringPrintf("gpumemorybuffer/client_0x%" PRIX32 "/buffer_%d",
+                         client_id, buffer_id_.id));
+  if (!dump)
+    return false;
+
+  dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
+                  base::trace_event::MemoryAllocatorDump::kUnitsBytes,
+                  size_in_bytes_);
+
+  // Create the shared ownership edge to avoid double counting memory.
+  if (type_ == gfx::SHARED_MEMORY_BUFFER) {
+    pmd->CreateSharedMemoryOwnershipEdge(dump->guid(), shared_memory_guid_,
+                                         /*importance=*/0);
+  } else {
+    auto shared_buffer_guid = gfx::GetGenericSharedGpuMemoryGUIDForTracing(
+        client_tracing_process_id, buffer_id_);
+    pmd->CreateSharedGlobalAllocatorDump(shared_buffer_guid);
+    pmd->AddOwnershipEdge(dump->guid(), shared_buffer_guid);
+  }
+
+  return true;
+}
+
 }  // namespace gpu

gpu/command_buffer/client/gpu_memory_buffer_manager.h

@@ -33,6 +33,29 @@ class GPU_EXPORT GpuMemoryBufferManager {
   // thread.
   virtual void SetDestructionSyncToken(gfx::GpuMemoryBuffer* buffer,
                                        const gpu::SyncToken& sync_token) = 0;
+
+ protected:
+  class GPU_EXPORT AllocatedBufferInfo {
+   public:
+    AllocatedBufferInfo(const gfx::GpuMemoryBufferHandle& handle,
+                        const gfx::Size& size,
+                        gfx::BufferFormat format);
+    ~AllocatedBufferInfo();
+
+    gfx::GpuMemoryBufferType type() const { return type_; }
+
+    // Add a memory dump for this buffer to |pmd|. Returns false if adding the
+    // dump failed.
+    bool OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd,
+                      int client_id,
+                      uint64_t client_tracing_process_id) const;
+
+   private:
+    gfx::GpuMemoryBufferId buffer_id_;
+    gfx::GpuMemoryBufferType type_;
+    size_t size_in_bytes_;
+    base::UnguessableToken shared_memory_guid_;
+  };
 };
 
 }  // namespace gpu