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/strings/string_number_conversions.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, uintptr_t, size_t>> usages;

[danakj, 2017/05/24 16:39:45]

no base:: inside base, throughout this CL

[hajimehoshi, 2017/05/25 00:33:30]

Done.

   {
     AutoLock hold(usages_lock_);
+    usages.reserve(usages_.size());
     for (const auto& usage : usages_)

[Primiano Tucci (use gerrit), 2017/05/24 11:23:37]

small nit: add a curly brace around the for if the statement is > 1 line

[hajimehoshi, 2017/05/25 00:33:30]

Done.

-      sizes[usage.second.unique_id] += usage.second.size;

[hajimehoshi, 2017/05/22 08:32:01]

I found that this implementation (before this CL) was wrong. In the single
process mode, sizes of a same shared memory are accumulated, which doesn't make
sense. And this hid the failures of SingleProcessMemoryTracingTest failures.

I think I can fix this independently, but this CL will change the logic so the
fix will become irrelevant. I want to commit this CL as it is, but what do you
think primiano@ and erikchen@?

[erikchen, 2017/05/22 16:19:31]

We need to fix the interface of ProcessMemoryDump to make sense for shared
memory in single-process cases. But otherwise this CL is fine.

+      usages.emplace_back(usage.first->handle().GetGUID(),
+                          reinterpret_cast<uintptr_t>(usage.first->memory()),
+                          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 (const auto& usage : usages) {
+    const base::UnguessableToken& guid = std::get<0>(usage);

[danakj, 2017/05/24 16:39:45]

nit: memory_guid? since there's 2 guids in this function

[hajimehoshi, 2017/05/25 00:33:30]

Done.

+    uintptr_t address = std::get<1>(usage);
+    size_t size = std::get<2>(usage);
+    std::string dump_name = "shared_memory/";
+    if (guid.is_empty()) {
+      // TODO(hajimehoshi): As passing ID across mojo is not implemented yet
+      // (crbug/713763), ID can be empty. For such case, use an address instead
+      // of GUID so that approximate memory usages are available.
+      dump_name += "(address)" + base::Uint64ToString(address);

[Primiano Tucci (use gerrit), 2017/05/24 11:23:37]

just uint64tostring should be fine, without the extra "address" concat

[hajimehoshi, 2017/05/25 00:33:29]

Hmm, so both will be just hex-integer strings so I preferred adding concat to
distinguish them. Dump names with 'addresses' will be removed after Mojo is
fixed anyway. Done.

+    } else {
+      dump_name += guid.ToString();
+    }
+    auto dump_guid = trace_event::MemoryAllocatorDumpGuid(dump_name);
+    // The dump might already be created on single-process mode.

[Primiano Tucci (use gerrit), 2017/05/24 11:23:37]

took me a while to realize that this was referring to the if below. can you:
1. move it just before the if.
2. Say "Discard duplicates that might be seen in single-process mode."

when I read " The dump might already be created on single-process mode." i am
not fully sure what is the semantic we intend to have.

[danakj, 2017/05/24 16:39:45]

I would not use the local variable for GetAllocatorDump and then you don't have
to choose between having this on the Get or the if. Just

if (GetAllocatorDump(...)) continue;

Since we don't use the local var more than that anyway, it gets replaced
immediately if we don't continue.

[hajimehoshi, 2017/05/25 00:33:30]

Done.

     trace_event::MemoryAllocatorDump* local_dump =
-        pmd->CreateAllocatorDump(dump_name);
+        pmd->GetAllocatorDump(dump_name);
+    if (local_dump)
+      continue;
+    local_dump = pmd->CreateAllocatorDump(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