Document BFS dominator tree approximation bug and add regression test

build.gradle.kts

@@ -84,6 +84,10 @@ subprojects {
     }
   }
 
+  dependencies {
+    "detektPlugins"(rootProject.libs.detekt.formatting)
+  }
+
   extensions.configure<DetektExtension> {
     config = rootProject.files("config/detekt-config.yml")
     parallel = true
@@ -158,7 +162,13 @@ configure(subprojects.filter {
 //Git hook installation
 tasks.register<Copy>("installGitHooks") {
   from(File(rootProject.rootDir, "config/hooks"))
-  into({ File(rootProject.rootDir, ".git/hooks") })
+  into({
+    val gitCommonDir = providers.exec {
+      commandLine("git", "rev-parse", "--git-common-dir")
+      workingDir = rootProject.rootDir
+    }.standardOutput.asText.get().trim()
+    File(gitCommonDir, "hooks")
+  })
   fileMode = "0777".toInt(8) // Make files executable
 }
 

leakcanary/leakcanary-app/src/main/java/org/leakcanary/screens/TreeMapScreen.kt

@@ -33,14 +33,27 @@ import org.leakcanary.screens.Destination.TreeMapDestination
 import org.leakcanary.screens.TreeMapState.Loading
 import org.leakcanary.screens.TreeMapState.Success
 import org.leakcanary.screens.TreemapLayout.NodeValue
+import java.io.Serializable
+import shark.ActualMatchingReferenceReaderFactory
+import shark.AndroidObjectSizeCalculator
 import shark.AndroidReferenceMatchers
+import shark.DominatorNode
+import shark.HeapObject.HeapClass
+import shark.HeapObject.HeapInstance
+import shark.HeapObject.HeapObjectArray
+import shark.HeapObject.HeapPrimitiveArray
 import shark.HprofHeapGraph.Companion.openHeapGraph
 import shark.IgnoredReferenceMatcher
-import shark.ObjectDominators
-import shark.ObjectDominators.OfflineDominatorNode
+import shark.LinkEvalDominatorTree
+import shark.MatchingGcRootProvider
 import shark.ReferenceMatcher
 import shark.ValueHolder
 
+data class OfflineDominatorNode(
+  val node: DominatorNode,
+  val name: String
+) : Serializable
+
 sealed interface TreeMapState {
   object Loading : TreeMapState
   class Success(val dominators: Map<Long, OfflineDominatorNode>) : TreeMapState
@@ -70,7 +83,24 @@ class TreeMapViewModel @Inject constructor(
             matcher as IgnoredReferenceMatcher
           }
 
-        ObjectDominators().buildOfflineDominatorTree(heapGraph, ignoredRefs)
+        val dominators = LinkEvalDominatorTree(
+          heapGraph,
+          ActualMatchingReferenceReaderFactory(emptyList()),
+          MatchingGcRootProvider(ignoredRefs)
+        ).compute(AndroidObjectSizeCalculator(heapGraph))
+        dominators.mapValues { (objectId, node) ->
+          val name = if (objectId == ValueHolder.NULL_REFERENCE) {
+            "root"
+          } else {
+            when (val heapObject = heapGraph.findObjectById(objectId)) {
+              is HeapClass -> "class ${heapObject.name}"
+              is HeapInstance -> heapObject.instanceClassName
+              is HeapObjectArray -> heapObject.arrayClassName
+              is HeapPrimitiveArray -> heapObject.arrayClassName
+            }
+          }
+          OfflineDominatorNode(node, name)
+        }
       }
     }
     emit(Success(result))
@@ -158,7 +188,24 @@ fun OnDeviceHeapTreemapPreview() {
         matcher as IgnoredReferenceMatcher
       }
 
-    ObjectDominators().buildOfflineDominatorTree(heapGraph, ignoredRefs)
+    val dominators = LinkEvalDominatorTree(
+      heapGraph,
+      ActualMatchingReferenceReaderFactory(emptyList()),
+      MatchingGcRootProvider(ignoredRefs)
+    ).compute(AndroidObjectSizeCalculator(heapGraph))
+    dominators.mapValues { (objectId, node) ->
+      val name = if (objectId == ValueHolder.NULL_REFERENCE) {
+        "root"
+      } else {
+        when (val heapObject = heapGraph.findObjectById(objectId)) {
+          is HeapClass -> "class ${heapObject.name}"
+          is HeapInstance -> heapObject.instanceClassName
+          is HeapObjectArray -> heapObject.arrayClassName
+          is HeapPrimitiveArray -> heapObject.arrayClassName
+        }
+      }
+      OfflineDominatorNode(node, name)
+    }
   }
   val root = ValueHolder.NULL_REFERENCE
   val treemapInput = DominatorNodeMapper(

shark/shark-android/src/test/java/shark/HprofIOPerfTest.kt

@@ -179,7 +179,7 @@ class HprofIOPerfTest {
     )
       .isEqualTo(
         listOf(
-          19713, 40.0, 1021868, 20981, 40.0, 1079140
+          19711, 40.0, 1021265, 20979, 40.0, 1078529
         )
       )
   }
@@ -197,7 +197,7 @@ class HprofIOPerfTest {
     )
       .isEqualTo(
         listOf(
-          17408, 40.0, 1954105, 17413, 40.0, 1954285
+          17407, 40.0, 1953885, 17412, 40.0, 1954065
         )
       )
   }
@@ -215,7 +215,7 @@ class HprofIOPerfTest {
     )
       .isEqualTo(
         listOf(
-          11787, 32.0, 554413, 11789, 32.0, 554477
+          11786, 32.0, 554362, 11788, 32.0, 554426
         )
       )
   }
@@ -259,7 +259,11 @@ class HprofIOPerfTest {
           listener = {},
           referenceReaderFactory = AndroidReferenceReaderFactory(referenceMatchers),
           gcRootProvider = MatchingGcRootProvider(referenceMatchers),
-          computeRetainedHeapSize = computeRetainedHeapSize,
+          objectSizeCalculatorFactory = if (computeRetainedHeapSize) {
+            ObjectSizeCalculator.Factory { heapGraph -> AndroidObjectSizeCalculator(heapGraph) }
+          } else {
+            null
+          },
         ),
         objectInspectors = AndroidObjectInspectors.appDefaults,
         listener = {}

shark/shark-android/src/test/java/shark/HprofRetainedHeapPerfTest.kt

@@ -19,7 +19,6 @@ import shark.HprofHeapGraph.Companion.openHeapGraph
 import shark.OnAnalysisProgressListener.Step.COMPUTING_NATIVE_RETAINED_SIZE
 import shark.OnAnalysisProgressListener.Step.COMPUTING_RETAINED_SIZE
 import shark.OnAnalysisProgressListener.Step.EXTRACTING_METADATA
-import shark.OnAnalysisProgressListener.Step.FINDING_DOMINATORS
 import shark.OnAnalysisProgressListener.Step.FINDING_PATHS_TO_RETAINED_OBJECTS
 import shark.OnAnalysisProgressListener.Step.FINDING_RETAINED_OBJECTS
 import shark.OnAnalysisProgressListener.Step.INSPECTING_OBJECTS
@@ -49,9 +48,9 @@ class HprofRetainedHeapPerfTest {
       baselineHeap to heapWithIndex
     }
 
-    val (analysisRetained, _) = heapWithIndex.retainedHeap(ANALYSIS_THREAD)
+    val analysisRetained = heapWithIndex.retainedHeap(ANALYSIS_THREAD)
 
-    val retained = analysisRetained - baselineHeap.retainedHeap(ANALYSIS_THREAD).first
+    val retained = analysisRetained - baselineHeap.retainedHeap(ANALYSIS_THREAD)
 
     assertThat(retained).isEqualTo(4.5 MB +-5 % margin)
   }
@@ -66,9 +65,9 @@ class HprofRetainedHeapPerfTest {
       baselineHeap to heapWithIndex
     }
 
-    val (analysisRetained, _) = heapWithIndex.retainedHeap(ANALYSIS_THREAD)
+    val analysisRetained = heapWithIndex.retainedHeap(ANALYSIS_THREAD)
 
-    val retained = analysisRetained - baselineHeap.retainedHeap(ANALYSIS_THREAD).first
+    val retained = analysisRetained - baselineHeap.retainedHeap(ANALYSIS_THREAD)
 
     assertThat(retained).isEqualTo(4.4 MB +-5 % margin)
   }
@@ -104,20 +103,18 @@ class HprofRetainedHeapPerfTest {
       baselineHeap
     }
 
-    val retainedBeforeAnalysis = baselineHeap.retainedHeap(ANALYSIS_THREAD).first
+    val retainedBeforeAnalysis = baselineHeap.retainedHeap(ANALYSIS_THREAD)
     val retained = stepsToHeapDumpFile.mapValues {
-      val retainedPair = it.value.retainedHeap(ANALYSIS_THREAD, computeDominators = true)
-      retainedPair.first - retainedBeforeAnalysis to retainedPair.second
+      it.value.retainedHeap(ANALYSIS_THREAD) - retainedBeforeAnalysis
     }
 
-    assertThat(retained after PARSING_HEAP_DUMP).isEqualTo(5.01 MB +-5 % margin)
-    assertThat(retained after EXTRACTING_METADATA).isEqualTo(5.06 MB +-5 % margin)
-    assertThat(retained after FINDING_RETAINED_OBJECTS).isEqualTo(5.16 MB +-5 % margin)
-    assertThat(retained after FINDING_PATHS_TO_RETAINED_OBJECTS).isEqualTo(6.56 MB +-5 % margin)
-    assertThat(retained after FINDING_DOMINATORS).isEqualTo(6.56 MB +-5 % margin)
-    assertThat(retained after INSPECTING_OBJECTS).isEqualTo(6.57 MB +-5 % margin)
-    assertThat(retained after COMPUTING_NATIVE_RETAINED_SIZE).isEqualTo(6.57 MB +-5 % margin)
-    assertThat(retained after COMPUTING_RETAINED_SIZE).isEqualTo(5.49 MB +-5 % margin)
+    assertThat(retained after PARSING_HEAP_DUMP).isEqualTo(4.98 MB +-10 % margin)
+    assertThat(retained after EXTRACTING_METADATA).isEqualTo(5.20 MB +-10 % margin)
+    assertThat(retained after FINDING_RETAINED_OBJECTS).isEqualTo(5.25 MB +-10 % margin)
+    assertThat(retained after FINDING_PATHS_TO_RETAINED_OBJECTS).isEqualTo(6.00 MB +-10 % margin)
+    assertThat(retained after INSPECTING_OBJECTS).isEqualTo(6.00 MB +-10 % margin)
+    assertThat(retained after COMPUTING_NATIVE_RETAINED_SIZE).isEqualTo(6.00 MB +-10 % margin)
+    assertThat(retained after COMPUTING_RETAINED_SIZE).isEqualTo(6.00 MB +-10 % margin)
   }
 
   private fun indexRecordsOf(hprofFile: File): HprofIndex {
@@ -161,26 +158,23 @@ class HprofRetainedHeapPerfTest {
     return result
   }
 
-  private infix fun Map<OnAnalysisProgressListener.Step, Pair<Bytes, String>>.after(step: OnAnalysisProgressListener.Step): Pair<Bytes, String> {
+  private infix fun Map<OnAnalysisProgressListener.Step, Bytes>.after(step: OnAnalysisProgressListener.Step): Bytes {
     val values = OnAnalysisProgressListener.Step.values()
     for (nextOrdinal in step.ordinal + 1 until values.size) {
-      val pair = this[values[nextOrdinal]]
-      if (pair != null) {
-        val (nextStepRetained, dominatorTree) = pair
-
-        return nextStepRetained to "\n$nextStepRetained retained by analysis thread after step ${step.name} not valid\n" + dominatorTree
+      val bytes = this[values[nextOrdinal]]
+      if (bytes != null) {
+        return bytes
       }
     }
     error("No step in $this after $step")
   }
 
   private fun File.retainedHeap(
     threadName: String,
-    computeDominators: Boolean = false
-  ): Pair<Bytes, String> {
+  ): Bytes {
     val heapAnalyzer = HeapAnalyzer(OnAnalysisProgressListener.NO_OP)
 
-    val (analysis, dominatorTree) = openHeapGraph().use { graph ->
+    val analysis = openHeapGraph().use { graph ->
       val analysis = heapAnalyzer.analyze(
         heapDumpFile = this,
         graph = graph,
@@ -201,20 +195,10 @@ class HprofRetainedHeapPerfTest {
       check(analysis is HeapAnalysisSuccess) {
         "Expected success not $analysis"
       }
-
-      val dominatorTree = if (computeDominators) {
-        val weakAndFinalizerRefs = EnumSet.of(REFERENCES, FINALIZER_WATCHDOG_DAEMON)
-        val ignoredRefs = ReferenceMatcher.fromListBuilders(weakAndFinalizerRefs).map { matcher ->
-          matcher as IgnoredReferenceMatcher
-        }
-        ObjectDominators().renderDominatorTree(
-          graph, ignoredRefs, 200, threadName, true
-        )
-      } else ""
-      analysis to dominatorTree
+      analysis
     }
 
-    return analysis.applicationLeaks.single().leakTraces.single().retainedHeapByteSize!!.bytes to dominatorTree
+    return analysis.applicationLeaks.single().leakTraces.single().retainedHeapByteSize!!.bytes
   }
 
   class BytesAssert(
@@ -239,8 +223,6 @@ class HprofRetainedHeapPerfTest {
 
   private fun assertThat(bytes: Bytes) = BytesAssert(bytes, "")
 
-  private fun assertThat(pair: Pair<Bytes, String>) = BytesAssert(pair.first, pair.second)
-
   data class Bytes(val count: Int)
 
   operator fun Bytes.minus(other: Bytes) = Bytes(count - other.count)

shark/shark-android/src/test/java/shark/LegacyHprofTest.kt

@@ -48,7 +48,7 @@ class LegacyHprofTest {
     val analysis = analyzeHprof("gcroot_unknown_object.hprof")
 
     assertThat(analysis.applicationLeaks).hasSize(2)
-    assertThat(analysis.allLeaks.sumBy { it.totalRetainedHeapByteSize!! }).isEqualTo(5306218)
+    assertThat(analysis.allLeaks.sumBy { it.totalRetainedHeapByteSize!! }).isEqualTo(5018520)
   }
 
   @Test fun androidMStripped() {

shark/shark-graph/src/main/java/shark/internal/HprofInMemoryIndex.kt

@@ -210,7 +210,7 @@ internal class HprofInMemoryIndex private constructor(
   }
 
   fun objectAtIndex(index: Int): LongObjectPair<IndexedObject> {
-    require(index > 0)
+    require(index >= 0)
     if (index < classIndex.size) {
       val objectId = classIndex.keyAt(index)
       val array = classIndex.getAtIndex(index)
@@ -237,7 +237,7 @@ internal class HprofInMemoryIndex private constructor(
       )
     }
     shiftedIndex -= objectArrayIndex.size
-    require(index < primitiveArrayIndex.size)
+    require(shiftedIndex < primitiveArrayIndex.size)
     val objectId = primitiveArrayIndex.keyAt(shiftedIndex)
     val array = primitiveArrayIndex.getAtIndex(shiftedIndex)
     return objectId to IndexedPrimitiveArray(
@@ -276,7 +276,7 @@ internal class HprofInMemoryIndex private constructor(
     index = primitiveArrayIndex.indexOf(objectId)
     if (index >= 0) {
       val array = primitiveArrayIndex.getAtIndex(index)
-      return classIndex.size + instanceIndex.size + index + primitiveArrayIndex.size to IndexedPrimitiveArray(
+      return classIndex.size + instanceIndex.size + objectArrayIndex.size + index to IndexedPrimitiveArray(
         position = array.readTruncatedLong(positionSize),
         primitiveType = PrimitiveType.values()[array.readByte()
           .toInt()],