Replace SharedMemory::UniqueID usages with SharedMemoryHandle's guid

base/memory/shared_memory_tracker.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/memory/shared_memory_tracker.h"
 
 #include "base/memory/shared_memory.h"
-#include "base/strings/stringprintf.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/process_memory_dump.h"
 
 namespace base {
 
-SharedMemoryTracker::Usage::Usage() = default;
-
-SharedMemoryTracker::Usage::Usage(const Usage& rhs) = default;
-
-SharedMemoryTracker::Usage::~Usage() = default;
-
 // static
 SharedMemoryTracker* SharedMemoryTracker::GetInstance() {
   static SharedMemoryTracker* instance = new SharedMemoryTracker;
   return instance;
 }
 
 void SharedMemoryTracker::IncrementMemoryUsage(
     const SharedMemory& shared_memory) {
-  Usage usage;
-  // |shared_memory|'s unique ID must be generated here and it'd be too late at
-  // OnMemoryDump. An ID is generated with a SharedMemoryHandle, but the handle
-  // might already be closed at that time. Now IncrementMemoryUsage is called
-  // just after mmap and the handle must live then. See the discussion at
-  // crbug.com/604726#c30.
-  SharedMemory::UniqueId id;
-  if (!shared_memory.GetUniqueId(&id))
-    return;
-  usage.unique_id = id;
-  usage.size = shared_memory.mapped_size();
   AutoLock hold(usages_lock_);
-  usages_[&shared_memory] = usage;
+  usages_[&shared_memory] = shared_memory.mapped_size();
 }
 
 void SharedMemoryTracker::DecrementMemoryUsage(
     const SharedMemory& shared_memory) {
   AutoLock hold(usages_lock_);
   usages_.erase(&shared_memory);
 }
 
 bool SharedMemoryTracker::OnMemoryDump(const trace_event::MemoryDumpArgs& args,
                                        trace_event::ProcessMemoryDump* pmd) {
-  std::unordered_map<SharedMemory::UniqueId, size_t, SharedMemory::UniqueIdHash>
-      sizes;
+  std::vector<std::tuple<base::UnguessableToken, size_t>> usages;
   {
     AutoLock hold(usages_lock_);
-    for (const auto& usage : usages_)
-      sizes[usage.second.unique_id] += usage.second.size;
+    usages.reserve(usages_.size());
+    for (auto& usage : usages_)

[erikchen, 2017/05/11 21:54:40]

The C++ style guide says to only use auto "to avoid type names that are noisy,
obvious, or unimportant". IN this CL, I think every use of auto would have been
more clear as an explicit type.

https://google.github.io/styleguide/cppguide.html#auto

Also, please use a const reference.

[hajimehoshi, 2017/05/12 09:27:31]

Done. I think auto for map iterators is encouraged, right?

+      usages.emplace_back(usage.first->handle().GetGUID(), usage.second);
   }
-  for (auto& size : sizes) {
-    const SharedMemory::UniqueId& id = size.first;
-    std::string dump_name = StringPrintf("%s/%lld.%lld", "shared_memory",
-                                         static_cast<long long>(id.first),
-                                         static_cast<long long>(id.second));
-    auto guid = trace_event::MemoryAllocatorDumpGuid(dump_name);
+  for (auto& usage : usages) {
+    auto guid = std::get<0>(usage);

[Primiano Tucci (use gerrit), 2017/05/11 15:10:43]

const auto&?

[hajimehoshi, 2017/05/12 09:27:31]

Done.

+    auto size = std::get<1>(usage);
+    // TODO(hajimehoshi): As passing ID across mojo is not implemented yet
+    // (crbug/713763), ID can be empty.
+    if (guid.is_empty())

[Primiano Tucci (use gerrit), 2017/05/11 15:10:43]

see my previous comment, copy-pasting
----
However I wouldn't have the continue below. if you do this, shared_memory is
gong to report nothing until mojo is fixed. Instead I would just have a fallback
for the dump_name (use the address  of the mmaped shared memory if mapped, or
something else, e.g. a monotonic counter, if not mapped)
this will allow the approximation mentioned above
----

[hajimehoshi, 2017/05/12 09:27:31]

Ah sorry I missed that comment. As all shared memories are mapped here, we can
use their addresses. Done.

+      continue;
+    std::string dump_name = "shared_memory/" + guid.ToString();
+    auto dump_guid = trace_event::MemoryAllocatorDumpGuid(dump_name);
+    // The dump might already be created on single-process mode.
     trace_event::MemoryAllocatorDump* local_dump =
-        pmd->CreateAllocatorDump(dump_name);
+        pmd->GetAllocatorDump(dump_name);
+    if (local_dump)

[erikchen, 2017/05/11 21:54:40]

This logic looks new? In general, we shouldn't need to special case for
single-process mode [which is mostly unsupported these days anyways]. 

I'm not sure what's going on, maybe primiano would know better?

[hajimehoshi, 2017/05/12 09:27:31]

Ah, in theory this conflict can happen even before this CL. I found this with
`content_browsertests --gtest_filter=SingleProcessMemoryTracingTest.QueuedDumps`
but I couldn't find this before this CL. primiano@, what do you think?

[hajimehoshi, 2017/05/17 10:13:35]

ping primiano@

+      continue;
+    local_dump = pmd->GetOrCreateAllocatorDump(dump_name);
     // TODO(hajimehoshi): The size is not resident size but virtual size so far.
     // Fix this to record resident size.
     local_dump->AddScalar(trace_event::MemoryAllocatorDump::kNameSize,
-                          trace_event::MemoryAllocatorDump::kUnitsBytes,
-                          size.second);
+                          trace_event::MemoryAllocatorDump::kUnitsBytes, size);
     trace_event::MemoryAllocatorDump* global_dump =
-        pmd->CreateSharedGlobalAllocatorDump(guid);
+        pmd->CreateSharedGlobalAllocatorDump(dump_guid);
     global_dump->AddScalar(trace_event::MemoryAllocatorDump::kNameSize,
-                           trace_event::MemoryAllocatorDump::kUnitsBytes,
-                           size.second);
+                           trace_event::MemoryAllocatorDump::kUnitsBytes, size);
     // TOOD(hajimehoshi): Detect which the shared memory comes from browser,
     // renderer or GPU process.
     // TODO(hajimehoshi): Shared memory reported by GPU and discardable is
     // currently double-counted. Add ownership edges to avoid this.
     pmd->AddOwnershipEdge(local_dump->guid(), global_dump->guid());
   }
   return true;
 }
 
 SharedMemoryTracker::SharedMemoryTracker() {
   trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
       this, "SharedMemoryTracker", nullptr);
 }
 
 SharedMemoryTracker::~SharedMemoryTracker() = default;
 
 }  // namespace