Heap flamegraph: sort roots by name to improve stability

src/trace_processor/importers/proto/heap_graph_tracker.cc

@@ -164,6 +164,33 @@ constexpr std::array<protos::pbzero::HeapGraphRoot::Type, 3>
         protos::pbzero::HeapGraphRoot::ROOT_JNI_GLOBAL,
         protos::pbzero::HeapGraphRoot::ROOT_JNI_LOCAL,
 };
+
+void SortRoots(TraceStorage* storage,
+               const std::set<ObjectTable::RowNumber>& roots,
+               std::vector<ObjectTable::RowNumber>& sorted_roots) {
+  std::vector<std::pair<base::StringView, ObjectTable::RowNumber>>
+      sorted_with_names;
+  sorted_with_names.reserve(roots.size());
+  auto* object_table = storage->mutable_heap_graph_object_table();
+  auto* class_table = storage->mutable_heap_graph_class_table();
+  for (ObjectTable::RowNumber root : roots) {
+    auto obj_row = root.ToRowReference(object_table);
+    auto cls_row = *class_table->FindById(obj_row.type_id());
+    sorted_with_names.emplace_back(storage->GetString(cls_row.name()), root);
+  }
+  std::sort(sorted_with_names.begin(), sorted_with_names.end(),
+            [](const auto& a, const auto& b) {
+              if (a.first != b.first) {
+                return a.first < b.first;
+              }
+              return a.second.row_number() < b.second.row_number();
+            });
+  sorted_roots.clear();
+  sorted_roots.reserve(roots.size());
+  for (const auto& p : sorted_with_names) {
+    sorted_roots.push_back(p.second);
+  }
+}
 }  // namespace
 
 std::optional<base::StringView> GetStaticClassTypeName(base::StringView type) {
@@ -1141,7 +1168,10 @@ HeapGraphTracker::BuildFlamegraph(const int64_t current_ts,
 
   // First pass to calculate shortest paths
   PathFromRoot init_path;
-  for (ObjectTable::RowNumber root : roots) {
+  std::vector<ObjectTable::RowNumber> sorted_roots;
+  SortRoots(storage_, roots, sorted_roots);
+
+  for (ObjectTable::RowNumber root : sorted_roots) {
     FindPathFromRoot(root.ToRowReference(object_table), &init_path);
   }
 

src/trace_processor/importers/proto/heap_graph_tracker_unittest.cc

@@ -439,6 +439,142 @@ TEST(HeapGraphTrackerTest, BuildFlamegraphWeakReferences) {
   EXPECT_THAT(counts, UnorderedElementsAre(1, 1));
 }
 
+TEST(HeapGraphTrackerTest, ShortestPathStability) {
+  // Root -> ParentA (Class A) -> Child
+  // Root -> ParentB (Class B) -> Child
+  // We want to see Child attributed to ParentA if A < B alphabetically.
+
+  constexpr uint64_t kSeqId = 1;
+  constexpr UniquePid kPid = 1;
+  constexpr int64_t kTimestamp = 1;
+
+  TraceProcessorContext context;
+  context.storage.reset(new TraceStorage());
+  context.process_tracker.reset(new ProcessTracker(&context));
+  context.process_tracker->GetOrCreateProcess(kPid);
+
+  HeapGraphTracker tracker(context.storage.get());
+
+  constexpr uint64_t kField = 1;
+  constexpr uint64_t kLocation = 0;
+
+  constexpr uint64_t kRoot = 1;
+  constexpr uint64_t kParentA = 3;  // ID 3, but Class A
+  constexpr uint64_t kParentB = 2;  // ID 2, but Class B
+  constexpr uint64_t kChild = 4;
+
+  auto field = base::StringView("foo");
+  StringPool::Id root_class = context.storage->InternString("RootClass");
+  StringPool::Id a_class = context.storage->InternString("A_Parent");
+  StringPool::Id b_class = context.storage->InternString("B_Parent");
+  StringPool::Id child_class = context.storage->InternString("ChildClass");
+
+  tracker.AddInternedFieldName(kSeqId, kField, field);
+  tracker.AddInternedLocationName(kSeqId, kLocation,
+                                  context.storage->InternString("location"));
+
+  tracker.AddInternedType(kSeqId, kRoot, root_class, kLocation, 0, {}, 0, 0,
+                          false, protos::pbzero::HeapGraphType::KIND_NORMAL);
+  tracker.AddInternedType(kSeqId, kParentA, a_class, kLocation, 0, {}, 0, 0,
+                          false, protos::pbzero::HeapGraphType::KIND_NORMAL);
+  tracker.AddInternedType(kSeqId, kParentB, b_class, kLocation, 0, {}, 0, 0,
+                          false, protos::pbzero::HeapGraphType::KIND_NORMAL);
+  tracker.AddInternedType(kSeqId, kChild, child_class, kLocation, 0, {}, 0, 0,
+                          false, protos::pbzero::HeapGraphType::KIND_NORMAL);
+
+  // Object 1: Root
+  {
+    HeapGraphTracker::SourceObject obj;
+    obj.object_id = 1;
+    obj.self_size = 1;
+    obj.type_id = kRoot;
+    obj.field_name_ids = {kField, kField};
+    obj.referred_objects = {2, 3};  // ParentB, ParentA
+    tracker.AddObject(kSeqId, kPid, kTimestamp, std::move(obj));
+  }
+  // Object 2: ParentB
+  {
+    HeapGraphTracker::SourceObject obj;
+    obj.object_id = 2;
+    obj.self_size = 100;
+    obj.type_id = kParentB;
+    obj.field_name_ids = {kField};
+    obj.referred_objects = {4};  // Child
+    tracker.AddObject(kSeqId, kPid, kTimestamp, std::move(obj));
+  }
+  // Object 3: ParentA
+  {
+    HeapGraphTracker::SourceObject obj;
+    obj.object_id = 3;
+    obj.self_size = 10;
+    obj.type_id = kParentA;
+    obj.field_name_ids = {kField};
+    obj.referred_objects = {4};  // Child
+    tracker.AddObject(kSeqId, kPid, kTimestamp, std::move(obj));
+  }
+  // Object 4: Child
+  {
+    HeapGraphTracker::SourceObject obj;
+    obj.object_id = 4;
+    obj.self_size = 1000;
+    obj.type_id = kChild;
+    tracker.AddObject(kSeqId, kPid, kTimestamp, std::move(obj));
+  }
+
+  HeapGraphTracker::SourceRoot root;
+  root.root_type = protos::pbzero::HeapGraphRoot::ROOT_UNKNOWN;
+  root.object_ids.emplace_back(1);
+  root.object_ids.emplace_back(2);  // ParentB is also a root
+  root.object_ids.emplace_back(3);  // ParentA is also a root
+  tracker.AddRoot(kSeqId, kPid, kTimestamp, root);
+
+  tracker.FinalizeAllProfiles();
+  std::unique_ptr<tables::ExperimentalFlamegraphTable> flame =
+      tracker.BuildFlamegraph(kPid, kTimestamp);
+  ASSERT_NE(flame, nullptr);
+
+  // Check that Child (size 1000) is under ParentA (A_Parent) and not ParentB
+  // (B_Parent).
+  // Flamegraph structure:
+  // Root (size 1)
+  //   A_Parent (size 10)
+  //     ChildClass (size 1000)
+  //   B_Parent (size 100)
+
+  bool found_child_under_a = false;
+  bool found_child_under_b = false;
+
+  for (auto it = flame->IterateRows(); it; ++it) {
+    auto name = context.storage->string_pool().Get(it.name());
+    auto parent_id = it.parent_id();
+    if (name == "ChildClass") {
+      ASSERT_TRUE(parent_id.has_value());
+      // To get the parent name, we need to find the parent row.
+      // Since we don't have an easy way to lookup by ID on the flame table
+      // without building an index, and we know the parent is likely before the
+      // child, we can just iterate again or store names.
+      // For simplicity in this test, let's just iterate and find the parent
+      // name by its ID.
+      std::optional<std::string> parent_name;
+      for (auto pit = flame->IterateRows(); pit; ++pit) {
+        if (pit.id().value == parent_id->value) {
+          parent_name =
+              context.storage->string_pool().Get(pit.name()).ToStdString();
+          break;
+        }
+      }
+      ASSERT_TRUE(parent_name.has_value());
+      if (*parent_name == "A_Parent [ROOT_UNKNOWN]") {
+        found_child_under_a = true;
+      } else if (*parent_name == "B_Parent [ROOT_UNKNOWN]") {
+        found_child_under_b = true;
+      }
+    }
+  }
+  EXPECT_TRUE(found_child_under_a);
+  EXPECT_FALSE(found_child_under_b);
+}
+
 constexpr char kArray[] = "X[]";
 constexpr char kDoubleArray[] = "X[][]";
 constexpr char kNoArray[] = "X";

src/traceconv/trace_to_pprof_integrationtest.cc

@@ -200,7 +200,7 @@ TEST_F(TraceToPprofRealTraceTest, AllocationCountForClass) {
       pprof.get_samples("android.content.pm.parsing.component.ParsedActivity")
           .size(),
       5U);
-  EXPECT_EQ(pprof.get_sample_count(), 83256U);
+  EXPECT_EQ(pprof.get_sample_count(), 83028U);
 
   const std::vector<std::string> expected_function_names = {
       "android.content.pm.parsing.component.ParsedActivity",

test/trace_processor/diff_tests/parser/profiling/heap_graph_flamegraph_system-server-heap-graph.out

@@ -1,11 +1,11 @@
 "id","depth","name","map_name","count","cumulative_count","size","cumulative_size","parent_id"
-0,0,"java.lang.Class<java.lang.Object[]> [ROOT_STICKY_CLASS]","JAVA",1,3,224,292,"[NULL]"
-1,1,"java.lang.Object[]","JAVA",1,1,28,28,0
-2,1,"java.lang.String","JAVA",1,1,40,40,0
-3,0,"java.lang.String [ROOT_INTERNED_STRING]","JAVA",41197,41197,1845640,1845640,"[NULL]"
-4,0,"java.lang.Class<android.content.pm.ApplicationInfo> [ROOT_STICKY_CLASS]","JAVA",1,5,1152,1268,"[NULL]"
-5,1,"java.lang.String","JAVA",1,1,56,56,4
-6,1,"android.content.pm.ApplicationInfo$1","JAVA",1,1,8,8,4
-7,1,"java.lang.Object[]","JAVA",1,2,20,52,4
-8,2,"long[]","JAVA",1,1,32,32,7
-9,0,"java.lang.Class<java.io.File> [ROOT_STICKY_CLASS]","JAVA",1,11,645,997,"[NULL]"
+0,0,"android.app.LoadedApk$ServiceDispatcher$DeathMonitor [ROOT_JNI_GLOBAL]","JAVA",32,789,640,54860,"[NULL]"
+1,1,"android.app.LoadedApk$ServiceDispatcher","JAVA",31,683,1395,51674,0
+2,2,"android.app.ActivityThread$H","JAVA",1,1,32,32,1
+3,2,"android.app.LoadedApk$ServiceDispatcher$InnerConnection","JAVA",31,62,868,1612,1
+4,3,"java.lang.ref.WeakReference","JAVA",31,31,744,744,3
+5,2,"android.util.ArrayMap","JAVA",31,125,775,3519,1
+6,3,"java.lang.Object[]","JAVA",31,63,1364,1876,5
+7,4,"android.app.LoadedApk$ServiceDispatcher$ConnectionInfo","JAVA",32,32,512,512,6
+8,3,"int[]","JAVA",31,31,868,868,5
+9,2,"android.app.ServiceConnectionLeaked","JAVA",31,145,868,22009,1