Merge pull request #2964 from square/index-entry-shrink

Shrink the per-object entries of the heap dump index

Author: pyricau

docs/changelog.md

@@ -57,6 +57,9 @@ Releases before 2.8.1 predate these markers.
 * 💥 A minified app depending on `leakcanary-android-process` died on launch with *"Failed to create an instance of androidx.work.impl.WorkDatabase"*, caused by a `NoSuchMethodException` on that class's no argument constructor, and once that was worked around, never ran the analysis, failing instead with *"java.lang.Class<androidx.work.OverwritingInputMerger> has no zero argument constructor"*. Both classes are instantiated reflectively through their no argument constructor, and the rules Room and WorkManager ship to protect them — `-keep class * extends androidx.room.RoomDatabase` and `-keep class * extends androidx.work.InputMerger` — kept that constructor under the old R8 and stopped doing so in R8 full mode, the default since AGP 8. WorkManager fixed its rule in 2.10.0; Room hasn't as of 2.6.1, and `leakcanary-android-process` depends on the oldest WorkManager LeakCanary supports on purpose, so that apps resolve to their own newer version. `leakcanary-android-process` is also what puts WorkManager on the classpath of an app that doesn't otherwise use it, so it now ships the keep rules those two classes need itself, naming just those two so that an app's own Room databases and input mergers are left to whatever the app already does about them. Setting up multi process analysis is once again adding one dependency, with nothing to add to your ProGuard configuration.
 * 🐛 [Running the LeakCanary analysis in a separate process](recipes.md#running-the-leakcanary-analysis-in-a-separate-process) told you to *replace* the `leakcanary-android` dependency with `leakcanary-android-process`. That stopped being true in 2.8, when `leakcanary-android-process` became an add-on that no longer brings the analyzer in itself, so anyone who followed it since had no leak detection at all — silently, since there's nothing left to report a leak. The recipe now says to add `leakcanary-android-process` and keep `leakcanary-android`, which is what the 2.8 release notes said all along. If you set up multi process analysis from that recipe, check your dependencies.
 * 🐛 [#2857](https://github.com/square/leakcanary/pull/2857) Retained fragments were reported as not leaking with androidx.fragment 1.1.0 and higher.
+* 🔨 The heap dump index no longer spends a class id on every instance and every object array. Each of those objects belongs to a class that has a class dump record, so an entry now holds the index of its class among those records — 2 bytes for the few thousand classes of a typical heap dump, against the 4 bytes an Android heap dump identifier takes or the 8 of a JVM one — and resolves it back through the class index, which is sorted by class id and therefore in that same order. On the Android heap dumps in our test resources the four object indexes go from 12.0-14.1 bytes per object to 10.9-12.6, i.e. 8.4% to 12.5% smaller, and on a JVM heap dump from 19.2 to 14.8, i.e. 23% smaller. What an analysis of `leak_asynctask_o.hprof` retains goes from 4.48 MB to 4.33 MB, and heap growth detection computing leak shares over two heap dumps from 985 KB to 865 KB. Indexing now binary searches a class id where it used to write it out, and reading an object resolves the index back to one, neither of which is measurable end to end: over five alternating runs the median analysis of `leak_asynctask_o.hprof` is 159 ms before and 161 ms after, and of `leak_asynctask_m.hprof` 168 ms before and 165 ms after.
+    * 🔀 A heap dump holding an instance or object array whose class has no class dump record now fails to open, naming that class id. Nothing could be read from such an object anyway, since its fields are laid out by a class the heap dump doesn't contain, and none of the Android and JVM heap dumps in our test resources hold one.
+* 🔨 The heap dump index no longer spends a whole object id on the key of every entry. Ids are stored as offsets from the smallest id in the heap dump, over as many bytes as the distance up to the largest one needs, so a key costs what the *span* of a heap dump's ids needs rather than what an id needs. This is worth the most on JVM heap dumps: their ids are 8 bytes but a heap spans well under the 4 GB an unsigned int covers, so keys drop to 4 bytes and the four object indexes go from 14.8 bytes per object to 10.8, i.e. 27% smaller — on a heap dump of hundreds of millions of objects, gigabytes of index. Android heap dumps mostly don't move, since their ids are already 4 bytes and ART spreads its heaps over most of the range an int addresses: eight of the nine in our test resources are unchanged, and the ninth, of a heap small enough to fit its ids in 3 bytes, is 8.5% smaller. What heap growth detection retains while computing leak shares over two heap dumps goes from 865 KB to 745 KB, the synthetic dumps there being small enough for 2 byte keys. Entries are sorted and binary searched on the stored offsets, which are in the same order as the ids they encode, so a lookup decodes nothing: over five alternating runs the median analysis of `leak_asynctask_o.hprof` is 159 ms before and 161 ms after, and of `leak_asynctask_m.hprof` 166 ms before and 167 ms after.
 * 💥 Opening the overflow menu in the LeakCanary activity crashed apps that wrap `Window.Callback`, because the framework decor action bar calls `Window.Callback.onMenuOpened()` with a `null` menu ([b/188568911](https://issuetracker.google.com/issues/188568911)) even though that parameter is annotated as non null, which Kotlin wrappers rightfully null check. The LeakCanary activity now hosts its own `Toolbar` instead of using the decor action bar, so `onMenuOpened()` is never called.
 * 🐛 [#1789](https://github.com/square/leakcanary/issues/1789) When running the analysis in a separate process (`leakcanary-android-process`), the `HeapAnalysisDone` event was dispatched in the `:leakcanary` process, so it never reached the `LeakCanary.Config.eventListeners` configured in the main process. The `:leakcanary` process now stores the analysis in LeakCanary's database and a chained WorkManager worker dispatches the event from the main process, which also means the event still gets dispatched if the main process dies while the analysis is running.
     * 🔀 `HeapAnalysisDone` is now dispatched in the main process instead of the `:leakcanary` process. If you were relying on an `EventListener` configured in the `:leakcanary` process receiving it, configure that listener in the main process instead. `HeapAnalysisProgress` is still dispatched from the `:leakcanary` process. See [Running the LeakCanary analysis in a separate process](recipes.md#running-the-leakcanary-analysis-in-a-separate-process).

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.33 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.24 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(4.84 MB +-5 % margin)
+    assertThat(retained after EXTRACTING_METADATA).isEqualTo(5.07 MB +-5 % margin)
+    assertThat(retained after FINDING_RETAINED_OBJECTS).isEqualTo(5.14 MB +-5 % margin)
+    assertThat(retained after FINDING_PATHS_TO_RETAINED_OBJECTS).isEqualTo(5.34 MB +-5 % margin)
+    assertThat(retained after INSPECTING_OBJECTS).isEqualTo(5.34 MB +-5 % margin)
+    assertThat(retained after COMPUTING_NATIVE_RETAINED_SIZE).isEqualTo(5.34 MB +-5 % margin)
+    assertThat(retained after COMPUTING_RETAINED_SIZE).isEqualTo(5.34 MB +-5 % margin)
   }
 
   @Test fun `freeze retained memory when computing leak shares`() {
@@ -156,7 +156,7 @@ class HprofRetainedHeapPerfTest {
     val retained = heapWhenComputingLeakShares.retainedHeap(ANALYSIS_THREAD).first -
       baselineHeap.retainedHeap(ANALYSIS_THREAD).first
 
-    assertThat(retained).isEqualTo(0.98 MB +-5 % margin)
+    assertThat(retained).isEqualTo(0.74 MB +-5 % margin)
   }
 
   private fun indexRecordsOf(hprofFile: File): HprofIndex {

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

@@ -46,18 +46,7 @@ internal class ByteSubArray(
     require(index >= 0 && index <= endInclusive - (byteCount - 1)) {
       "Index $index should be between 0 and ${endInclusive - (byteCount - 1)}"
     }
-    var pos = rangeStart + index
-    val array = array
-
-    var value = 0L
-
-    var shift = (byteCount - 1) * 8
-    while (shift >= 8) {
-      value = value or (array[pos++] and 0xffL shl shift)
-      shift -= 8
-    }
-    value = value or (array[pos] and 0xffL)
-    return value
+    return array.readTruncatedLong(rangeStart + index, byteCount)
   }
 
   fun readLong(): Long {
@@ -70,6 +59,40 @@ internal class ByteSubArray(
   }
 }
 
+/**
+ * Number of bytes an unsigned big endian integer needs to hold any value from 0 to [maxValue]
+ * inclusive, which is what the fields packed into an index entry are sized with.
+ */
+internal fun byteSizeForUnsigned(maxValue: Long): Int {
+  var value = maxValue
+  var byteCount = 0
+  while (value != 0L) {
+    value = value shr 8
+    byteCount++
+  }
+  return byteCount
+}
+
+/**
+ * Reads the [byteCount] bytes at [index] as an unsigned big endian integer, the counterpart of
+ * [ByteSubArrayWriter.writeTruncatedLong]. A [byteCount] of 8 reads the bytes back as the signed
+ * long they were written from.
+ */
+internal fun ByteArray.readTruncatedLong(
+  index: Int,
+  byteCount: Int
+): Long {
+  var pos = index
+  val array = this
+  var value = 0L
+  var shift = (byteCount - 1) * 8
+  while (shift >= 8) {
+    value = value or (array[pos++] and 0xffL shl shift)
+    shift -= 8
+  }
+  return value or (array[pos] and 0xffL)
+}
+
 internal fun ByteArray.readShort(index: Int): Short {
   var pos = index
   val array = this