Attribute heap growth to the code that caused it - #2882
Open
pyricau wants to merge 4 commits into
Open
Conversation
ShortestPathObjectNode.name is a display string: it folds the reference type, the simple name of the owning class, the field name and the class of the referenced object into one line, and that string is also the key the traversal groups objects by. It reads well, but it's the only thing a growing node says about the reference that reached it, so anything that wants to act on that reference rather than print it has to parse the string back apart, and can't recover the package of the owning class at all. The traversal already resolves Reference.LazyDetails to build that string, so keep the structured form around instead of dropping it: a growing node now exposes the reference location type, the fully qualified owning class name and the field name. This is the first step towards attributing heap growth to the code that caused it, as described in section 4.2 of the BLeak paper: watching the field that grows requires naming it as data. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Heap growth detection says what is growing and where it hangs off the heap, but not which code grew it. Section 4.2 of the BLeak paper closes that gap by recording a stack trace for every write that grows a growing object, which needs the growing object watched while the scenario runs one more time. BLeak watches growth by rewriting the growing container's mutating methods. ART can't retransform loaded classes, so java.util.ArrayList.add can't be instrumented that way. ObjectGrowthAttributor instead swaps the growing collection out for a java.lang.reflect.Proxy that records a stack trace on each call that can add to it and delegates to the collection it replaced, then puts the original back once the scenario is done. Only growing objects held by a writable field of interface type can be watched, and an instance field also needs the instance that declares it, which a heap dump can't hand back. So the attributor reports NotAttributed with a reason per growing object it can't watch instead of failing: the result is never less informative than the HeapDiff it started from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ObjectGrowthAttributor writes to a final instance field and installs a java.lang.reflect.Proxy, both of which behave differently enough across runtimes to be worth checking on a device: heap growth detection is aimed at instrumentation tests, so ART is where this has to work. CI only runs connectedCheck for leakcanary-android, leakcanary-android-core and leakcanary-android-instrumentation, so this test, like the RepeatingAndroidInProcessScenarioTest it sits next to, only runs when leakcanary-android-test:connectedCheck is run by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Heap growth detection tells you what is growing and where it hangs off the heap. It doesn't tell you which code grew it. Section 4.2 of BLeak closes that gap by recording a stack trace per growth event. This does that, for the case that is reachable without a native agent.
What a caller gets
which prints, verbatim, from a real run:
Design
Two phases, because field identity is only known after diffing two heap dumps.
Phase 1, in
shark: name the reference as data.ShortestPathObjectNode.nameis a display string — reference type, simple name of the owning class, field name, class of the referenced object, all folded into one line — and it's also the key the traversal groups objects by. It was the only thing a node said about the reference that reached it, so acting on that reference meant parsing the string back apart, and the package of the owning class was gone for good. The traversal already resolvesReference.LazyDetailsto build the string, so the structured form is now kept instead of dropped:ShortestPathObjectNode.referenceis aReferenceLocationwith the location type, the fully qualified owning class name and the reference name. TheObjectGrowthDetectordiff is 27 lines and doesn't touchLeakShareCalculator, to keep the merge with #2880 and its follow-up cheap.Phase 2, in
leakcanary-core: watch the field and record stacks. BLeak rewrites the growing container's mutating methods. That is not available here — ART reportscan_generate_all_class_hook_eventsas 0, sojava.util.ArrayList.addcannot be retransformed.ObjectGrowthAttributorinstead resolves the growing object'sReferenceLocationto a livejava.lang.reflect.Field, swaps the collection it holds for ajava.lang.reflect.Proxythat records a stack trace on every call that can add to it and delegates to the collection it replaced, runs the scenario again, then puts the original back in afinally. Stacks are grouped and counted, most frequent first — a stack that fires once per scenario loop is the suspect.Degrading instead of crashing. Each growing object comes back as
GrowthAttribution.AttributedorNotAttributed(reason), so the result is never less informative than theHeapDiffit started from. Reasons cover: not reached through a field, class not loaded, field declared as a concrete type (can't hold a proxy), static final, value neitherCollectionnorMap, and no/ambiguousfieldOwners.Why
fieldOwnersexists, and it's the thing I'd most like pushback on. A static field is found from the class name alone. An instance field needs the instance that declares it, and there is no way to get from an object in a heap dump back to the live object it was dumped from — Android heap dumps don't identify live objects, and enumerating a class's instances in process needs JVMTI. So the caller names the candidates, which in a test meanslistOf(this). It's a wart. The alternative is walking a reflective path from a GC root, which doesn't work for the common case: the growing field usually hangs off the test instance, which is only reachable as a thread local.Feasibility: why there's no JVMTI agent here
I scoped the agent out, on this evidence:
CMakeLists.txt, noexternalNativeBuild, nondkVersion, no.cppanywhere. A JVMTI agent is a.so, so it means a new module, NDK config, four ABIs and NDK on CI, with no precedent to follow.Debug.attachJvmtiAgentis the only way in.can_generate_field_modification_events. But a watch is per field, not per object, so watchingArrayList.sizefires for everyArrayListin the process. Filtering to the growing instance needs a live handle for it, and the heap dump can't provide one. JVMTI would be needed forIterateOverInstancesOfClass, i.e. for instance enumeration, not for the watching. That's the real research question, and it's the same obstaclefieldOwnersworks around.Allocation tracking is the cheaper non-JVMTI route, but an allocation site is not the write that retained the object, and it's hidden API. Not pursued.
Verified
Ran with
ANDROID_HOMEset andJAVA_HOMEon 17:./gradlew build -x lint— green./gradlew detekt— green (moving the two newsharktests into their own class was needed: they pushedObjectGrowthDetectorTestto 601 lines against detekt's 600-lineLargeClasslimit)./gradlew checkKotlinAbi— green; ABI dumps regenerated withupdateKotlinAbiand read before committing. Purely additive:shark.ReferenceLocation,ShortestPathObjectNode.getReference(), and the newleakcanaryattribution types. No removals, no signature changes../gradlew :leakcanary:leakcanary-jvm-test:test— 5 new tests pass: attribution of a list, count per scenario loop, original collection restored with every added object kept, concrete field type not attributed, missingfieldOwnersnot attributed../gradlew :shark:shark:test— 3 new tests pass, built with thedump { }DSL../gradlew :leakcanary:leakcanary-android-test:connectedCheck— 3/3 pass on an API 29 emulator, including the newAndroidGrowthAttributionTest. This is what confirms ART allows the final-instance-field write and theProxyinstall; the JVM tests can't.Not verified
connectedCheckruns forleakcanary-android,leakcanary-android-coreandleakcanary-android-instrumentationonly, soleakcanary-android-test— where the pre-existingRepeatingAndroidInProcessScenarioTestalso lives — runs by hand only. Worth fixing separately; I didn't want to widen the CI matrix in this PR.Attributedwith an emptygrowthStacks.field as ArrayListinside the scenario throws. Documented on the class; code that uses the declared interface is fine.🤖 Generated with Claude Code