Gather the explorer's dominator rules into one rule set - #2923
Open
pyricau wants to merge 2 commits into
Open
Conversation
Every curated list the explorer applies to a heap dump — which references hold their target without retaining it, which reference is the one way an object is held, which ones aren't worth following at all — is now in ExplorerRules, read by both halves of the edge set. The java.lang.ref classes used to be declared twice: their strengths in ReferenceStrengthReader, and the ignoring of the same fields by importing JdkReferenceMatchers.REFERENCES. That wasn't a design, it was the absence of a place to put the strength — an IgnoredReferenceMatcher carries no payload, FieldInstanceReferenceReader drops what it matches, and LazyDetails carries only a matched library leak, so shark can't be asked why a reference was dropped. Deriving the ignore list from the strengths is what the cache and thread local entries already did. Two entries fell out as redundant: KeyedWeakReference.referent, covered by the WeakReference rule because an InstanceFieldPattern matches subclasses, and zombie on the four classes that don't declare it. Measured before and after on leak_asynctask_o.hprof and on large-dump.hprof: every per-strength byte and object count identical, totals and root weight identical. What stays inherited is now spelled out rather than imported: the finalizer and cleaner queues' own list links, which are a decision about leak traces rather than a strength, and which cost the explorer a queue with one reachable entry and a tail of garbage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rule said ActivityThread$ActivityClientRecord.activity owns an activity, which is the construct named one level too low. A record is an implementation detail of how the framework runs an activity, so a tree built on it draws every screen of an app under a different unnamed record instead of side by side under the one thread that runs them all. So ActivityThreadReferenceReader gives the ActivityThread one virtual reference per running activity, read out of the ArrayMap it keeps them in, and the owner rule claims ownership through that — the same shape as a ViewGroup pointing at its children, and additive in the same way: the map, its array and each record are still nodes holding their own bytes, and it is the dominator tree that takes them out of the middle. Measured on leak_asynctask_o.hprof: the live MainActivity retains 51,634 B either way, its dominator moves from ActivityThread$ActivityClientRecord to ActivityThread, and the chain down to it goes from four steps to one. The leaked MainActivity is untouched at 211,038 B under the MainActivity$2 that leaks it, which is the fallback doing its job on a destroyed activity. The ArrayMap is read the way it reads itself, keys at the even slots and values at the odd ones over the first mSize pairs. The bound matters more here than for a ViewGroup's children: an ArrayMap leaves the slots it gives up for the next put rather than nulling them, so past the count is exactly where the record of a destroyed activity is still written down, and attributing that activity to the framework would hide the leak. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pyricau
force-pushed
the
shark-explorer-dominator-rules
branch
from
August 4, 2026 06:31
d8d70e4 to
9d2a314
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The explorer applies a handful of curated rules on top of the exact dominator tree, so that a treemap says something true about an app: a view is held by its parent, an activity by the framework, a cached image by whatever is showing it. They had grown into four different places.
Rebased onto #2930, which landed the other half of this while it was open — see the last section.
One rule set
ExplorerRulesnow holds every curated list — the fields that weaken and how, and theOwnerRules — read by both halves of the edge set throughReferenceStrengthReaderandOwnerReferences. One wiring point inHeapExplorer.open, and one file to read when asking whether a rule about a class we don't own is still true.Why the explorer keeps its own copy of the
java.lang.reffield names rather than importing shark's is worth writing down, because it isn't a preference: shark's matchers are a boolean. AnIgnoredReferenceMatchercarries no payload,FieldInstanceReferenceReaderdrops what it matches with!is IgnoredReferenceMatcher, andLazyDetailscarries only a matched library leak, so shark can't be asked why a reference was dropped and there is nowhere for "and it's weak" to live. Two ways out: add a strength-carrying matcher to shark —ReferenceMatcherissealed, so an ABI change plus a reader that emits what it currently drops, on LeakCanary's hot path — or derive the ignore list from the strengths, which is what #2930 did.One redundancy fell out:
zombieon the four reference classes that don't declare it, which only Android'sFinalizerReferencehas. That takes the matcher list from 12 to 8.A/B on two real dumps, 12 matchers against 8: every per-strength byte and object count identical, totals identical, and the same number of children under the root weighing the same, on
leak_asynctask_o.hprofand on the 39 MBlarge-dump.hprof.The ActivityThread owns the activities it runs
The activity rule named
ActivityThread$ActivityClientRecord.activity, which is the construct named one level too low: a record is an implementation detail of how the framework runs an activity, so a tree built on it draws every screen of an app under a different unnamed record instead of side by side under the one thread that runs them all.ActivityThreadReferenceReadernow gives theActivityThreadone virtualmActivitiesreference per running activity, read out of theArrayMapit keeps them in, and the owner rule claims ownership through that. Same shape as aViewGrouppointing at its children, and additive in the same way — the map, its array and each record are still nodes holding their own bytes, and it is the dominator tree that takes them out of the middle rather than any pruning.Measured on
leak_asynctask_o.hprof:MainActivityretainsActivityThread$ActivityClientRecordActivityThreadmActivities → ArrayMap, mArray → Object[], 1 → ActivityClientRecord, activity → MainActivitymActivities → MainActivityMainActivityMainActivity$2The leaked one being unchanged is the fallback doing its job:
handleDestroyActivitytakes the record out ofmActivities, so a destroyed activity has no owner left and lands under whatever is leaking it.The
ArrayMapis read the way it reads itself — keys at the even slots, values at the odd ones, over the firstmSizepairs. The bound matters more here than for aViewGroup's children: anArrayMapleaves the slots it gives up for the next put rather than nulling them, so past the count is exactly where the record of a destroyed activity is still written down, and attributing that activity to the framework would hide the leak. Pinned byan activity in a slot the map doesn't count is not one the process is running.What #2930 changed about this PR
It answered the same question from the other end, and it found a bug doing it. This PR originally also gathered the
prev/element/nextlinks ofFinalizer,FinalizerReferenceandCleanerinto the rule set as "fields followed for nothing", inherited fromJdkReferenceMatchers.REFERENCES, with a note saying that following them atFINALIZERinstead was a change worth measuring and nobody had measured it. #2930 measured it: those links are the only thing holding the finalizer and cleaner lists on Android, so ignoring them was calling 4773 oflarge-dump.hprof's 4774FinalizerReferences garbage.So that category is gone from
ExplorerRulesrather than gathered into it — a rule set is the wrong place for something that shouldn't exist — andweakeningReferenceMatchersderives from the weakening fields alone, which is what #2930 made it do. The A/B above was re-run against the post-#2930 baseline rather than carried over.Notes
notes/dominator-tree.mdgets the measurements above, a section on the rule set and on why shark's matchers can't carry a strength, and a subsection on the new reader beside theViewGroupone.GcRoot.reachabilityStrength()is deliberately left as code and out of the rule set: it maps asealedhierarchy, so making it data means mirroring that hierarchy in an enum of our own, which is more of exactly the duplication this PR is about.ExplorerRulesisinternal, andHeapExplorer.opengained norulesparameter, because nothing would pass one yet. Surfacing the rules in the UI and letting them be edited is the follow-up this makes possible, and that is when the type earns being public.Tested:
:shark:shark-explorer:shark-explorer-core:check,…-app:check,…-jdwp:check, detekt included. #2930'sJvmReferenceStrengthTestand its twoHeapReachabilityTestlist cases pass on the rebase.🤖 Generated with Claude Code