Skip to content

Commit 6db8cc1

Browse files
pyricauclaude
andcommitted
Remove convergence loop from DominatorTree; point to LinkEvalDominatorTree
The convergence loop was shown to be ~40x too slow for production use. Now that LinkEvalDominatorTree provides an exact solution, strip the convergence loop machinery from DominatorTree: - Remove collectCrossEdges constructor param, crossEdges field, runConvergenceLoop(), and pruneSettledCrossEdges() - Delete LongPairList (no longer used anywhere) - Delete ConvergenceLoopBenchmark and RealHeapEdgeCountTest - Remove convergence loop tests from DominatorTreeTest - Update class-level and updateDominated KDoc to document the approximation bug and point to LinkEvalDominatorTree as the fix - Regenerate API dump Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 3e0f2a0 commit 6db8cc1

File tree

8 files changed

+978
-421
lines changed

8 files changed

+978
-421
lines changed

dominator_tree.md

Lines changed: 480 additions & 0 deletions
Large diffs are not rendered by default.

dominators_plan.md

Lines changed: 485 additions & 0 deletions
Large diffs are not rendered by default.

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

Lines changed: 0 additions & 89 deletions
This file was deleted.

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

Lines changed: 0 additions & 95 deletions
This file was deleted.

shark/shark/api/shark.api

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -138,14 +138,12 @@ public final class shark/ClassReferenceReader : shark/ReferenceReader {
138138

139139
public final class shark/DominatorTree {
140140
public fun <init> ()V
141-
public fun <init> (IZ)V
142-
public synthetic fun <init> (IZILkotlin/jvm/internal/DefaultConstructorMarker;)V
141+
public fun <init> (I)V
142+
public synthetic fun <init> (IILkotlin/jvm/internal/DefaultConstructorMarker;)V
143143
public final fun buildFullDominatorTree (Lshark/DominatorTree$ObjectSizeCalculator;)Ljava/util/Map;
144144
public final fun computeRetainedSizes (Landroidx/collection/LongSet;Lshark/DominatorTree$ObjectSizeCalculator;)Landroidx/collection/LongLongMap;
145145
public final fun contains (J)Z
146146
public final fun get (J)J
147-
public final fun runConvergenceLoop (I)I
148-
public static synthetic fun runConvergenceLoop$default (Lshark/DominatorTree;IILjava/lang/Object;)I
149147
public final fun updateDominated (JJ)Z
150148
public final fun updateDominatedAsRoot (J)Z
151149
}

shark/shark/src/main/java/shark/DominatorTree.kt

Lines changed: 11 additions & 124 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import shark.ObjectDominators.DominatorNode
1010
import shark.internal.hppc.LongLongScatterMap
1111
import shark.internal.hppc.LongLongScatterMap.ForEachCallback
1212
import shark.internal.hppc.LongScatterSet
13-
import shark.internal.LongPairList
1413
import shark.internal.packedWith
1514
import shark.internal.unpackAsFirstInt
1615
import shark.internal.unpackAsSecondInt
@@ -20,32 +19,19 @@ import shark.internal.unpackAsSecondInt
2019
* Lowest Common Ancestor (LCA) approach to update immediate dominators as new parents are
2120
* discovered.
2221
*
23-
* **Known limitation**: this is an approximation, not an exact dominator algorithm (compare
24-
* with Lengauer-Tarjan or Cooper et al.'s iterative algorithm). It can produce incorrect
25-
* immediate dominators when a cross-edge (to an already-visited node) is processed using a
26-
* stale parent dominator, and that parent's dominator is later raised by another cross-edge
27-
* after the first cross-edge's children have already been settled. The child's dominator is then
28-
* left too specific (too far from root), so its retained size is under-attributed — some
29-
* objects that it truly retains exclusively are instead attributed to a higher ancestor. See
30-
* [updateDominated] for a concrete example and [DominatorTreeTest] for a test documenting this
31-
* behavior.
22+
* **Known limitation**: this is an approximation, not an exact dominator algorithm. It can
23+
* produce incorrect immediate dominators when a cross-edge (to an already-visited node) is
24+
* processed using a stale parent dominator, and that parent's dominator is later raised by
25+
* another cross-edge after the first cross-edge's children have already been settled. The child's
26+
* dominator is then left too specific (too far from root), so its retained size is
27+
* under-attributed — some objects that it truly retains exclusively are instead attributed to a
28+
* higher ancestor. See [updateDominated] for a concrete example and [DominatorTreeTest] for a
29+
* test documenting this behavior.
3230
*
33-
* **Fix**: pass `collectCrossEdges = true` to the constructor, then call [runConvergenceLoop]
34-
* after the BFS traversal completes and before calling [computeRetainedSizes] or
35-
* [buildFullDominatorTree]. The convergence loop re-processes stored cross-edges with updated
36-
* dominator values until the tree stabilizes.
37-
*
38-
* **Performance warning**: [runConvergenceLoop] is an O(cross-edges × depth × iterations)
39-
* algorithm. On real Android heap dumps the iteration count scales with the longest chain of
40-
* stale-dominator propagation, which can reach hundreds of iterations. Benchmarks on a 25 MB
41-
* heap dump show ~107 K cross-edges requiring ~780 iterations, adding ~62 seconds on top of a
42-
* 1.5-second analysis — roughly 40× slower. The loop is therefore **not suitable for production
43-
* use** in its current form and is provided only as an opt-in diagnostic / correctness-testing
44-
* tool.
31+
* For an exact dominator tree, use [LinkEvalDominatorTree] instead.
4532
*/
4633
class DominatorTree(
47-
expectedElements: Int = 4,
48-
collectCrossEdges: Boolean = false
34+
expectedElements: Int = 4
4935
) {
5036

5137
fun interface ObjectSizeCalculator {
@@ -60,12 +46,6 @@ class DominatorTree(
6046
*/
6147
private val dominated = LongLongScatterMap(expectedElements)
6248

63-
/**
64-
* Stores cross-edges discovered during BFS, for use by [runConvergenceLoop].
65-
* Only populated when `collectCrossEdges = true`.
66-
*/
67-
private val crossEdges: LongPairList? = if (collectCrossEdges) LongPairList() else null
68-
6949
operator fun contains(objectId: Long): Boolean = dominated.containsKey(objectId)
7050

7151
/**
@@ -110,8 +90,7 @@ class DominatorTree(
11090
* The consequence is that dom(C) is left too specific (too far from root), so C's retained
11191
* size is incorrectly attributed to A instead of root. A's retained size is over-reported.
11292
*
113-
* To fix this, construct with `collectCrossEdges = true` and call [runConvergenceLoop] after
114-
* the full BFS traversal and before [computeRetainedSizes] or [buildFullDominatorTree].
93+
* For an exact dominator tree that avoids this issue entirely, use [LinkEvalDominatorTree].
11594
*
11695
* @return true if [objectId] already had a known dominator, false otherwise.
11796
*/
@@ -128,17 +107,6 @@ class DominatorTree(
128107
} else {
129108
val currentDominator = dominated.getSlotValue(dominatedSlot)
130109
if (currentDominator != ValueHolder.NULL_REFERENCE) {
131-
// Only record this cross-edge if both dom(objectId) and dom(parentObjectId) are
132-
// non-null. If either is already NULL_REFERENCE the convergence loop would always
133-
// produce a no-op: a NULL_REFERENCE currentDominator can't be raised further, and a
134-
// NULL_REFERENCE parent dominator means the LCA walk terminates in one step at the
135-
// same value already set by this call.
136-
if (crossEdges != null) {
137-
val parentSlot = dominated.getSlot(parentObjectId)
138-
if (parentSlot != -1 && dominated.getSlotValue(parentSlot) != ValueHolder.NULL_REFERENCE) {
139-
crossEdges.add(objectId, parentObjectId)
140-
}
141-
}
142110
// We're looking for the Lowest Common Dominator between currentDominator and
143111
// parentObjectId. We know that currentDominator likely has a shorter dominator path than
144112
// parentObjectId since we're exploring the graph with a breadth first search. So we build
@@ -179,87 +147,6 @@ class DominatorTree(
179147
return hasDominator
180148
}
181149

182-
/**
183-
* Re-processes stored cross-edges until the dominator tree stabilizes or [maxIterations] is
184-
* reached. Must be called after the BFS traversal is complete and before calling
185-
* [computeRetainedSizes] or [buildFullDominatorTree].
186-
*
187-
* This fixes cases where a cross-edge `P→C` was processed using a stale `dom(P)` value.
188-
* When `dom(P)` is later raised by another cross-edge, `dom(C)` is not automatically updated.
189-
* The convergence loop re-runs the LCA computation for each stored cross-edge until no
190-
* further changes occur, propagating all such corrections.
191-
*
192-
* Requires `collectCrossEdges = true` to have been passed to the constructor; throws
193-
* [IllegalStateException] otherwise.
194-
*
195-
* @param maxIterations maximum number of passes over the cross-edge set. Pass [Int.MAX_VALUE]
196-
* to run until fully stable. **Warning**: real heap graphs can require hundreds of iterations;
197-
* see the class-level KDoc for performance implications.
198-
* @return the number of iterations performed.
199-
*/
200-
fun runConvergenceLoop(maxIterations: Int = Int.MAX_VALUE): Int {
201-
val edges = crossEdges
202-
?: error("Cross-edge collection not enabled. Construct DominatorTree with collectCrossEdges = true.")
203-
204-
var iterations = 0
205-
var changed = true
206-
while (changed && iterations < maxIterations) {
207-
// Prune settled edges before each pass so we iterate fewer entries. This also covers
208-
// edges already settled after the BFS traversal (when the LCA inside updateDominated
209-
// set dom(objectId) to NULL_REFERENCE after the edge was recorded).
210-
pruneSettledCrossEdges(edges)
211-
changed = false
212-
iterations++
213-
edges.forEach { objectId, parentObjectId ->
214-
val dominatedSlot = dominated.getSlot(objectId)
215-
if (dominatedSlot == -1) return@forEach
216-
val currentDominator = dominated.getSlotValue(dominatedSlot)
217-
// If already attributed to the virtual root, it cannot be raised further.
218-
if (currentDominator == ValueHolder.NULL_REFERENCE) return@forEach
219-
// LCA computation: same approach as updateDominated.
220-
val currentDominators = LongScatterSet()
221-
var dominator = currentDominator
222-
while (dominator != ValueHolder.NULL_REFERENCE) {
223-
currentDominators.add(dominator)
224-
val nextSlot = dominated.getSlot(dominator)
225-
if (nextSlot == -1) {
226-
throw IllegalStateException(
227-
"Did not find dominator for $dominator when going through the dominator chain for $currentDominator: $currentDominators"
228-
)
229-
} else {
230-
dominator = dominated.getSlotValue(nextSlot)
231-
}
232-
}
233-
dominator = parentObjectId
234-
while (dominator != ValueHolder.NULL_REFERENCE) {
235-
if (dominator in currentDominators) break
236-
val nextSlot = dominated.getSlot(dominator)
237-
if (nextSlot == -1) {
238-
throw IllegalStateException(
239-
"Did not find dominator for $dominator when going through the dominator chain for $parentObjectId"
240-
)
241-
} else {
242-
dominator = dominated.getSlotValue(nextSlot)
243-
}
244-
}
245-
if (dominator != currentDominator) {
246-
dominated[objectId] = dominator
247-
changed = true
248-
}
249-
}
250-
}
251-
return iterations
252-
}
253-
254-
private fun pruneSettledCrossEdges(edges: LongPairList) {
255-
edges.forEachIndexed { index, objectId, _ ->
256-
val slot = dominated.getSlot(objectId)
257-
if (slot == -1 || dominated.getSlotValue(slot) == ValueHolder.NULL_REFERENCE) {
258-
edges.clearAt(index)
259-
}
260-
}
261-
}
262-
263150
private class MutableDominatorNode {
264151
var shallowSize = 0
265152
var retainedSize = 0

0 commit comments

Comments
 (0)