Count content shared by several objects once in the dominator tree

docs/changelog.md

@@ -19,6 +19,8 @@ Releases before 2.8.1 predate these markers.
 
 ## Unreleased
 
+* 🐛 [#2700](https://github.com/square/leakcanary/issues/2700) The retained size of a leak could be reported as larger than the whole heap, and wrapped past `Int.MAX_VALUE` into a negative *"Total retained"* in the analysis result. The traversal skipped the reference from a `java.lang.String` to the array holding its characters and the references from a `java.lang.Integer[]` and its equivalents for the other primitive types to the boxed primitives they hold, because a leak trace never needs to name them, and the shallow size of the object holding that content was inflated to make up for it. That's exact while each piece of content has a single holder, and content is shared more often than it looks: `new String(String)` copies the reference to the array of characters rather than the array, `Integer.valueOf()` caches -128 to 127, `Boolean.valueOf()` returns one of two instances, and before Android Marshmallow `String.substring()` shared its parent's array. Shared content was then credited to every object holding it: on the pre Marshmallow heap dump in our test resources, 525 arrays of characters held by up to 577 strings each add up to 9.5 MB of double counting on a 6.8 MB heap. The traversal now follows those references, so each piece of content is counted once, against the object that dominates it.
+    * 🔀 Indexing now picks up the id of the array holding the characters of every string as it reads the heap dump, so that following that reference costs no read. Strings are 44% to 70% of the instances in the Android heap dumps in our test resources, and reading their reference from the heap dump instead would take the analysis from 11786, 17407 and 19711 random access reads to 22592, 64520 and 32494. What following content does cost is one record read per wrapper array reached, which those heap dumps hold few of: 11791, 17412 and 19730 reads. The index holds two ids per string, which is 92 KB, 407 KB and 341 KB on those heap dumps, 7% to 9% more memory than the index used to take.
 * 💥 [#2789](https://github.com/square/leakcanary/issues/2789) [#2773](https://github.com/square/leakcanary/issues/2773) The heap analysis of a large heap failed with an `OutOfMemoryError` while growing the set of objects the path finding traversal has already visited. That set was keyed by object id, so it held every reachable object in an 8 byte per slot hash table at a 0.75 load factor, sized from a guess (`instanceCount / 2`) that is always too small: on the Android heap dumps in our test resources the traversal ends up visiting 0.65x to 1.08x of `instanceCount`. Growing doubles the table and rehashes into it while the old one is still referenced, so the moment of growth needs 1.5x the new table — on a heap of 4 million objects, a 33.6 MB table and a 67.1 MB one live at the same time, in an app capped at a 512 MB growth limit. The visited set is now one bit per object in the heap dump, keyed by the object's index rather than by its id, allocated once at a size that's known upfront, so it can't grow and can't rehash: 509 KB rather than a 100.7 MB peak on that 4 million object heap. On a 4.4 million object Android heap dump, the smallest heap the analysis completes in goes from 513 MB to 385 MB.
     * 🔀 Mapping an object id to its index is a binary search where a hash lookup used to do, and the traversal does that once per reference it reads, so the analysis is about 2% slower end to end (4% to 8% of the path finding step) on the Android heap dumps in our test resources.
 * 💥 [#2773](https://github.com/square/leakcanary/issues/2773) The path finding traversal also kept a set of the object ids waiting in its queue, keyed by object id and therefore growing to the size of the traversal frontier — on a heap dump whose objects are reachable through a wide array, most of the heap. It turned out to be write-only: an object is added to exactly one of the two queues and removed from it when polled, so an object waiting in the low priority queue is never in the high priority queue as well, which is all the one read of that set was checking. Removing it takes the smallest heap the analysis of a 4.4 million object Android heap dump completes in from 385 MB to 321 MB, and makes path finding 2.6% to 4.3% faster, since maintaining the set cost a hash insert per reference enqueued and a hash removal per object visited.

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

@@ -0,0 +1,55 @@
+package shark
+
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.Test
+import shark.HprofHeapGraph.Companion.openHeapGraph
+
+/**
+ * Indexing picks up the id of the array holding the characters of every string as it reads a heap
+ * dump, which relies on it working out where java.lang.String keeps that array before reaching the
+ * instances. Android heap dumps don't cooperate: most string records come before the class dump of
+ * java.lang.String, so the first of the two indexing passes is what finds the layout. These check
+ * that against real Android heap dumps, where being off by a field would go unnoticed with a
+ * synthetic one.
+ */
+class AndroidStringValueReferenceTest {
+
+  @Test fun `strings point at the array their record holds on Android O`() {
+    assertStringsPointAtTheArrayTheirRecordHolds("leak_asynctask_o.hprof")
+  }
+
+  @Test fun `strings point at the array their record holds on Android M`() {
+    assertStringsPointAtTheArrayTheirRecordHolds("leak_asynctask_m.hprof")
+  }
+
+  @Test fun `strings point at the array their record holds pre Android M`() {
+    assertStringsPointAtTheArrayTheirRecordHolds("leak_asynctask_pre_m.hprof")
+  }
+
+  private fun assertStringsPointAtTheArrayTheirRecordHolds(hprofFileName: String) {
+    hprofFileName.classpathFile().openHeapGraph().use { graph ->
+      // No reference matcher, so that reading a string goes through the index rather than falling
+      // back to reading its record.
+      val referenceReader = FieldInstanceReferenceReader(graph, emptyList())
+      val strings = graph.findClassByName("java.lang.String")!!.instances.toList()
+      var withValueArray = 0
+
+      strings.forEach { string ->
+        val fromRecord = string["java.lang.String", "value"]?.value?.asNonNullObjectId
+        val references = referenceReader.read(string).toList()
+        assertThat(references.map { it.valueObjectId })
+          .describedAs("references of string ${string.objectId}")
+          .isEqualTo(listOfNotNull(fromRecord))
+        references.forEach { reference ->
+          assertThat(reference.lazyDetailsResolver.resolve().name).isEqualTo("value")
+        }
+        if (fromRecord != null) {
+          withValueArray++
+        }
+      }
+
+      // Fail loudly rather than pass on an empty check if strings stop holding an array.
+      assertThat(withValueArray).isGreaterThan(strings.size / 2)
+    }
+  }
+}

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

@@ -172,10 +172,10 @@ class HprofIOPerfTest {
     val metrics = trackAnalyzeRandomAccessMetrics(hprofFile)
 
     assertThat(metrics.withoutRetainedSize.toString()).isEqualTo(
-      "reads=19711 medianBytes=40.0 totalBytes=1021265 distinctPages=447 pageReads=19947"
+      "reads=19730 medianBytes=40.0 totalBytes=1026245 distinctPages=448 pageReads=19967"
     )
     assertThat(metrics.withRetainedSize.toString()).isEqualTo(
-      "reads=20979 medianBytes=40.0 totalBytes=1078529 distinctPages=455 pageReads=21229"
+      "reads=20993 medianBytes=40.0 totalBytes=1083329 distinctPages=456 pageReads=21244"
     )
   }
 
@@ -185,10 +185,10 @@ class HprofIOPerfTest {
     val metrics = trackAnalyzeRandomAccessMetrics(hprofFile)
 
     assertThat(metrics.withoutRetainedSize.toString()).isEqualTo(
-      "reads=17407 medianBytes=40.0 totalBytes=1953885 distinctPages=696 pageReads=17885"
+      "reads=17412 medianBytes=40.0 totalBytes=1958573 distinctPages=699 pageReads=17890"
     )
     assertThat(metrics.withRetainedSize.toString()).isEqualTo(
-      "reads=17412 medianBytes=40.0 totalBytes=1954065 distinctPages=696 pageReads=17890"
+      "reads=17412 medianBytes=40.0 totalBytes=1958573 distinctPages=699 pageReads=17890"
     )
   }
 
@@ -198,10 +198,10 @@ class HprofIOPerfTest {
     val metrics = trackAnalyzeRandomAccessMetrics(hprofFile)
 
     assertThat(metrics.withoutRetainedSize.toString()).isEqualTo(
-      "reads=11786 medianBytes=32.0 totalBytes=554362 distinctPages=511 pageReads=11922"
+      "reads=11791 medianBytes=32.0 totalBytes=559050 distinctPages=512 pageReads=11927"
     )
     assertThat(metrics.withRetainedSize.toString()).isEqualTo(
-      "reads=11788 medianBytes=32.0 totalBytes=554426 distinctPages=511 pageReads=11924"
+      "reads=11791 medianBytes=32.0 totalBytes=559050 distinctPages=512 pageReads=11927"
     )
   }
 

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

@@ -52,7 +52,7 @@ class HprofRetainedHeapPerfTest {
 
     val retained = analysisRetained - baselineHeap.retainedHeap(ANALYSIS_THREAD).first
 
-    assertThat(retained).isEqualTo(4.5 MB +-5 % margin)
+    assertThat(retained).isEqualTo(4.8 MB +-5 % margin)
   }
 
   @Test fun `freeze retained memory when indexing leak_asynctask_m`() {
@@ -69,7 +69,7 @@ class HprofRetainedHeapPerfTest {
 
     val retained = analysisRetained - baselineHeap.retainedHeap(ANALYSIS_THREAD).first
 
-    assertThat(retained).isEqualTo(4.4 MB +-5 % margin)
+    assertThat(retained).isEqualTo(4.8 MB +-5 % margin)
   }
 
   @Test fun `freeze retained memory through analysis steps of leak_asynctask_o`() {
@@ -109,13 +109,13 @@ class HprofRetainedHeapPerfTest {
       retainedPair.first - retainedBeforeAnalysis to retainedPair.second
     }
 
-    assertThat(retained after PARSING_HEAP_DUMP).isEqualTo(4.98 MB +-5 % margin)
-    assertThat(retained after EXTRACTING_METADATA).isEqualTo(5.20 MB +-5 % margin)
-    assertThat(retained after FINDING_RETAINED_OBJECTS).isEqualTo(5.28 MB +-5 % margin)
-    assertThat(retained after FINDING_PATHS_TO_RETAINED_OBJECTS).isEqualTo(5.47 MB +-5 % margin)
-    assertThat(retained after INSPECTING_OBJECTS).isEqualTo(5.47 MB +-5 % margin)
-    assertThat(retained after COMPUTING_NATIVE_RETAINED_SIZE).isEqualTo(5.47 MB +-5 % margin)
-    assertThat(retained after COMPUTING_RETAINED_SIZE).isEqualTo(5.47 MB +-5 % margin)
+    assertThat(retained after PARSING_HEAP_DUMP).isEqualTo(5.32 MB +-5 % margin)
+    assertThat(retained after EXTRACTING_METADATA).isEqualTo(5.55 MB +-5 % margin)
+    assertThat(retained after FINDING_RETAINED_OBJECTS).isEqualTo(5.62 MB +-5 % margin)
+    assertThat(retained after FINDING_PATHS_TO_RETAINED_OBJECTS).isEqualTo(5.82 MB +-5 % margin)
+    assertThat(retained after INSPECTING_OBJECTS).isEqualTo(5.82 MB +-5 % margin)
+    assertThat(retained after COMPUTING_NATIVE_RETAINED_SIZE).isEqualTo(5.82 MB +-5 % margin)
+    assertThat(retained after COMPUTING_RETAINED_SIZE).isEqualTo(5.82 MB +-5 % margin)
   }
 
   private fun indexRecordsOf(hprofFile: File): HprofIndex {

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

@@ -48,7 +48,10 @@ class LegacyHprofTest {
     val analysis = analyzeHprof("gcroot_unknown_object.hprof")
 
     assertThat(analysis.applicationLeaks).hasSize(2)
-    assertThat(analysis.allLeaks.sumBy { it.totalRetainedHeapByteSize!! }).isEqualTo(5018520)
+    // 48 bytes lower than it used to be: this heap dump holds boxed primitives that a wrapper array
+    // the leaks retain isn't the only object referencing, and the size of those is no longer added
+    // to the wrapper array holding them.
+    assertThat(analysis.allLeaks.sumBy { it.totalRetainedHeapByteSize!! }).isEqualTo(5018472)
   }
 
   @Test fun androidMStripped() {

shark/shark-graph/api/shark-graph.api

@@ -103,6 +103,7 @@ public final class shark/HeapObject$HeapInstance : shark/HeapObject {
 	public final fun get (Lkotlin/reflect/KClass;Ljava/lang/String;)Lshark/HeapField;
 	public final fun getByteSize ()J
 	public fun getGraph ()Lshark/HeapGraph;
+	public final fun getIndexedStringValueObjectId ()J
 	public final fun getInstanceClass ()Lshark/HeapObject$HeapClass;
 	public final fun getInstanceClassId ()J
 	public final fun getInstanceClassName ()Ljava/lang/String;

shark/shark-graph/src/main/java/shark/HeapObject.kt

@@ -366,6 +366,17 @@ sealed class HeapObject {
     val instanceClassId: Long
       get() = indexedObject.classId
 
+    /**
+     * If this is a `java.lang.String`, the heap identifier of the array holding its characters,
+     * which indexing picked up as it read the heap dump, so reading this triggers no IO.
+     *
+     * [ValueHolder.NULL_REFERENCE] for every other instance, and for a string too when the heap
+     * dump didn't let indexing work out where `java.lang.String` keeps that array. Callers that
+     * need the array in every case have to fall back to [readRecord] and read the field.
+     */
+    val indexedStringValueObjectId: Long
+      get() = hprofGraph.stringValueObjectId(objectId)
+
     /**
      * Reads and returns the underlying [InstanceDumpRecord].
      *

shark/shark-graph/src/main/java/shark/HprofHeapGraph.kt

@@ -261,6 +261,10 @@ class HprofHeapGraph internal constructor(
     return index.classFieldsReader.classDumpHasReferenceFields(indexedClass)
   }
 
+  internal fun stringValueObjectId(objectId: Long): Long {
+    return index.stringValueObjectId(objectId)
+  }
+
   internal fun fieldName(
     classId: Long,
     fieldRecord: FieldRecord