Merge pull request #2324 from square/py/dedup_stickyclass

Deduplicate StickyClass GC roots
square · Apr 3, 2022 · 775de4d · 775de4d
2 parents 7617c39 + 2e22b9e
commit 775de4d
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 2 deletions.
diff --git a/shark-android/src/test/java/shark/HprofRetainedHeapPerfTest.kt b/shark-android/src/test/java/shark/HprofRetainedHeapPerfTest.kt
@@ -72,7 +72,7 @@ class HprofRetainedHeapPerfTest {
 
     val retained = analysisRetained - baselineHeap.retainedHeap(ANALYSIS_THREAD).first
 
-    assertThat(retained).isEqualTo(8.2 MB +-5 % margin)
+    assertThat(retained).isEqualTo(4.9 MB +-5 % margin)
   }
 
   @Test fun `freeze retained memory through analysis steps of leak_asynctask_o`() {

diff --git a/shark-graph/src/main/java/shark/internal/HprofInMemoryIndex.kt b/shark-graph/src/main/java/shark/internal/HprofInMemoryIndex.kt
@@ -44,6 +44,7 @@ import shark.internal.hppc.LongObjectScatterMap
 import shark.internal.hppc.to
 import java.util.EnumSet
 import kotlin.math.max
+import shark.internal.hppc.LongScatterSet
 
 /**
  * This class is not thread safe, should be used from a single thread.
@@ -361,6 +362,8 @@ internal class HprofInMemoryIndex private constructor(
 
     private val gcRoots = mutableListOf<GcRoot>()
 
+    private val stickyClassGcRootIds = LongScatterSet()
+
     private fun HprofRecordReader.copyToClassFields(byteCount: Int) {
       for (i in 1..byteCount) {
         classFieldBytes[classFieldsIndex++] = readByte()
@@ -427,7 +430,12 @@ internal class HprofInMemoryIndex private constructor(
         }
         ROOT_STICKY_CLASS -> {
           reader.readStickyClassGcRootRecord().apply {
-            if (id != ValueHolder.NULL_REFERENCE) {
+            // StickyClass has only 1 field: id. Our API 23 emulators in CI are creating heap
+            // dumps with duplicated sticky class roots, up to 30K times for some objects.
+            // There's no point in keeping all these in our list of roots, 1 per each is enough
+            // so we deduplicate with stickyClassGcRootIds.
+            if (id != ValueHolder.NULL_REFERENCE && id !in stickyClassGcRootIds) {
+              stickyClassGcRootIds += id
               gcRoots += this
             }
           }

diff --git a/shark-graph/src/test/java/shark/HprofIndexParsingTest.kt b/shark-graph/src/test/java/shark/HprofIndexParsingTest.kt
@@ -0,0 +1,43 @@
+package shark
+
+import org.assertj.core.api.Assertions
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.Test
+import shark.GcRoot.StickyClass
+import shark.HprofHeapGraph.Companion.openHeapGraph
+import shark.HprofRecord.HeapDumpRecord.GcRootRecord
+import shark.HprofRecord.HeapDumpRecord.ObjectRecord.ClassDumpRecord
+import shark.HprofRecord.LoadClassRecord
+import shark.HprofRecord.StringRecord
+
+class HprofIndexParsingTest {
+
+  private var lastId = 0L
+  private val id: Long
+    get() = ++lastId
+
+  @Test fun `duplicated StickyClass GC roots are deduplicated`() {
+    val className = StringRecord(id, "com.example.VeryStickyClass")
+    val loadClassRecord = LoadClassRecord(1, id, 1, className.id)
+    val classDump = ClassDumpRecord(
+      id = loadClassRecord.id,
+      stackTraceSerialNumber = 1,
+      superclassId = 0,
+      classLoaderId = 0,
+      signersId = 0,
+      protectionDomainId = 0,
+      instanceSize = 0,
+      staticFields = emptyList(),
+      fields = emptyList()
+    )
+    val stickyClassRecords = (1..5).map { GcRootRecord(StickyClass(loadClassRecord.id)) }
+    val bytes = (listOf(className, loadClassRecord, classDump) + stickyClassRecords).asHprofBytes()
+
+    val stickyClassRoots = bytes.openHeapGraph().use { graph: HeapGraph ->
+      graph.gcRoots.filterIsInstance(StickyClass::class.java)
+    }
+
+    assertThat(stickyClassRoots).hasSize(1)
+    assertThat(stickyClassRoots.first().id).isEqualTo(loadClassRecord.id)
+  }
+}