Restructure V8 isolate memory dumps

The motivation for this change is to make the paths to heap space
nodes static so that they can be plumbed into UMA/UKM reporting.

This groups the nodes by the isolate type and turns the dynamic
"isolate_0x*" into leaf nodes in the tree.

V8 memory dump before:
- V8
  - isolate_0x1234
    - heap_spaces
      - code_space
      ...
  - isolate_0x5678
    ...
  - isolate_0x9012

V8 memory dump after:
- V8
  - main
    - heap
      - code_space
      ...
  - workers
    - heap
      - code_space
        - isolate_0x5678
        - isolate_0x9012
        ...

In this example, "isolate_0x1234" is the main isolate, so it is removed.
The other isolate nodes are for workers, so they are made into leaf nodes.

Besides that there are other minor changes:
- "heap_spaces" is renamed to "heap".
- the V8 code stats are reported under "code_stats" node.

Bug: chromium:852415
Change-Id: I6e4c6e6fddb6e8cf9be3fe6d5fca837ac6fbdc34
Reviewed-on: https://chromium-review.googlesource.com/1181263
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Siddhartha S <ssid@chromium.org>
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588387}

base/trace_event/memory_infra_background_whitelist.cc

@@ -261,19 +261,44 @@ const char* const kAllocatorDumpNameWhitelist[] = {
     "sqlite",
     "ui/resource_manager_0x?/default_resource/0x?",
     "ui/resource_manager_0x?/tinted_resource",
-    "v8/isolate_0x?/contexts/detached_context",
-    "v8/isolate_0x?/contexts/native_context",
-    "v8/isolate_0x?/heap_spaces",
-    "v8/isolate_0x?/heap_spaces/code_space",
-    "v8/isolate_0x?/heap_spaces/large_object_space",
-    "v8/isolate_0x?/heap_spaces/map_space",
-    "v8/isolate_0x?/heap_spaces/new_space",
-    "v8/isolate_0x?/heap_spaces/new_large_object_space",
-    "v8/isolate_0x?/heap_spaces/old_space",
-    "v8/isolate_0x?/heap_spaces/read_only_space",
-    "v8/isolate_0x?/malloc",
-    "v8/isolate_0x?/zapped_for_debug",
     "site_storage/blob_storage/0x?",
+    "v8/main/code_stats",
+    "v8/main/contexts/detached_context",
+    "v8/main/contexts/native_context",
+    "v8/main/heap/code_space",
+    "v8/main/heap/code_stats",
+    "v8/main/heap/large_object_space",
+    "v8/main/heap/map_space",
+    "v8/main/heap/new_large_object_space",
+    "v8/main/heap/new_space",
+    "v8/main/heap/old_space",
+    "v8/main/heap/read_only_space",
+    "v8/main/malloc",
+    "v8/main/zapped_for_debug",
+    "v8/utility/code_stats",
+    "v8/utility/contexts/detached_context",
+    "v8/utility/contexts/native_context",
+    "v8/utility/heap/code_space",
+    "v8/utility/heap/large_object_space",
+    "v8/utility/heap/map_space",
+    "v8/utility/heap/new_large_object_space",
+    "v8/utility/heap/new_space",
+    "v8/utility/heap/old_space",
+    "v8/utility/heap/read_only_space",
+    "v8/utility/malloc",
+    "v8/utility/zapped_for_debug",
+    "v8/workers/code_stats/isolate_0x?",
+    "v8/workers/contexts/detached_context/isolate_0x?",
+    "v8/workers/contexts/native_context/isolate_0x?",
+    "v8/workers/heap/code_space/isolate_0x?",
+    "v8/workers/heap/large_object_space/isolate_0x?",
+    "v8/workers/heap/map_space/isolate_0x?",
+    "v8/workers/heap/new_large_object_space/isolate_0x?",
+    "v8/workers/heap/new_space/isolate_0x?",
+    "v8/workers/heap/old_space/isolate_0x?",
+    "v8/workers/heap/read_only_space/isolate_0x?",
+    "v8/workers/malloc/isolate_0x?",
+    "v8/workers/zapped_for_debug/isolate_0x?",
     "site_storage/index_db/db_0x?",
     "site_storage/index_db/memenv_0x?",
     "site_storage/localstorage/0x?/cache_size",

gin/public/isolate_holder.h

@@ -70,8 +70,9 @@ class GIN_EXPORT IsolateHolder {
     kUtility
   };
 
-  IsolateHolder(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-                IsolateType isolate_type);
+  explicit IsolateHolder(
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+      IsolateType isolate_type);
   IsolateHolder(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
                 AccessMode access_mode,
                 IsolateType isolate_type);

gin/v8_isolate_memory_dump_provider.cc

@@ -50,9 +50,8 @@ namespace {
 
 // Dump statistics related to code/bytecode when memory-infra.v8.code_stats is
 // enabled.
-void DumpCodeStatistics(
-    base::trace_event::MemoryAllocatorDump* heap_spaces_dump,
-    IsolateHolder* isolate_holder) {
+void DumpCodeStatistics(base::trace_event::MemoryAllocatorDump* dump,
+                        IsolateHolder* isolate_holder) {
   // Collecting code statistics is an expensive operation (~10 ms) when
   // compared to other v8 metrics (< 1 ms). So, dump them only when
   // memory-infra.v8.code_stats is enabled.
@@ -71,37 +70,37 @@ void DumpCodeStatistics(
     return;
   }
 
-  heap_spaces_dump->AddScalar(
-      "code_and_metadata_size",
-      base::trace_event::MemoryAllocatorDump::kUnitsBytes,
-      code_statistics.code_and_metadata_size());
-  heap_spaces_dump->AddScalar(
-      "bytecode_and_metadata_size",
-      base::trace_event::MemoryAllocatorDump::kUnitsBytes,
-      code_statistics.bytecode_and_metadata_size());
-  heap_spaces_dump->AddScalar(
-      "external_script_source_size",
-      base::trace_event::MemoryAllocatorDump::kUnitsBytes,
-      code_statistics.external_script_source_size());
+  dump->AddScalar("code_and_metadata_size",
+                  base::trace_event::MemoryAllocatorDump::kUnitsBytes,
+                  code_statistics.code_and_metadata_size());
+  dump->AddScalar("bytecode_and_metadata_size",
+                  base::trace_event::MemoryAllocatorDump::kUnitsBytes,
+                  code_statistics.bytecode_and_metadata_size());
+  dump->AddScalar("external_script_source_size",
+                  base::trace_event::MemoryAllocatorDump::kUnitsBytes,
+                  code_statistics.external_script_source_size());
 }
 
 // Dump the number of native and detached contexts.
 // The result looks as follows in the Chrome trace viewer:
-// ======================================
-// Component                 object_count
+// ========================================
+// Component                   object_count
 // - v8
-//   - isolate
+//   - main
 //     - contexts
 //       - detached_context  10
 //       - native_context    20
-// ======================================
+// ========================================
 void DumpContextStatistics(
     base::trace_event::ProcessMemoryDump* process_memory_dump,
     std::string dump_base_name,
+    std::string dump_name_suffix,
     size_t number_of_detached_contexts,
     size_t number_of_native_contexts) {
-  std::string dump_name_prefix = dump_base_name + "/contexts";
-  std::string native_context_name = dump_name_prefix + "/native_context";
+  std::string dump_name_prefix =
+      dump_base_name + "/contexts" + dump_name_suffix;
+  std::string native_context_name =
+      dump_name_prefix + "/native_context" + dump_name_suffix;
   auto* native_context_dump =
       process_memory_dump->CreateAllocatorDump(native_context_name);
   native_context_dump->AddScalar(
@@ -115,20 +114,57 @@ void DumpContextStatistics(
       number_of_detached_contexts);
 }
 
+std::string IsolateTypeString(IsolateHolder::IsolateType isolate_type) {
+  switch (isolate_type) {
+    case IsolateHolder::IsolateType::kBlinkMainThread:
+      return "main";
+    case IsolateHolder::IsolateType::kBlinkWorkerThread:
+      return "workers";
+    case IsolateHolder::IsolateType::kTest:
+      LOG(FATAL) << "Unreachable code";
+      return "test";
+    case IsolateHolder::IsolateType::kUtility:
+      return "utility";
+  }
+  LOG(FATAL) << "Unreachable code";
+}
+
+bool CanHaveMultipleIsolates(IsolateHolder::IsolateType isolate_type) {
+  switch (isolate_type) {
+    case IsolateHolder::IsolateType::kBlinkMainThread:
+      return false;
+    case IsolateHolder::IsolateType::kBlinkWorkerThread:
+      return true;
+    case IsolateHolder::IsolateType::kTest:
+      LOG(FATAL) << "Unreachable code";
+      return false;
+    case IsolateHolder::IsolateType::kUtility:
+      // PDFium and ProxyResolver create one isolate per process.
+      return false;
+  }
+  LOG(FATAL) << "Unreachable code";
+}
+
 }  // namespace anonymous
 
 void V8IsolateMemoryDumpProvider::DumpHeapStatistics(
     const base::trace_event::MemoryDumpArgs& args,
     base::trace_event::ProcessMemoryDump* process_memory_dump) {
-  std::string dump_base_name = base::StringPrintf(
-      "v8/isolate_0x%" PRIXPTR,
+  std::string isolate_name = base::StringPrintf(
+      "isolate_0x%" PRIXPTR,
       reinterpret_cast<uintptr_t>(isolate_holder_->isolate()));
 
   // Dump statistics of the heap's spaces.
-  std::string space_name_prefix = dump_base_name + "/heap_spaces";
   v8::HeapStatistics heap_statistics;
   isolate_holder_->isolate()->GetHeapStatistics(&heap_statistics);
 
+  IsolateHolder::IsolateType isolate_type = isolate_holder_->isolate_type();
+  std::string dump_base_name = "v8/" + IsolateTypeString(isolate_type);
+  std::string dump_name_suffix =
+      CanHaveMultipleIsolates(isolate_type) ? "/" + isolate_name : "";
+
+  std::string space_name_prefix = dump_base_name + "/heap";
+
   size_t known_spaces_used_size = 0;
   size_t known_spaces_size = 0;
   size_t known_spaces_physical_size = 0;
@@ -145,14 +181,15 @@ void V8IsolateMemoryDumpProvider::DumpHeapStatistics(
     known_spaces_used_size += space_used_size;
     known_spaces_physical_size += space_physical_size;
 
-    std::string space_dump_name =
-        space_name_prefix + "/" + space_statistics.space_name();
+    std::string space_dump_name = dump_base_name + "/heap/" +
+                                  space_statistics.space_name() +
+                                  dump_name_suffix;
+
     auto* space_dump =
         process_memory_dump->CreateAllocatorDump(space_dump_name);
     space_dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
                           base::trace_event::MemoryAllocatorDump::kUnitsBytes,
                           space_physical_size);
-
     space_dump->AddScalar("virtual_size",
                           base::trace_event::MemoryAllocatorDump::kUnitsBytes,
                           space_size);
@@ -172,15 +209,15 @@ void V8IsolateMemoryDumpProvider::DumpHeapStatistics(
   // resident values.
   if (heap_statistics.does_zap_garbage()) {
     auto* zap_dump = process_memory_dump->CreateAllocatorDump(
-        dump_base_name + "/zapped_for_debug");
+        dump_base_name + "/zapped_for_debug" + dump_name_suffix);
     zap_dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
                         base::trace_event::MemoryAllocatorDump::kUnitsBytes,
                         heap_statistics.total_heap_size() -
                             heap_statistics.total_physical_size());
   }
 
   // Dump statistics about malloced memory.
-  std::string malloc_name = dump_base_name + "/malloc";
+  std::string malloc_name = dump_base_name + "/malloc" + dump_name_suffix;
   auto* malloc_dump = process_memory_dump->CreateAllocatorDump(malloc_name);
   malloc_dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
                          base::trace_event::MemoryAllocatorDump::kUnitsBytes,
@@ -196,17 +233,15 @@ void V8IsolateMemoryDumpProvider::DumpHeapStatistics(
                                           system_allocator_name);
   }
 
-  DumpContextStatistics(process_memory_dump, dump_base_name,
+  DumpContextStatistics(process_memory_dump, dump_base_name, dump_name_suffix,
                         heap_statistics.number_of_detached_contexts(),
                         heap_statistics.number_of_native_contexts());
 
-  // Add an empty row for the heap_spaces. This is to keep the shape of the
-  // dump stable, whether code stats are enabled or not.
-  auto* heap_spaces_dump =
-      process_memory_dump->CreateAllocatorDump(space_name_prefix);
+  auto* code_stats_dump = process_memory_dump->CreateAllocatorDump(
+      dump_base_name + "/code_stats" + dump_name_suffix);
 
   // Dump statistics related to code and bytecode if requested.
-  DumpCodeStatistics(heap_spaces_dump, isolate_holder_);
+  DumpCodeStatistics(code_stats_dump, isolate_holder_);
 
   // Dump object statistics only for detailed dumps.
   if (args.level_of_detail !=
@@ -217,7 +252,8 @@ void V8IsolateMemoryDumpProvider::DumpHeapStatistics(
   // Dump statistics of the heap's live objects from last GC.
   // TODO(primiano): these should not be tracked in the same trace event as they
   // report stats for the last GC (not the current state). See crbug.com/498779.
-  std::string object_name_prefix = dump_base_name + "/heap_objects_at_last_gc";
+  std::string object_name_prefix =
+      dump_base_name + "/heap_objects_at_last_gc" + dump_name_suffix;
   bool did_dump_object_stats = false;
   const size_t object_types =
       isolate_holder_->isolate()->NumberOfTrackedHeapObjectTypes();
@@ -256,7 +292,8 @@ void V8IsolateMemoryDumpProvider::DumpHeapStatistics(
   if (did_dump_object_stats) {
     process_memory_dump->AddOwnershipEdge(
         process_memory_dump->CreateAllocatorDump(object_name_prefix)->guid(),
-        heap_spaces_dump->guid());
+        process_memory_dump->GetOrCreateAllocatorDump(space_name_prefix)
+            ->guid());
   }
 }
 

gin/v8_isolate_memory_dump_provider_unittest.cc

@@ -38,12 +38,13 @@ TEST_F(V8MemoryDumpProviderTest, DumpStatistics) {
   bool did_dump_objects_stats = false;
   for (const auto& name_dump : allocator_dumps) {
     const std::string& name = name_dump.first;
-    if (name.find("v8/isolate") != std::string::npos) {
+    if (name.find("v8/main") != std::string::npos) {
       did_dump_isolate_stats = true;
     }
-    if (name.find("heap_spaces") != std::string::npos) {
+    if (name.find("v8/main/heap") != std::string::npos) {
       did_dump_space_stats = true;
-    } else if (name.find("heap_objects") != std::string::npos) {
+    }
+    if (name.find("v8/main/heap_objects") != std::string::npos) {
       did_dump_objects_stats = true;
     }
   }
@@ -101,7 +102,7 @@ TEST_F(V8MemoryDumpProviderTest, DumpCodeStatistics) {
 
   for (const auto& name_dump : allocator_dumps) {
     const std::string& name = name_dump.first;
-    if (name.find("heap_spaces") != std::string::npos) {
+    if (name.find("code_stats") != std::string::npos) {
       for (const base::trace_event::MemoryAllocatorDump::Entry& entry :
            name_dump.second->entries()) {
         if (entry.name == "bytecode_and_metadata_size") {