Count content shared by several objects once in the dominator tree - #2899
Open
pyricau wants to merge 1 commit into
Open
Count content shared by several objects once in the dominator tree#2899pyricau wants to merge 1 commit into
pyricau wants to merge 1 commit into
Conversation
The traversal skipped the reference from a java.lang.String to the array holding its characters and the references from a java.lang.Integer[] and its equivalents for the other primitive types to the boxed primitives they hold, because a leak trace never needs to name them, and ShallowSizeCalculator inflated the shallow size of the object holding the content to make up for it. That's exact while each piece of content has a single holder, and content is shared more often than it looks: new String(String) copies the reference to the array of characters rather than the array, Integer.valueOf() caches -128 to 127, Boolean.valueOf() returns one of two instances, and before Android Marshmallow String.substring() shared its parent's array. Shared content was then credited to every object holding it, so a dominator tree reported more retained than the heap holds, which is how issue 2700 ended up with a total retained size that wrapped past Int.MAX_VALUE. The traversal now follows those references, so each piece of content is counted once, against the object that dominates it. Content objects are leaves, so this adds nodes to the traversal without lengthening any leak trace. Following a string's reference from the heap dump would have been the expensive part: strings are 44% to 70% of the instances in the Android heap dumps in our test resources, and reading their reference takes the analysis from 11786, 17407 and 19711 random access reads to 22592, 64520 and 32494. So indexing now picks the id of the array holding a string's characters up as it reads the heap dump. It already makes two sequential passes, the first to size the index; that pass now also works out where java.lang.String keeps that array, which Android heap dumps don't hand over for free (86% of the strings in leak_asynctask_m come before the class dump of java.lang.String), and counts strings so the new index is sized exactly. The second pass then harvests the id out of bytes that were streaming past anyway. What's left of the cost is one record read per wrapper array reached, which those heap dumps hold few of: 11791, 17412 and 19730 reads. The index holds two ids per string, which is 92 KB, 407 KB and 341 KB on those heap dumps, 7% to 9% more memory than the index used to take, and is what HprofRetainedHeapPerfTest's new numbers are. Closes #2700 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.
Closes #2700, where a heap analysis reported a "Total retained" of -2145098144 bytes.
The bug
The traversal skipped the reference from a
java.lang.Stringto the array holding its characters, and the references from ajava.lang.Integer[]and its equivalents for the other primitive types to the boxed primitives they hold. A leak trace never needs to name those, soShallowSizeCalculatorinflated the shallow size of the object holding the content to make up for it.That's exact while each piece of content has a single holder. Content is shared more often than it looks:
new String(String)copies the reference to the array of characters rather than the array (String.java:259).Integer.valueOf()caches -128 to 127, andBoolean.valueOf()returns one of two instances.String.substring()shared its parent's array.Shared content was then credited to every object holding it. On
leak_asynctask_pre_m.hprofin our test resources, 525 arrays of characters held by up to 577 strings each add up to 9.5 MB of double counting on a 6.8 MB heap. On a JVM heap dump of 6669985 bytes holding 200 strings that share one 2 MB array, the dominator tree reported 205816571 bytes retained — 31x the heap.The fix
The traversal follows those references now, so each piece of content is counted once, against the object that dominates it, and there's nothing to add back. Content objects are leaves — an array of characters and a boxed primitive have no outgoing references — so this adds nodes to the traversal without lengthening any leak trace.
Making following cheap
Following a string's reference from the heap dump is the expensive part. Strings are 44% to 70% of the instances in the Android heap dumps in our test resources, and reading their reference takes the analysis from 11786, 17407 and 19711 random access reads to 22592, 64520 and 32494 (measured by disabling the index below).
So indexing picks the id of the array holding a string's characters up as it reads the heap dump. It already makes two sequential passes, the first to size the index exactly; that pass now also works out where
java.lang.Stringkeeps that array — which Android heap dumps don't hand over for free, 86% of the strings inleak_asynctask_m.hprofcome before the class dump ofjava.lang.String— and counts strings, so the new index is sized exactly too. The second pass harvests the id out of bytes that were streaming past anyway, so it costs no IO. When a heap dump doesn't let the first pass work out the layout (nojava.lang.String, or it holds a number of references other than one), nothing is captured and reading a string's reference falls back to reading its record.What's left of the cost is one record read per wrapper array reached, which those heap dumps hold few of.
The frozen numbers
HprofIOPerfTest, reads without computing retained size: 11786 → 11791, 17407 → 17412, 19711 → 19730 (+0.03% to +0.1%). Those extra reads are all wrapper array records: with the wrapper array skip left in place and only string content followed, the counts don't move at all. Computing retained size no longer adds any read on two of the three dumps, because the read that used to fetch a string'svaluefield to add its size back is gone.HprofRetainedHeapPerfTest, memory retained by the index: 4.4 MB → 4.8 MB (leak_asynctask_m) and 4.5 MB → 4.8 MB (leak_asynctask_o). The whole increase is the new index: 407392 and 341240 bytes, two ids per string. It's constant across the analysis steps, so following content doesn't retain anything beyond it.LegacyHprofTest.gcRootReferencesUnknownObject: total retained across two leaks 5018520 → 5018472. That heap dump holds 64 boxed primitives that more than one object references, and their size is no longer added to a wrapper array a leak retains.Tests
SharedContentRetainedSizeTest— the regression test for this issue: 200 strings sharing one 2 MB array, anIntegershared by 200 wrapper arrays, and a real JVM heap dump driven throughHotSpotDiagnosticMXBean.dumpHeap. Each asserts the root dominates the whole heap exactly once.StringValueReferenceTest— what indexing captured has to match what reading the record would have found: the array is found whenvalueisn't the first field, a string with no array has no reference, ajava.lang.Stringwith two reference fields falls back to reading the record, and a reference matcher on the field is still applied.AndroidStringValueReferenceTest— replacesStringPathFinderOptimTest, which existed to check the assumption behind the skip. For every string in the three real Android heap dumps, the reference read through the index is the one its record holds. Also verified against a run with the index disabled, which takes the fallback path and passes.Verified with
./gradlew buildon JDK 17 (ANDROID_HOMEset,JAVA_HOMEon 17). Instrumentation tests weren't run: nothing here is Android-runtime specific, and the Android heap dumps it's about are in the unit test resources.🤖 Generated with Claude Code