Add retained size to leak reports

Fixes #162
square · Jan 6, 2016 · 1baf962 · 1baf962
1 parent 3d1f524
commit 1baf962
Show file tree

Hide file tree

Showing 12 changed files with 202 additions and 54 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,6 +14,7 @@ You can watch releases [on Bintray](https://bintray.com/pyricau/maven/com.square
 * Added CanaryLog API to replace the logger: [#201](https://github.com/square/leakcanary/issues/201).
 * Renamed all resources to begin with `leak_canary_` instead of `__leak_canary`[#161](https://github.com/square/leakcanary/pull/161)
 * No crash when heap dump fails [#226](https://github.com/square/leakcanary/issues/226).
+* Add retained size to leak reports [#162](https://github.com/square/leakcanary/issues/162).
 
 ### Public API changes
 

diff --git a/leakcanary-analyzer/src/main/java/com/squareup/leakcanary/AnalysisResult.java b/leakcanary-analyzer/src/main/java/com/squareup/leakcanary/AnalysisResult.java
@@ -20,16 +20,17 @@
 public final class AnalysisResult implements Serializable {
 
   public static AnalysisResult noLeak(long analysisDurationMs) {
-    return new AnalysisResult(false, false, null, null, null, analysisDurationMs);
+    return new AnalysisResult(false, false, null, null, null, 0, analysisDurationMs);
   }
 
   public static AnalysisResult leakDetected(boolean excludedLeak, String className,
-      LeakTrace leakTrace, long analysisDurationMs) {
-    return new AnalysisResult(true, excludedLeak, className, leakTrace, null, analysisDurationMs);
+      LeakTrace leakTrace, long retainedHeapSize, long analysisDurationMs) {
+    return new AnalysisResult(true, excludedLeak, className, leakTrace, null, retainedHeapSize,
+        analysisDurationMs);
   }
 
   public static AnalysisResult failure(Throwable failure, long analysisDurationMs) {
-    return new AnalysisResult(false, false, null, null, failure, analysisDurationMs);
+    return new AnalysisResult(false, false, null, null, failure, 0, analysisDurationMs);
   }
 
   /** True if a leak was found in the heap dump. */
@@ -56,16 +57,23 @@ public static AnalysisResult failure(Throwable failure, long analysisDurationMs)
   /** Null unless the analysis failed. */
   public final Throwable failure;
 
+  /**
+   * The number of bytes which would be freed if all references to the leaking object were
+   * released. 0 if {@link #leakFound} is false.
+   */
+  public final long retainedHeapSize;
+
   /** Total time spent analyzing the heap. */
   public final long analysisDurationMs;
 
   private AnalysisResult(boolean leakFound, boolean excludedLeak, String className,
-      LeakTrace leakTrace, Throwable failure, long analysisDurationMs) {
+      LeakTrace leakTrace, Throwable failure, long retainedHeapSize, long analysisDurationMs) {
     this.leakFound = leakFound;
     this.excludedLeak = excludedLeak;
     this.className = className;
     this.leakTrace = leakTrace;
     this.failure = failure;
+    this.retainedHeapSize = retainedHeapSize;
     this.analysisDurationMs = analysisDurationMs;
   }
 }
diff --git a/leakcanary-analyzer/src/main/java/com/squareup/leakcanary/HeapAnalyzer.java b/leakcanary-analyzer/src/main/java/com/squareup/leakcanary/HeapAnalyzer.java
@@ -22,6 +22,7 @@
 import com.squareup.haha.perflib.HprofParser;
 import com.squareup.haha.perflib.Instance;
 import com.squareup.haha.perflib.RootObj;
+import com.squareup.haha.perflib.RootType;
 import com.squareup.haha.perflib.Snapshot;
 import com.squareup.haha.perflib.Type;
 import com.squareup.haha.perflib.io.HprofBuffer;
@@ -119,10 +120,73 @@ private AnalysisResult findLeakTrace(long analysisStartNanoTime, Snapshot snapsh
 
     String className = leakingRef.getClassObj().getClassName();
 
-    return leakDetected(result.excludingKnownLeaks, className, leakTrace,
+    // Side effect: computes retained size.
+    snapshot.computeDominators();
+
+    Instance leakingInstance = result.leakingNode.instance;
+
+    long retainedSize = leakingInstance.getTotalRetainedSize();
+
+    retainedSize += computeIgnoredBitmapRetainedSize(snapshot, leakingInstance);
+
+    return leakDetected(result.excludingKnownLeaks, className, leakTrace, retainedSize,
         since(analysisStartNanoTime));
   }
 
+  /**
+   * Bitmaps and bitmap byte arrays are sometimes held by native gc roots, so they aren't included
+   * in the retained size because their root dominator is a native gc root.
+   * To fix this, we check if the leaking instance is a dominator for each bitmap instance and then
+   * add the bitmap size.
+   *
+   * From experience, we've found that bitmap created in code (Bitmap.createBitmap()) are correctly
+   * accounted for, however bitmaps set in layouts are not.
+   */
+  private int computeIgnoredBitmapRetainedSize(Snapshot snapshot, Instance leakingInstance) {
+    int bitmapRetainedSize = 0;
+    ClassObj bitmapClass = snapshot.findClass("android.graphics.Bitmap");
+
+    for (Instance bitmapInstance : bitmapClass.getInstancesList()) {
+      if (isIgnoredDominator(leakingInstance, bitmapInstance)) {
+        ArrayInstance mBufferInstance = fieldValue(classInstanceValues(bitmapInstance), "mBuffer");
+        // Native bitmaps have mBuffer set to null. We sadly can't account for them.
+        if (mBufferInstance == null) {
+          continue;
+        }
+        long bufferSize = mBufferInstance.getTotalRetainedSize();
+        long bitmapSize = bitmapInstance.getTotalRetainedSize();
+        // Sometimes the size of the buffer isn't accounted for in the bitmap retained size. Since
+        // the buffer is large, it's easy to detect by checking for bitmap size < buffer size.
+        if (bitmapSize < bufferSize) {
+          bitmapSize += bufferSize;
+        }
+        bitmapRetainedSize += bitmapSize;
+      }
+    }
+    return bitmapRetainedSize;
+  }
+
+  private boolean isIgnoredDominator(Instance dominator, Instance instance) {
+    boolean foundNativeRoot = false;
+    while (true) {
+      Instance immediateDominator = instance.getImmediateDominator();
+      if (immediateDominator instanceof RootObj
+          && ((RootObj) immediateDominator).getRootType() == RootType.UNKNOWN) {
+        // Ignore native roots
+        instance = instance.getNextInstanceToGcRoot();
+        foundNativeRoot = true;
+      } else {
+        instance = immediateDominator;
+      }
+      if (instance == null) {
+        return false;
+      }
+      if (instance == dominator) {
+        return foundNativeRoot;
+      }
+    }
+  }
+
   private LeakTrace buildLeakTrace(LeakNode leakingNode) {
     List<LeakTraceElement> elements = new ArrayList<>();
     // We iterate from the leak to the GC root

diff --git a/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/AsyncTaskLeakTest.java b/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/AsyncTaskLeakTest.java
@@ -15,7 +15,6 @@
  */
 package com.squareup.leakcanary;
 
-import java.io.File;
 import java.lang.ref.WeakReference;
 import java.util.Arrays;
 import java.util.Collection;
@@ -24,10 +23,12 @@
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 
-import static com.squareup.leakcanary.TestUtil.fileFromName;
-import static com.squareup.leakcanary.TestUtil.analyze;
 import static com.squareup.leakcanary.LeakTraceElement.Holder.THREAD;
 import static com.squareup.leakcanary.LeakTraceElement.Type.STATIC_FIELD;
+import static com.squareup.leakcanary.TestUtil.HeapDumpFile.ASYNC_TASK;
+import static com.squareup.leakcanary.TestUtil.HeapDumpFile.ASYNC_TASK_MPREVIEW2;
+import static com.squareup.leakcanary.TestUtil.HeapDumpFile.ASYNC_TASK_M_POSTPREVIEW2;
+import static com.squareup.leakcanary.TestUtil.analyze;
 import static org.hamcrest.core.StringContains.containsString;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -44,20 +45,17 @@ public class AsyncTaskLeakTest {
 
   @Parameterized.Parameters public static Collection<Object[]> data() {
     return Arrays.asList(new Object[][] {
-        { fileFromName("leak_asynctask.hprof"), "dc983a12-d029-4003-8890-7dd644c664c5" },
-        { fileFromName("leak_asynctask_mpreview2.hprof"), "1114018e-e154-435f-9a3d-da63ae9b47fa" },
-        { fileFromName("leak_asynctask_m_postpreview2.hprof"), "25ae1778-7c1d-4ec7-ac50-5cce55424069" }
+        { ASYNC_TASK }, //
+        { ASYNC_TASK_MPREVIEW2 }, //
+        { ASYNC_TASK_M_POSTPREVIEW2 } //
     });
   }
 
-  final File heapDumpFile;
-  final String referenceKey;
-
+  private final TestUtil.HeapDumpFile heapDumpFile;
   ExcludedRefs.Builder excludedRefs;
 
-  public AsyncTaskLeakTest(File heapDumpFile, String referenceKey) {
+  public AsyncTaskLeakTest(TestUtil.HeapDumpFile heapDumpFile) {
     this.heapDumpFile = heapDumpFile;
-    this.referenceKey = referenceKey;
   }
 
   @Before public void setUp() {
@@ -66,7 +64,7 @@ public AsyncTaskLeakTest(File heapDumpFile, String referenceKey) {
   }
 
   @Test public void leakFound() {
-    AnalysisResult result = analyze(heapDumpFile, referenceKey, excludedRefs);
+    AnalysisResult result = analyze(heapDumpFile, excludedRefs);
     assertTrue(result.leakFound);
     assertFalse(result.excludedLeak);
     LeakTraceElement gcRoot = result.leakTrace.elements.get(0);
@@ -77,7 +75,7 @@ public AsyncTaskLeakTest(File heapDumpFile, String referenceKey) {
 
   @Test public void excludeThread() {
     excludedRefs.thread(ASYNC_TASK_THREAD);
-    AnalysisResult result = analyze(heapDumpFile, referenceKey, excludedRefs);
+    AnalysisResult result = analyze(heapDumpFile, excludedRefs);
     assertTrue(result.leakFound);
     assertFalse(result.excludedLeak);
     LeakTraceElement gcRoot = result.leakTrace.elements.get(0);
@@ -91,9 +89,8 @@ public AsyncTaskLeakTest(File heapDumpFile, String referenceKey) {
     excludedRefs.thread(ASYNC_TASK_THREAD);
     excludedRefs.staticField(ASYNC_TASK_CLASS, EXECUTOR_FIELD_1);
     excludedRefs.staticField(ASYNC_TASK_CLASS, EXECUTOR_FIELD_2);
-    AnalysisResult result = analyze(heapDumpFile, referenceKey, excludedRefs);
+    AnalysisResult result = analyze(heapDumpFile, excludedRefs);
     assertTrue(result.leakFound);
     assertTrue(result.excludedLeak);
   }
-
 }
diff --git a/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/RetainedSizeTest.java b/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/RetainedSizeTest.java
@@ -0,0 +1,53 @@
+package com.squareup.leakcanary;
+
+import java.lang.ref.WeakReference;
+import java.util.Arrays;
+import java.util.Collection;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static com.squareup.leakcanary.TestUtil.HeapDumpFile.ASYNC_TASK;
+import static com.squareup.leakcanary.TestUtil.HeapDumpFile.ASYNC_TASK_MPREVIEW2;
+import static com.squareup.leakcanary.TestUtil.HeapDumpFile.ASYNC_TASK_M_POSTPREVIEW2;
+import static com.squareup.leakcanary.TestUtil.HeapDumpFile.SERVICE_BINDER;
+import static com.squareup.leakcanary.TestUtil.HeapDumpFile.SERVICE_BINDER_IGNORED;
+import static com.squareup.leakcanary.TestUtil.analyze;
+import static org.junit.Assert.assertEquals;
+
+/**
+ * This test makes sure there is no regression on the retained size calculation.
+ */
+@RunWith(Parameterized.class) //
+public class RetainedSizeTest {
+
+  @Parameterized.Parameters public static Collection<Object[]> data() {
+    return Arrays.asList(new Object[][] {
+        { ASYNC_TASK, 207_407 }, //
+        { ASYNC_TASK_MPREVIEW2, 1_604 }, //
+        { ASYNC_TASK_M_POSTPREVIEW2, 1_870 }, //
+        { SERVICE_BINDER, 378 }, //
+        { SERVICE_BINDER_IGNORED, 378 }, //
+    });
+  }
+
+  private final TestUtil.HeapDumpFile heapDumpFile;
+  private final long expectedRetainedHeapSize;
+  ExcludedRefs.Builder excludedRefs;
+
+  public RetainedSizeTest(TestUtil.HeapDumpFile heapDumpFile, long expectedRetainedHeapSize) {
+    this.heapDumpFile = heapDumpFile;
+    this.expectedRetainedHeapSize = expectedRetainedHeapSize;
+  }
+
+  @Before public void setUp() {
+    excludedRefs = new ExcludedRefs.Builder().clazz(WeakReference.class.getName(), true)
+        .clazz("java.lang.ref.FinalizerReference", true);
+  }
+
+  @Test public void leakFound() {
+    AnalysisResult result = analyze(heapDumpFile, excludedRefs);
+    assertEquals(expectedRetainedHeapSize, result.retainedHeapSize);
+  }
+}
diff --git a/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/ServiceBinderLeakTest.java b/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/ServiceBinderLeakTest.java
@@ -15,15 +15,16 @@
  */
 package com.squareup.leakcanary;
 
+import java.lang.ref.WeakReference;
 import org.junit.Before;
 import org.junit.Test;
 
-import java.lang.ref.WeakReference;
-
 import static com.squareup.leakcanary.LeakTraceElement.Holder.CLASS;
 import static com.squareup.leakcanary.LeakTraceElement.Holder.OBJECT;
 import static com.squareup.leakcanary.LeakTraceElement.Type.INSTANCE_FIELD;
 import static com.squareup.leakcanary.LeakTraceElement.Type.STATIC_FIELD;
+import static com.squareup.leakcanary.TestUtil.HeapDumpFile.SERVICE_BINDER;
+import static com.squareup.leakcanary.TestUtil.HeapDumpFile.SERVICE_BINDER_IGNORED;
 import static com.squareup.leakcanary.TestUtil.analyze;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -38,19 +39,14 @@ public class ServiceBinderLeakTest {
   ExcludedRefs.Builder excludedRefs;
 
   @Before public void setUp() {
-    excludedRefs = new ExcludedRefs.Builder()
-        .clazz(WeakReference.class.getName(), true)
+    excludedRefs = new ExcludedRefs.Builder().clazz(WeakReference.class.getName(), true)
         .clazz("java.lang.ref.FinalizerReference", true);
   }
 
   @Test public void realBinderLeak() {
     excludedRefs.rootSuperClass("android.os.Binder", true);
 
-    AnalysisResult result = analyze(
-        TestUtil.fileFromName("leak_service_binder.hprof"),
-        "b3abfae6-2c53-42e1-b8c1-96b0558dbeae",
-        excludedRefs
-    );
+    AnalysisResult result = analyze(SERVICE_BINDER, excludedRefs);
 
     assertTrue(result.leakFound);
     assertFalse(result.excludedLeak);
@@ -63,11 +59,7 @@ public class ServiceBinderLeakTest {
   @Test public void ignorableBinderLeak() {
     excludedRefs.rootSuperClass("android.os.Binder", false);
 
-    AnalysisResult result = analyze(
-        TestUtil.fileFromName("leak_service_binder_ignored.hprof"),
-        "6e524414-9581-4ce7-8690-e8ddf8b82454",
-        excludedRefs
-    );
+    AnalysisResult result = analyze(SERVICE_BINDER_IGNORED, excludedRefs);
 
     assertTrue(result.leakFound);
     assertTrue(result.excludedLeak);
@@ -80,13 +72,8 @@ public class ServiceBinderLeakTest {
   @Test public void alwaysIgnorableBinderLeak() {
     excludedRefs.rootSuperClass("android.os.Binder", true);
 
-    AnalysisResult result = analyze(
-        TestUtil.fileFromName("leak_service_binder_ignored.hprof"),
-        "6e524414-9581-4ce7-8690-e8ddf8b82454",
-        excludedRefs
-    );
+    AnalysisResult result = analyze(SERVICE_BINDER_IGNORED, excludedRefs);
 
     assertFalse(result.leakFound);
   }
-
 }
diff --git a/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/TestUtil.java b/leakcanary-analyzer/src/test/java/com/squareup/leakcanary/TestUtil.java
@@ -19,15 +19,38 @@
 import java.net.URL;
 
 final class TestUtil {
+
+  enum HeapDumpFile {
+    ASYNC_TASK("leak_asynctask.hprof", "dc983a12-d029-4003-8890-7dd644c664c5"),
+    ASYNC_TASK_MPREVIEW2("leak_asynctask_mpreview2.hprof", "1114018e-e154-435f-9a3d-da63ae9b47fa"),
+    ASYNC_TASK_M_POSTPREVIEW2("leak_asynctask_m_postpreview2.hprof",
+        "25ae1778-7c1d-4ec7-ac50-5cce55424069"),
+
+    SERVICE_BINDER("leak_service_binder.hprof", "b3abfae6-2c53-42e1-b8c1-96b0558dbeae"),
+    SERVICE_BINDER_IGNORED("leak_service_binder_ignored.hprof",
+        "6e524414-9581-4ce7-8690-e8ddf8b82454"),;
+
+    private final String filename;
+    private final String referenceKey;
+
+    HeapDumpFile(String filename, String referenceKey) {
+      this.filename = filename;
+      this.referenceKey = referenceKey;
+    }
+
+  }
+
   static File fileFromName(String filename) {
     ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
     URL url = classLoader.getResource(filename);
     return new File(url.getPath());
   }
 
-  static AnalysisResult analyze(File heapDumpFile, String referenceKey, ExcludedRefs.Builder excludedRefs) {
+  static AnalysisResult analyze(HeapDumpFile heapDumpFile, ExcludedRefs.Builder excludedRefs) {
+    File file = fileFromName(heapDumpFile.filename);
+    String referenceKey = heapDumpFile.referenceKey;
     HeapAnalyzer heapAnalyzer = new HeapAnalyzer(excludedRefs.build());
-    AnalysisResult result = heapAnalyzer.checkForLeak(heapDumpFile, referenceKey);
+    AnalysisResult result = heapAnalyzer.checkForLeak(file, referenceKey);
     if (result.failure != null) {
       result.failure.printStackTrace();
     }