Memory-infra: Remove use of unique tracing ID and add resident size t…

…o discardable dumps This CL does: 1. Remove the use of tracing process ID and unique client ID in discardable service since we switched to shared memory ownership edges, crbug.com/604726. 2. Use the resident size calculated by the shared memory dumps in the discardable segment instead of calculating again. This means the discardable providers will account for the wasted memory due to fragmentation. 3. Moves all the edge logic to DiscardableSharedMemory since it knows about |SharedMemory| and code is shared by manager and client. Note to perf sheriffs: This might cause perf regression or improvements in discardable memory total since it accounts for the resident size instead of allocated size. Bug: 661257 Change-Id: Ia88b90399237d81ff4e2410ae4712157e852ca9e Reviewed-on: https://chromium-review.googlesource.com/609441 Commit-Queue: Siddhartha S <ssid@chromium.org> Reviewed-by: Erik Chen <erikchen@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Hajime Hoshi <hajimehoshi@chromium.org> Reviewed-by: David Reveman <reveman@chromium.org> Cr-Commit-Position: refs/heads/master@{#495732}
hylschwy · Aug 18, 2017 · e241d0a · e241d0a
1 parent ea45782
commit e241d0a
Show file tree

Hide file tree

Showing 10 changed files with 150 additions and 78 deletions.
diff --git a/base/memory/discardable_shared_memory.cc b/base/memory/discardable_shared_memory.cc
@@ -11,8 +11,11 @@
 #include "base/atomicops.h"
 #include "base/bits.h"
 #include "base/logging.h"
+#include "base/memory/shared_memory_tracker.h"
 #include "base/numerics/safe_math.h"
 #include "base/process/process_metrics.h"
+#include "base/trace_event/memory_allocator_dump.h"
+#include "base/trace_event/process_memory_dump.h"
 #include "build/build_config.h"
 
 #if defined(OS_POSIX) && !defined(OS_NACL)
@@ -435,6 +438,41 @@ void DiscardableSharedMemory::Close() {
   shared_memory_.Close();
 }
 
+void DiscardableSharedMemory::CreateSharedMemoryOwnershipEdge(
+    trace_event::MemoryAllocatorDump* local_segment_dump,
+    trace_event::ProcessMemoryDump* pmd,
+    bool is_owned) const {
+  auto* shared_memory_dump =
+      SharedMemoryTracker::GetOrCreateSharedMemoryDump(&shared_memory_, pmd);
+  // TODO(ssid): Clean this by a new api to inherit size of parent dump once the
+  // we send the full PMD and calculate sizes inside chrome, crbug.com/704203.
+  size_t resident_size = shared_memory_dump->GetSizeInternal();
+  local_segment_dump->AddScalar(trace_event::MemoryAllocatorDump::kNameSize,
+                                trace_event::MemoryAllocatorDump::kUnitsBytes,
+                                resident_size);
+
+  // By creating an edge with a higher |importance| (w.r.t non-owned dumps)
+  // the tracing UI will account the effective size of the segment to the
+  // client instead of manager.
+  // TODO(ssid): Define better constants in MemoryAllocatorDump for importance
+  // values, crbug.com/754793.
+  const int kImportance = is_owned ? 2 : 0;
+  auto shared_memory_guid = shared_memory_.mapped_id();
+  local_segment_dump->AddString("id", "hash", shared_memory_guid.ToString());
+
+  // Owned discardable segments which are allocated by client process, could
+  // have been cleared by the discardable manager. So, the segment need not
+  // exist in memory and weak dumps are created to indicate the UI that the dump
+  // should exist only if the manager also created the global dump edge.
+  if (is_owned) {
+    pmd->CreateWeakSharedMemoryOwnershipEdge(local_segment_dump->guid(),
+                                             shared_memory_guid, kImportance);
+  } else {
+    pmd->CreateSharedMemoryOwnershipEdge(local_segment_dump->guid(),
+                                         shared_memory_guid, kImportance);
+  }
+}
+
 Time DiscardableSharedMemory::Now() const {
   return Time::Now();
 }

diff --git a/base/memory/discardable_shared_memory.h b/base/memory/discardable_shared_memory.h
@@ -32,6 +32,11 @@
 
 namespace base {
 
+namespace trace_event {
+class MemoryAllocatorDump;
+class ProcessMemoryDump;
+}  // namespace trace_event
+
 // Platform abstraction for discardable shared memory.
 //
 // This class is not thread-safe. Clients are responsible for synchronizing
@@ -131,6 +136,16 @@ class BASE_EXPORT DiscardableSharedMemory {
   // It is safe to call Close repeatedly.
   void Close();
 
+  // For tracing: Creates ownership edge to the underlying shared memory dump
+  // which is cross process in the given |pmd|. |local_segment_dump| is the dump
+  // associated with the local discardable shared memory segment and |is_owned|
+  // is true when the current process owns the segment and the effective memory
+  // is assigned to the current process.
+  void CreateSharedMemoryOwnershipEdge(
+      trace_event::MemoryAllocatorDump* local_segment_dump,
+      trace_event::ProcessMemoryDump* pmd,
+      bool is_owned) const;
+
  private:
   // Virtual for tests.
   virtual Time Now() const;

diff --git a/base/memory/discardable_shared_memory_unittest.cc b/base/memory/discardable_shared_memory_unittest.cc
@@ -5,7 +5,10 @@
 #include <stdint.h>
 
 #include "base/memory/discardable_shared_memory.h"
+#include "base/memory/shared_memory_tracker.h"
 #include "base/process/process_metrics.h"
+#include "base/trace_event/memory_allocator_dump.h"
+#include "base/trace_event/process_memory_dump.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
@@ -390,5 +393,30 @@ TEST(DiscardableSharedMemoryTest, ZeroFilledPagesAfterPurge) {
 }
 #endif
 
+TEST(DiscardableSharedMemoryTest, TracingOwnershipEdges) {
+  const uint32_t kDataSize = 1024;
+  TestDiscardableSharedMemory memory1;
+  bool rv = memory1.CreateAndMap(kDataSize);
+  ASSERT_TRUE(rv);
+
+  base::trace_event::MemoryDumpArgs args = {
+      base::trace_event::MemoryDumpLevelOfDetail::DETAILED};
+  trace_event::ProcessMemoryDump pmd(nullptr, args);
+  trace_event::MemoryAllocatorDump* client_dump =
+      pmd.CreateAllocatorDump("discardable_manager/map1");
+  const bool is_owned = false;
+  memory1.CreateSharedMemoryOwnershipEdge(client_dump, &pmd, is_owned);
+  const auto* shm_dump = pmd.GetAllocatorDump(
+      SharedMemoryTracker::GetDumpNameForTracing(memory1.mapped_id()));
+  EXPECT_TRUE(shm_dump);
+  EXPECT_EQ(shm_dump->GetSizeInternal(), client_dump->GetSizeInternal());
+  const auto edges = pmd.allocator_dumps_edges_for_testing();
+  EXPECT_EQ(2u, edges.size());
+  EXPECT_NE(edges.end(), edges.find(shm_dump->guid()));
+  EXPECT_NE(edges.end(), edges.find(client_dump->guid()));
+  // TODO(ssid): test for weak global dump once the
+  // CreateWeakSharedMemoryOwnershipEdge() is fixed, crbug.com/661257.
+}
+
 }  // namespace
 }  // namespace base
diff --git a/base/memory/shared_memory_tracker.cc b/base/memory/shared_memory_tracker.cc
@@ -12,27 +12,17 @@
 
 namespace base {
 
-namespace {
-
-std::string GetDumpNameForTracing(const UnguessableToken& id) {
-  DCHECK(!id.is_empty());
-  return "shared_memory/" + id.ToString();
-}
-
-}  // namespace
-
 // static
 SharedMemoryTracker* SharedMemoryTracker::GetInstance() {
   static SharedMemoryTracker* instance = new SharedMemoryTracker;
   return instance;
 }
 
 // static
-trace_event::MemoryAllocatorDumpGuid SharedMemoryTracker::GetDumpIdForTracing(
+std::string SharedMemoryTracker::GetDumpNameForTracing(
     const UnguessableToken& id) {
-  std::string dump_name = GetDumpNameForTracing(id);
-  return trace_event::MemoryAllocatorDump::GetDumpIdFromName(
-      std::move(dump_name));
+  DCHECK(!id.is_empty());
+  return "shared_memory/" + id.ToString();
 }
 
 // static
@@ -42,6 +32,47 @@ SharedMemoryTracker::GetGlobalDumpIdForTracing(const UnguessableToken& id) {
   return trace_event::MemoryAllocatorDumpGuid(dump_name);
 }
 
+// static
+const trace_event::MemoryAllocatorDump*
+SharedMemoryTracker::GetOrCreateSharedMemoryDump(
+    const SharedMemory* shared_memory,
+    trace_event::ProcessMemoryDump* pmd) {
+  const std::string dump_name =
+      GetDumpNameForTracing(shared_memory->mapped_id());
+  trace_event::MemoryAllocatorDump* local_dump =
+      pmd->GetAllocatorDump(dump_name);
+  if (local_dump)
+    return local_dump;
+
+  size_t virtual_size = shared_memory->mapped_size();
+  // If resident size is not available, a virtual size is used as fallback.
+  size_t size = virtual_size;
+#if defined(COUNT_RESIDENT_BYTES_SUPPORTED)
+  base::Optional<size_t> resident_size =
+      trace_event::ProcessMemoryDump::CountResidentBytesInSharedMemory(
+          *shared_memory);
+  if (resident_size.has_value())
+    size = resident_size.value();
+#endif
+
+  local_dump = pmd->CreateAllocatorDump(dump_name);
+  local_dump->AddScalar(trace_event::MemoryAllocatorDump::kNameSize,
+                        trace_event::MemoryAllocatorDump::kUnitsBytes, size);
+  local_dump->AddScalar("virtual_size",
+                        trace_event::MemoryAllocatorDump::kUnitsBytes,
+                        virtual_size);
+  auto global_dump_guid = GetGlobalDumpIdForTracing(shared_memory->mapped_id());
+  trace_event::MemoryAllocatorDump* global_dump =
+      pmd->CreateSharedGlobalAllocatorDump(global_dump_guid);
+  global_dump->AddScalar(trace_event::MemoryAllocatorDump::kNameSize,
+                         trace_event::MemoryAllocatorDump::kUnitsBytes, size);
+
+  // The edges will be overriden by the clients with correct importance.
+  pmd->AddOverridableOwnershipEdge(local_dump->guid(), global_dump->guid(),
+                                   0 /* importance */);
+  return local_dump;
+}
+
 void SharedMemoryTracker::IncrementMemoryUsage(
     const SharedMemory& shared_memory) {
   AutoLock hold(usages_lock_);
@@ -58,52 +89,14 @@ void SharedMemoryTracker::DecrementMemoryUsage(
 
 bool SharedMemoryTracker::OnMemoryDump(const trace_event::MemoryDumpArgs& args,
                                        trace_event::ProcessMemoryDump* pmd) {
-  // The fields are shared memory's ID, its resident size and its virtual size
-  // respectively. If a resident size is not available, a virtual size is used
-  // as fallback.
-  std::vector<std::tuple<UnguessableToken, size_t, size_t>> usages;
   {
     AutoLock hold(usages_lock_);
-    usages.reserve(usages_.size());
     for (const auto& usage : usages_) {
-      const SharedMemory* shared_memory = usage.first;
-      size_t virtual_size = usage.second;
-      size_t size = virtual_size;
-#if defined(COUNT_RESIDENT_BYTES_SUPPORTED)
-      base::Optional<size_t> resident_size =
-          trace_event::ProcessMemoryDump::CountResidentBytesInSharedMemory(
-              *shared_memory);
-      if (resident_size.has_value())
-        size = resident_size.value();
-#endif
-      usages.emplace_back(shared_memory->mapped_id(), size, virtual_size);
+      const trace_event::MemoryAllocatorDump* dump =
+          GetOrCreateSharedMemoryDump(usage.first, pmd);
+      DCHECK(dump);
     }
   }
-  for (const auto& usage : usages) {
-    const UnguessableToken& memory_guid = std::get<0>(usage);
-    size_t size = std::get<1>(usage);
-    size_t virtual_size = std::get<2>(usage);
-    std::string dump_name = GetDumpNameForTracing(memory_guid);
-    // Discard duplicates that might be seen in single-process mode.
-    if (pmd->GetAllocatorDump(dump_name))
-      continue;
-    trace_event::MemoryAllocatorDump* local_dump =
-        pmd->CreateAllocatorDump(dump_name);
-    local_dump->AddScalar(trace_event::MemoryAllocatorDump::kNameSize,
-                          trace_event::MemoryAllocatorDump::kUnitsBytes, size);
-    local_dump->AddScalar("virtual_size",
-                          trace_event::MemoryAllocatorDump::kUnitsBytes,
-                          virtual_size);
-    auto global_dump_guid = GetGlobalDumpIdForTracing(memory_guid);
-    trace_event::MemoryAllocatorDump* global_dump =
-        pmd->CreateSharedGlobalAllocatorDump(global_dump_guid);
-    global_dump->AddScalar(trace_event::MemoryAllocatorDump::kNameSize,
-                           trace_event::MemoryAllocatorDump::kUnitsBytes, size);
-
-    // The edges will be overriden by the clients with correct importance.
-    pmd->AddOverridableOwnershipEdge(local_dump->guid(), global_dump->guid(),
-                                     0 /* importance */);
-  }
   return true;
 }
 

diff --git a/base/memory/shared_memory_tracker.h b/base/memory/shared_memory_tracker.h
@@ -6,6 +6,7 @@
 #define BASE_MEMORY_SHARED_MEMORY_TRACKER_H_
 
 #include <map>
+#include <string>
 
 #include "base/memory/shared_memory.h"
 #include "base/synchronization/lock.h"
@@ -14,6 +15,7 @@
 namespace base {
 
 namespace trace_event {
+class MemoryAllocatorDump;
 class MemoryAllocatorDumpGuid;
 class ProcessMemoryDump;
 }
@@ -24,12 +26,18 @@ class BASE_EXPORT SharedMemoryTracker : public trace_event::MemoryDumpProvider {
   // Returns a singleton instance.
   static SharedMemoryTracker* GetInstance();
 
-  static trace_event::MemoryAllocatorDumpGuid GetDumpIdForTracing(
-      const UnguessableToken& id);
+  static std::string GetDumpNameForTracing(const UnguessableToken& id);
 
   static trace_event::MemoryAllocatorDumpGuid GetGlobalDumpIdForTracing(
       const UnguessableToken& id);
 
+  // Gets or creates if non-existant, a memory dump for the |shared_memory|
+  // inside the given |pmd|. Also adds the necessary edges for the dump when
+  // creating the dump.
+  static const trace_event::MemoryAllocatorDump* GetOrCreateSharedMemoryDump(
+      const SharedMemory* shared_memory,
+      trace_event::ProcessMemoryDump* pmd);
+
   // Records shared memory usage on mapping.
   void IncrementMemoryUsage(const SharedMemory& shared_memory);
 

diff --git a/base/trace_event/memory_allocator_dump.h b/base/trace_event/memory_allocator_dump.h
@@ -33,6 +33,8 @@ class BASE_EXPORT MemoryAllocatorDump {
     WEAK = 1 << 0,
   };
 
+  // Returns the Guid of the dump for the given |absolute_name| for the
+  // current process.
   static MemoryAllocatorDumpGuid GetDumpIdFromName(
       const std::string& absolute_name);
 

diff --git a/base/trace_event/process_memory_dump.cc b/base/trace_event/process_memory_dump.cc
@@ -417,8 +417,8 @@ void ProcessMemoryDump::CreateSharedMemoryOwnershipEdgeInternal(
 
   // The guid of the local dump created by SharedMemoryTracker for the memory
   // segment.
-  auto local_shm_guid =
-      SharedMemoryTracker::GetDumpIdForTracing(shared_memory_guid);
+  auto local_shm_guid = MemoryAllocatorDump::GetDumpIdFromName(
+      SharedMemoryTracker::GetDumpNameForTracing(shared_memory_guid));
 
   // The dump guid of the global dump created by the tracker for the memory
   // segment.

diff --git a/base/trace_event/process_memory_dump_unittest.cc b/base/trace_event/process_memory_dump_unittest.cc
@@ -335,7 +335,8 @@ TEST(ProcessMemoryDumpTest, SharedMemoryOwnershipTest) {
   auto* client_dump2 = pmd->CreateAllocatorDump("discardable/segment2");
   auto shm_token2 = UnguessableToken::Create();
   MemoryAllocatorDumpGuid shm_local_guid2 =
-      SharedMemoryTracker::GetDumpIdForTracing(shm_token2);
+      MemoryAllocatorDump::GetDumpIdFromName(
+          SharedMemoryTracker::GetDumpNameForTracing(shm_token2));
   MemoryAllocatorDumpGuid shm_global_guid2 =
       SharedMemoryTracker::GetGlobalDumpIdForTracing(shm_token2);
   pmd->AddOverridableOwnershipEdge(shm_local_guid2, shm_global_guid2,

diff --git a/components/discardable_memory/common/discardable_shared_memory_heap.cc b/components/discardable_memory/common/discardable_shared_memory_heap.cc
@@ -395,11 +395,6 @@ void DiscardableSharedMemoryHeap::OnMemoryDump(
       base::StringPrintf("discardable/segment_%d", segment_id);
   base::trace_event::MemoryAllocatorDump* segment_dump =
       pmd->CreateAllocatorDump(segment_dump_name);
-  // The size is added here so that telemetry picks up the size. Usually it is
-  // just enough to add it to the global dump.
-  segment_dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
-                          base::trace_event::MemoryAllocatorDump::kUnitsBytes,
-                          allocated_objects_size_in_bytes);
   segment_dump->AddScalar("virtual_size",
                           base::trace_event::MemoryAllocatorDump::kUnitsBytes,
                           size);
@@ -416,14 +411,9 @@ void DiscardableSharedMemoryHeap::OnMemoryDump(
                       base::trace_event::MemoryAllocatorDump::kUnitsBytes,
                       locked_objects_size_in_bytes);
 
-  // By creating an edge with a higher |importance| (w.r.t. browser-side dumps)
-  // the tracing UI will account the effective size of the segment to the
-  // client.
-  const int kImportance = 2;
-  auto shared_memory_guid = shared_memory->mapped_id();
-  segment_dump->AddString("id", "hash", shared_memory_guid.ToString());
-  pmd->CreateWeakSharedMemoryOwnershipEdge(segment_dump->guid(),
-                                           shared_memory_guid, kImportance);
+  // The memory is owned by the client process (current).
+  shared_memory->CreateSharedMemoryOwnershipEdge(segment_dump, pmd,
+                                                 /*is_owned=*/true);
 }
 
 base::trace_event::MemoryAllocatorDump*

diff --git a/components/discardable_memory/service/discardable_shared_memory_manager.cc b/components/discardable_memory/service/discardable_shared_memory_manager.cc
@@ -16,6 +16,7 @@
 #include "base/memory/discardable_memory.h"
 #include "base/memory/memory_coordinator_client_registry.h"
 #include "base/memory/ptr_util.h"
+#include "base/memory/shared_memory_tracker.h"
 #include "base/numerics/safe_math.h"
 #include "base/process/memory.h"
 #include "base/strings/string_number_conversions.h"
@@ -286,8 +287,6 @@ bool DiscardableSharedMemoryManager::OnMemoryDump(
       if (!segment->memory()->mapped_size())
         continue;
 
-      // TODO(ssid): The "size" should be inherited from the shared memory dump,
-      // crbug.com/661257.
       std::string dump_name = base::StringPrintf(
           "discardable/process_%x/segment_%d", client_id, segment_id);
       base::trace_event::MemoryAllocatorDump* dump =
@@ -303,10 +302,8 @@ bool DiscardableSharedMemoryManager::OnMemoryDump(
           segment->memory()->IsMemoryLocked() ? segment->memory()->mapped_size()
                                               : 0u);
 
-      auto shared_memory_guid = segment->memory()->mapped_id();
-      dump->AddString("id", "hash", shared_memory_guid.ToString());
-      pmd->CreateSharedMemoryOwnershipEdge(dump->guid(), shared_memory_guid,
-                                           0 /* importance */);
+      segment->memory()->CreateSharedMemoryOwnershipEdge(dump, pmd,
+                                                         /*is_owned=*/false);
     }
   }
   return true;