Skip to content

Commit

Permalink
Deduplicate StickyClass GC roots
Browse files Browse the repository at this point in the history
System classes are GC roots and tracked by GcRoot.StickyClass. We've seen heap dumps from API 23 emulators that included millions of sticky class roots, pointing several times to the same instances. As we keep the list of roots in memory, this wasted a lot of memory and slowed down the sorting of gc roots which happens right before path finding. This change fixes this by deduplicating StickyClass GC roots based on their ids.
  • Loading branch information
pyricau committed Apr 2, 2022
1 parent 79cdab7 commit 2e22b9e
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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`() {
Expand Down
10 changes: 9 additions & 1 deletion shark-graph/src/main/java/shark/internal/HprofInMemoryIndex.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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
}
}
Expand Down
43 changes: 43 additions & 0 deletions shark-graph/src/test/java/shark/HprofIndexParsingTest.kt
Original file line number Diff line number Diff line change
@@ -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)
}
}

0 comments on commit 2e22b9e

Please sign in to comment.