Stop double counting self-referenced classes (flutter#4955)

Author: polina-c

packages/devtools_app/lib/src/screens/memory/panes/diff/controller/heap_diff.dart

@@ -166,13 +166,17 @@ class ObjectSetDiff {
       final object = before.objectsByCodes[code] ?? after.objectsByCodes[code]!;
 
       if (inBefore) {
-        deleted.countInstance(object);
-        delta.uncountInstance(object);
+        final excludeFromRetained =
+            before.notCountedInRetained.contains(object.code);
+        deleted.countInstance(object, excludeFromRetained: excludeFromRetained);
+        delta.uncountInstance(object, excludeFromRetained: excludeFromRetained);
         continue;
       }
       if (inAfter) {
-        created.countInstance(object);
-        delta.countInstance(object);
+        final excludeFromRetained =
+            after.notCountedInRetained.contains(object.code);
+        created.countInstance(object, excludeFromRetained: excludeFromRetained);
+        delta.countInstance(object, excludeFromRetained: excludeFromRetained);
         continue;
       }
       assert(false);

packages/devtools_app/lib/src/screens/memory/shared/heap/heap.dart

@@ -133,15 +133,19 @@ class SingleClassStats extends ClassStats {
     assert(!isSealed);
     final object = data.objects[objectIndex];
     assert(object.heapClass.fullName == heapClass.fullName);
-    objects.countInstance(object);
 
     final path = data.retainingPath(objectIndex);
+    objects.countInstance(
+      object,
+      excludeFromRetained: path?.isRetainedBySameClass ?? false,
+    );
+
     if (path == null) return;
     final objectsForPath = statsByPath.putIfAbsent(
       ClassOnlyHeapPath(path),
       () => ObjectSet(),
     );
-    objectsForPath.countInstance(object);
+    objectsForPath.countInstance(object, excludeFromRetained: false);
   }
 
   bool get isZero => objects.isZero;
@@ -174,16 +178,22 @@ class ObjectSetStats with Sealable {
   bool get isZero =>
       shallowSize == 0 && retainedSize == 0 && instanceCount == 0;
 
-  void countInstance(AdaptedHeapObject object) {
+  void countInstance(
+    AdaptedHeapObject object, {
+    required bool excludeFromRetained,
+  }) {
     assert(!isSealed);
-    retainedSize += object.retainedSize!;
+    if (!excludeFromRetained) retainedSize += object.retainedSize!;
     shallowSize += object.shallowSize;
     instanceCount++;
   }
 
-  void uncountInstance(AdaptedHeapObject object) {
+  void uncountInstance(
+    AdaptedHeapObject object, {
+    required bool excludeFromRetained,
+  }) {
     assert(!isSealed);
-    retainedSize -= object.retainedSize!;
+    if (!excludeFromRetained) retainedSize -= object.retainedSize!;
     shallowSize -= object.shallowSize;
     instanceCount--;
   }
@@ -194,19 +204,27 @@ class ObjectSet extends ObjectSetStats {
   static ObjectSet empty = ObjectSet()..seal();
 
   final objectsByCodes = <IdentityHashCode, AdaptedHeapObject>{};
+  final notCountedInRetained = <IdentityHashCode>{};
 
   @override
   bool get isZero => objectsByCodes.isEmpty;
 
   @override
-  void countInstance(AdaptedHeapObject object) {
+  void countInstance(
+    AdaptedHeapObject object, {
+    required bool excludeFromRetained,
+  }) {
     if (objectsByCodes.containsKey(object.code)) return;
-    super.countInstance(object);
+    super.countInstance(object, excludeFromRetained: excludeFromRetained);
     objectsByCodes[object.code] = object;
+    if (excludeFromRetained) notCountedInRetained.add(object.code);
   }
 
   @override
-  void uncountInstance(AdaptedHeapObject object) {
+  void uncountInstance(
+    AdaptedHeapObject object, {
+    required bool excludeFromRetained,
+  }) {
     throw AssertionError('uncountInstance is not valid for $ObjectSet');
   }
 }

packages/devtools_app/lib/src/screens/memory/shared/heap/model.dart

@@ -108,21 +108,26 @@ class AdaptedHeapData {
 typedef IdentityHashCode = int;
 
 /// Sequence of ids of objects in the heap.
-///
-/// TODO(polina-c): maybe we do not need to store path by objects.
-/// It can be that only classes are interesting, and we can save some
-/// performance on this object. It will become clear when the leak tracking
-/// feature stabilizes.
 class HeapPath {
   HeapPath(this.objects);
 
   final List<AdaptedHeapObject> objects;
 
+  late final bool isRetainedBySameClass = () {
+    if (objects.length < 2) return false;
+
+    final theClass = objects.last.heapClass;
+
+    return objects
+        .take(objects.length - 1)
+        .any((object) => object.heapClass == theClass);
+  }();
+
   /// Retaining path for the object in string format.
-  String? shortPath() => '/${objects.map((o) => o.shortName).join('/')}/';
+  String shortPath() => '/${objects.map((o) => o.shortName).join('/')}/';
 
   /// Retaining path for the object as an array of the retaining objects.
-  List<String>? detailedPath() =>
+  List<String> detailedPath() =>
       objects.map((o) => o.name).toList(growable: false);
 }
 

packages/devtools_app/test/memory/shared/heap/heap_analyzer_test.dart

@@ -230,14 +230,9 @@ class _SizeTest {
     required this.unreachableSize,
   }) : assert(_assertHeapIndexIsCode(heap));
 
-  /// For convenience of testing each heap object has code equal to the
-  /// index in array.
   final AdaptedHeapData heap;
-
   final String name;
-
   final int rootRetainedSize;
-
   final int unreachableSize;
 }
 
@@ -272,6 +267,8 @@ AdaptedHeapObject _createOneByteWeakObject(
   return result;
 }
 
+/// For convenience of testing each heap object has code equal to the
+/// index in array.
 bool _assertHeapIndexIsCode(AdaptedHeapData heap) => heap.objects
     .asMap()
     .entries

packages/devtools_app/test/memory/shared/heap/heap_test.dart

@@ -0,0 +1,119 @@
+// Copyright 2022 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.
+
+import 'package:devtools_app/src/screens/memory/shared/heap/heap.dart';
+import 'package:devtools_app/src/screens/memory/shared/heap/model.dart';
+import 'package:devtools_app/src/screens/memory/shared/heap/spanning_tree.dart';
+import 'package:devtools_app/src/screens/memory/shared/primitives/class_name.dart';
+
+import 'package:flutter_test/flutter_test.dart';
+
+class _ClassSizeTest {
+  _ClassSizeTest({
+    required this.name,
+    required this.heap,
+    required this.expectedClassARetainedSize,
+  }) : assert(_assertHeapIndexIsCode(heap)) {
+    buildSpanningTree(heap);
+  }
+
+  final AdaptedHeapData heap;
+  final String name;
+  final int expectedClassARetainedSize;
+}
+
+final _root = HeapClassName(className: 'Root', library: 'l');
+final _classA = HeapClassName(className: 'A', library: 'l');
+final _classB = HeapClassName(className: 'B', library: 'l');
+
+final _classSizeTests = <_ClassSizeTest>[
+  _ClassSizeTest(
+    name: 'separate',
+    heap: AdaptedHeapData(
+      [
+        _createOneByteObject(0, [1, 2, 3], _root),
+        _createOneByteObject(1, [], _classA),
+        _createOneByteObject(2, [], _classA),
+        _createOneByteObject(3, [], _classA),
+      ],
+      rootIndex: 0,
+    ),
+    expectedClassARetainedSize: 3,
+  ),
+  _ClassSizeTest(
+    name: 'linked',
+    heap: AdaptedHeapData(
+      [
+        _createOneByteObject(0, [1], _root),
+        _createOneByteObject(1, [2], _classA),
+        _createOneByteObject(2, [3], _classA),
+        _createOneByteObject(3, [], _classA),
+      ],
+      rootIndex: 0,
+    ),
+    expectedClassARetainedSize: 3,
+  ),
+  _ClassSizeTest(
+    name: 'full graph',
+    heap: AdaptedHeapData(
+      [
+        _createOneByteObject(0, [1], _root),
+        _createOneByteObject(1, [2, 3], _classA),
+        _createOneByteObject(2, [3, 1], _classA),
+        _createOneByteObject(3, [1, 2], _classA),
+      ],
+      rootIndex: 0,
+    ),
+    expectedClassARetainedSize: 3,
+  ),
+  _ClassSizeTest(
+    name: 'with global B',
+    heap: AdaptedHeapData(
+      [
+        _createOneByteObject(0, [1], _root),
+        _createOneByteObject(1, [2, 4], _classA),
+        _createOneByteObject(2, [3, 4], _classA),
+        _createOneByteObject(3, [4], _classA),
+        _createOneByteObject(4, [], _classB),
+      ],
+      rootIndex: 0,
+    ),
+    expectedClassARetainedSize: 4,
+  ),
+];
+
+void main() {
+  test('$SingleClassStats does not double-count self-referenced classes.', () {
+    for (final t in _classSizeTests) {
+      final classes = SingleClassStats(_classA);
+      for (final o in t.heap.objects) {
+        if (o.heapClass == _classA) classes.countInstance(t.heap, o.code);
+      }
+      expect(
+        classes.objects.retainedSize,
+        t.expectedClassARetainedSize,
+        reason: t.name,
+      );
+    }
+  });
+}
+
+AdaptedHeapObject _createOneByteObject(
+  int codeAndIndex,
+  List<int> references,
+  HeapClassName theClass,
+) =>
+    AdaptedHeapObject(
+      code: codeAndIndex,
+      references: references,
+      heapClass: theClass,
+      shallowSize: 1,
+    );
+
+/// For convenience of testing each heap object has code equal to the
+/// index in array.
+bool _assertHeapIndexIsCode(AdaptedHeapData heap) => heap.objects
+    .asMap()
+    .entries
+    .every((entry) => entry.key == entry.value.code);

packages/devtools_app/test/memory/shared/heap/model_test.dart

@@ -6,6 +6,56 @@ import 'package:devtools_app/src/screens/memory/shared/heap/model.dart';
 import 'package:devtools_app/src/screens/memory/shared/primitives/class_name.dart';
 import 'package:flutter_test/flutter_test.dart';
 
+class _HeapPathTest {
+  _HeapPathTest(
+    this.name,
+    this.heapPath, {
+    required this.isRetainedBySameClass,
+  });
+
+  final String name;
+  final HeapPath heapPath;
+  final bool isRetainedBySameClass;
+}
+
+final _heapPathTests = <_HeapPathTest>[
+  _HeapPathTest(
+    'empty',
+    HeapPath([]),
+    isRetainedBySameClass: false,
+  ),
+  _HeapPathTest(
+    'one item',
+    HeapPath([_objectForClass('myLib', 'myClass')]),
+    isRetainedBySameClass: false,
+  ),
+  _HeapPathTest(
+    'two different',
+    HeapPath([
+      _objectForClass('myLib1', 'myClass'),
+      _objectForClass('myLib2', 'myClass'),
+    ]),
+    isRetainedBySameClass: false,
+  ),
+  _HeapPathTest(
+    'two identical',
+    HeapPath([
+      _objectForClass('myLib', 'myClass'),
+      _objectForClass('myLib', 'myClass'),
+    ]),
+    isRetainedBySameClass: true,
+  ),
+  _HeapPathTest(
+    'three identical',
+    HeapPath([
+      _objectForClass('myLib', 'myClass'),
+      _objectForClass('myLib', 'myClass'),
+      _objectForClass('myLib', 'myClass'),
+    ]),
+    isRetainedBySameClass: true,
+  ),
+];
+
 void main() {
   test('$AdaptedHeapData serializes.', () {
     final json = AdaptedHeapData(
@@ -26,4 +76,22 @@ void main() {
 
     expect(json, AdaptedHeapData.fromJson(json).toJson());
   });
+
+  test('$HeapPath.isRetainedBySameClass returns expected result for.', () {
+    for (final t in _heapPathTests) {
+      expect(
+        t.heapPath.isRetainedBySameClass,
+        t.isRetainedBySameClass,
+        reason: t.name,
+      );
+    }
+  });
 }
+
+AdaptedHeapObject _objectForClass(String lib, String theClass) =>
+    AdaptedHeapObject(
+      code: 1,
+      references: [],
+      heapClass: HeapClassName(className: theClass, library: lib),
+      shallowSize: 1,
+    );