Make Site ids be independent of instance ids.
Cleans up construction of sites by allowing sites to be constructed
before knowing of any instances that were allocated at the site.
Test: m ahat-test, with new SiteTest added.
Change-Id: I5429996a51aa3be3094989b3673d1b0a850cc3e8
diff --git a/tools/ahat/src/ObjectsHandler.java b/tools/ahat/src/ObjectsHandler.java
index fd226c2..1a8f018 100644
--- a/tools/ahat/src/ObjectsHandler.java
+++ b/tools/ahat/src/ObjectsHandler.java
@@ -37,10 +37,9 @@
@Override
public void handle(Doc doc, Query query) throws IOException {
int id = query.getInt("id", 0);
- int depth = query.getInt("depth", 0);
String className = query.get("class", null);
String heapName = query.get("heap", null);
- Site site = mSnapshot.getSite(id, depth);
+ Site site = mSnapshot.getSite(id);
List<AhatInstance> insts = new ArrayList<AhatInstance>();
site.getObjects(heapName, className, insts);
diff --git a/tools/ahat/src/SiteHandler.java b/tools/ahat/src/SiteHandler.java
index 7a831d3..543eaa3 100644
--- a/tools/ahat/src/SiteHandler.java
+++ b/tools/ahat/src/SiteHandler.java
@@ -21,6 +21,7 @@
import com.android.ahat.heapdump.Site;
import com.android.ahat.heapdump.Sort;
import java.io.IOException;
+import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.List;
@@ -39,8 +40,7 @@
@Override
public void handle(Doc doc, Query query) throws IOException {
int id = query.getInt("id", 0);
- int depth = query.getInt("depth", 0);
- Site site = mSnapshot.getSite(id, depth);
+ Site site = mSnapshot.getSite(id);
doc.title("Site");
doc.big(Summarizer.summarize(site));
@@ -49,7 +49,8 @@
SitePrinter.printSite(mSnapshot, doc, query, ALLOCATION_SITE_ID, site);
doc.section("Sites Called from Here");
- List<Site> children = site.getChildren();
+ List<Site> children = new ArrayList<Site>(site.getChildren());
+
if (children.isEmpty()) {
doc.println(DocString.text("(none)"));
} else {
@@ -99,8 +100,8 @@
String className = info.getClassName();
SizeTable.row(doc, info.numBytes, baseinfo.numBytes,
DocString.link(
- DocString.formattedUri("objects?id=%d&depth=%d&heap=%s&class=%s",
- site.getId(), site.getDepth(), info.heap.getName(), className),
+ DocString.formattedUri("objects?id=%d&heap=%s&class=%s",
+ site.getId(), info.heap.getName(), className),
DocString.format("%,14d", info.numInstances)),
DocString.delta(false, false, info.numInstances, baseinfo.numInstances),
DocString.text(info.heap.getName()),
diff --git a/tools/ahat/src/Summarizer.java b/tools/ahat/src/Summarizer.java
index 3e9da31..50b2e4b 100644
--- a/tools/ahat/src/Summarizer.java
+++ b/tools/ahat/src/Summarizer.java
@@ -130,7 +130,7 @@
if (site.getLineNumber() > 0) {
text.append(":").append(Integer.toString(site.getLineNumber()));
}
- URI uri = DocString.formattedUri("site?id=%d&depth=%d", site.getId(), site.getDepth());
+ URI uri = DocString.formattedUri("site?id=%d", site.getId());
return DocString.link(uri, text);
}
}
diff --git a/tools/ahat/src/heapdump/AhatInstance.java b/tools/ahat/src/heapdump/AhatInstance.java
index 39a844a..e1b83b8 100644
--- a/tools/ahat/src/heapdump/AhatInstance.java
+++ b/tools/ahat/src/heapdump/AhatInstance.java
@@ -72,6 +72,7 @@
* snapshot.findInstance have been initialized yet.
*/
void initialize(AhatSnapshot snapshot, Instance inst, Site site) {
+ site.addInstance(this);
mSize = new Size(inst.getSize(), 0);
mHeap = snapshot.getHeap(inst.getHeap().getName());
diff --git a/tools/ahat/src/heapdump/AhatSnapshot.java b/tools/ahat/src/heapdump/AhatSnapshot.java
index 7df78c5..2f0b30d 100644
--- a/tools/ahat/src/heapdump/AhatSnapshot.java
+++ b/tools/ahat/src/heapdump/AhatSnapshot.java
@@ -145,8 +145,7 @@
if (stack != null) {
frames = stack.getFrames();
}
- Site site = mRootSite.add(frames, frames == null ? 0 : frames.length, ahat);
- ahat.initialize(this, inst, site);
+ ahat.initialize(this, inst, mRootSite.getSite(frames));
Long registeredNativeSize = registeredNative.get(inst);
if (registeredNativeSize != null) {
@@ -177,7 +176,7 @@
heap.addToSize(superRoot.getRetainedSize(heap));
}
- mRootSite.computeObjectsInfos(mHeaps.size());
+ mRootSite.prepareForUse(0, mHeaps.size());
}
/**
@@ -260,19 +259,11 @@
return mRootSite;
}
- // Get the site associated with the given id and depth.
+ // Get the site associated with the given id.
// Returns the root site if no such site found.
- public Site getSite(int id, int depth) {
- AhatInstance obj = findInstance(id);
- if (obj == null) {
- return mRootSite;
- }
-
- Site site = obj.getSite();
- for (int i = 0; i < depth && site.getParent() != null; i++) {
- site = site.getParent();
- }
- return site;
+ public Site getSite(long id) {
+ Site site = mRootSite.findSite(id);
+ return site == null ? mRootSite : site;
}
// Return the Value for the given perflib value object.
diff --git a/tools/ahat/src/heapdump/Diff.java b/tools/ahat/src/heapdump/Diff.java
index 489f709..98c7e58 100644
--- a/tools/ahat/src/heapdump/Diff.java
+++ b/tools/ahat/src/heapdump/Diff.java
@@ -333,7 +333,7 @@
// Add placeholders to their corresponding sites.
// This requires the sites have already been diffed.
for (AhatInstance placeholder : placeholders) {
- placeholder.getBaseline().getSite().getBaseline().addPlaceHolderInstance(placeholder);
+ placeholder.getBaseline().getSite().getBaseline().addInstance(placeholder);
}
}
}
diff --git a/tools/ahat/src/heapdump/Site.java b/tools/ahat/src/heapdump/Site.java
index f0fc5d2..82931f0 100644
--- a/tools/ahat/src/heapdump/Site.java
+++ b/tools/ahat/src/heapdump/Site.java
@@ -19,6 +19,7 @@
import com.android.tools.perflib.heap.StackFrame;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -33,16 +34,20 @@
private String mFilename;
private int mLineNumber;
- // To identify this site, we pick a stack trace that includes the site.
- // mId is the id of an object allocated at that stack trace, and mDepth
- // is the number of calls between this site and the innermost site of
- // allocation of the object with mId.
- // For the root site, mId is 0 and mDepth is 0.
- private long mId;
- private int mDepth;
+ // A unique id to identify this site with. The id is chosen based on a
+ // depth first traversal of the complete site tree, which gives it the
+ // following desired properties:
+ // * The id can easily be represented in a URL.
+ // * The id is determined by the hprof file, so that the same id can be used
+ // across different instances for viewing the same hprof file.
+ // * A binary search can be used to find a site by id from the root site in
+ // log time.
+ //
+ // The id is set by prepareForUse after the complete site tree is constructed.
+ private long mId = -1;
// The total size of objects allocated in this site (including child sites),
- // organized by heap index. Computed as part of computeObjectsInfos.
+ // organized by heap index. Computed as part of prepareForUse.
private Size[] mSizesByHeap;
// List of child sites.
@@ -99,18 +104,15 @@
* Construct a root site.
*/
public Site(String name) {
- this(null, name, "", "", 0, 0, 0);
+ this(null, name, "", "", 0);
}
- public Site(Site parent, String method, String signature, String file,
- int line, long id, int depth) {
+ private Site(Site parent, String method, String signature, String file, int line) {
mParent = parent;
mMethodName = method;
mSignature = signature;
mFilename = file;
mLineNumber = line;
- mId = id;
- mDepth = depth;
mChildren = new ArrayList<Site>();
mObjects = new ArrayList<AhatInstance>();
mObjectsInfos = new ArrayList<ObjectsInfo>();
@@ -119,50 +121,63 @@
}
/**
- * Add an instance to this site.
+ * Get a child site of this site.
* Returns the site at which the instance was allocated.
- * @param frames - The list of frames in the stack trace, starting with the inner-most frame.
- * @param depth - The number of frames remaining before the inner-most frame is reached.
+ * @param frames - The list of frames in the stack trace, starting with the
+ * inner-most frame. May be null, in which case this site is
+ * returned.
*/
- Site add(StackFrame[] frames, int depth, AhatInstance inst) {
- return add(this, frames, depth, inst);
+ Site getSite(StackFrame frames[]) {
+ return frames == null ? this : getSite(this, frames);
}
- private static Site add(Site site, StackFrame[] frames, int depth, AhatInstance inst) {
- while (depth > 0) {
- StackFrame next = frames[depth - 1];
+ private static Site getSite(Site site, StackFrame frames[]) {
+ for (int s = frames.length - 1; s >= 0; --s) {
+ StackFrame frame = frames[s];
Site child = null;
for (int i = 0; i < site.mChildren.size(); i++) {
Site curr = site.mChildren.get(i);
- if (curr.mLineNumber == next.getLineNumber()
- && curr.mMethodName.equals(next.getMethodName())
- && curr.mSignature.equals(next.getSignature())
- && curr.mFilename.equals(next.getFilename())) {
+ if (curr.mLineNumber == frame.getLineNumber()
+ && curr.mMethodName.equals(frame.getMethodName())
+ && curr.mSignature.equals(frame.getSignature())
+ && curr.mFilename.equals(frame.getFilename())) {
child = curr;
break;
}
}
if (child == null) {
- child = new Site(site, next.getMethodName(), next.getSignature(),
- next.getFilename(), next.getLineNumber(), inst.getId(), depth - 1);
+ child = new Site(site, frame.getMethodName(), frame.getSignature(),
+ frame.getFilename(), frame.getLineNumber());
site.mChildren.add(child);
}
- depth = depth - 1;
site = child;
}
- site.mObjects.add(inst);
return site;
}
/**
- * Recompute the ObjectsInfos for this and all child sites.
- * This should be done after the sites tree has been formed. It should also
- * be done after dominators computation has been performed to ensure only
- * reachable objects are included in the ObjectsInfos.
- *
- * @param numHeaps - The number of heaps in the heap dump.
+ * Add an instance allocated at this site.
*/
- void computeObjectsInfos(int numHeaps) {
+ void addInstance(AhatInstance inst) {
+ mObjects.add(inst);
+ }
+
+ /**
+ * Prepare this and all child sites for use.
+ * Recomputes site ids, sizes, ObjectInfos for this and all child sites.
+ * This should be called after the sites tree has been formed and after
+ * dominators computation has been performed to ensure only reachable
+ * objects are included in the ObjectsInfos.
+ *
+ * @param id - The smallest id that is allowed to be used for this site or
+ * any of its children.
+ * @param numHeaps - The number of heaps in the heap dump.
+ * @return An id larger than the largest id used for this site or any of its
+ * children.
+ */
+ long prepareForUse(long id, int numHeaps) {
+ mId = id++;
+
// Count up the total sizes by heap.
mSizesByHeap = new Size[numHeaps];
for (int i = 0; i < numHeaps; ++i) {
@@ -183,7 +198,7 @@
// Add objects allocated in child sites.
for (Site child : mChildren) {
- child.computeObjectsInfos(numHeaps);
+ id = child.prepareForUse(id, numHeaps);
for (ObjectsInfo childInfo : child.mObjectsInfos) {
ObjectsInfo info = getObjectsInfo(childInfo.heap, childInfo.classObj);
info.numInstances += childInfo.numInstances;
@@ -193,6 +208,7 @@
mSizesByHeap[i] = mSizesByHeap[i].plus(child.mSizesByHeap[i]);
}
}
+ return id;
}
// Get the size of a site for a specific heap.
@@ -285,22 +301,49 @@
}
/**
- * Returns the id of some object allocated in this site.
+ * Returns the unique id of this site.
*/
public long getId() {
return mId;
}
/**
- * Returns the number of frames between this site and the site where the
- * object with id getId() was allocated.
+ * Find the child site with the given id.
+ * Returns null if no such site was found.
*/
- public int getDepth() {
- return mDepth;
+ public Site findSite(long id) {
+ if (id == mId) {
+ return this;
+ }
+
+ // Binary search over the children to find the right child to search in.
+ int start = 0;
+ int end = mChildren.size();
+ while (start < end) {
+ int mid = start + ((end - start) / 2);
+ Site midSite = mChildren.get(mid);
+ if (id < midSite.mId) {
+ end = mid;
+ } else if (mid + 1 == end) {
+ // This is the last child we could possibly find the desired site in,
+ // so search in this child.
+ return midSite.findSite(id);
+ } else if (id < mChildren.get(mid + 1).mId) {
+ // The desired site has an id between this child's id and the next
+ // child's id, so search in this child.
+ return midSite.findSite(id);
+ } else {
+ start = mid + 1;
+ }
+ }
+ return null;
}
+ /**
+ * Returns an unmodifiable list of this site's immediate children.
+ */
public List<Site> getChildren() {
- return mChildren;
+ return Collections.unmodifiableList(mChildren);
}
void setBaseline(Site baseline) {
@@ -314,13 +357,4 @@
@Override public boolean isPlaceHolder() {
return false;
}
-
- /**
- * Adds a place holder instance to this site and all parent sites.
- */
- void addPlaceHolderInstance(AhatInstance placeholder) {
- for (Site site = this; site != null; site = site.mParent) {
- site.mObjects.add(placeholder);
- }
- }
}
diff --git a/tools/ahat/test-dump/Main.java b/tools/ahat/test-dump/Main.java
index 14c09af..cc44260 100644
--- a/tools/ahat/test-dump/Main.java
+++ b/tools/ahat/test-dump/Main.java
@@ -102,6 +102,17 @@
public StackSmasher stackSmasherAdded;
public static String modifiedStaticField;
public int[] modifiedArray;
+ public Object objectAllocatedAtKnownSite1;
+ public Object objectAllocatedAtKnownSite2;
+
+ private void allocateObjectAtKnownSite1() {
+ objectAllocatedAtKnownSite1 = new Object();
+ allocateObjectAtKnownSite2();
+ }
+
+ private void allocateObjectAtKnownSite2() {
+ objectAllocatedAtKnownSite2 = new Object();
+ }
DumpedStuff(boolean baseline) {
int N = baseline ? 400000 : 1000000;
@@ -129,6 +140,8 @@
modifiedStaticField = baseline ? "C1" : "C2";
modifiedArray = baseline ? new int[]{0,1,2,3} : new int[]{3,1,2,0};
+ allocateObjectAtKnownSite1();
+
// Deep matching dominator trees shouldn't smash the stack when we try
// to diff them. Make some deep dominator trees to help test it.
for (int i = 0; i < 10000; i++) {
diff --git a/tools/ahat/test/SiteTest.java b/tools/ahat/test/SiteTest.java
new file mode 100644
index 0000000..dc0fe08
--- /dev/null
+++ b/tools/ahat/test/SiteTest.java
@@ -0,0 +1,66 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.ahat;
+
+import com.android.ahat.heapdump.AhatInstance;
+import com.android.ahat.heapdump.AhatSnapshot;
+import com.android.ahat.heapdump.Site;
+import java.io.IOException;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+
+public class SiteTest {
+ @Test
+ public void objectsAllocatedAtKnownSites() throws IOException {
+ TestDump dump = TestDump.getTestDump();
+ AhatSnapshot snapshot = dump.getAhatSnapshot();
+
+ AhatInstance obj1 = dump.getDumpedAhatInstance("objectAllocatedAtKnownSite1");
+ AhatInstance obj2 = dump.getDumpedAhatInstance("objectAllocatedAtKnownSite2");
+ Site s2 = obj2.getSite();
+ Site s1b = s2.getParent();
+ Site s1a = obj1.getSite();
+ Site s = s1a.getParent();
+
+ // TODO: The following commented out assertion fails due to a problem with
+ // proguard deobfuscation. That bug should be fixed.
+ // assertEquals("Main.java", s.getFilename());
+
+ assertEquals("Main.java", s1a.getFilename());
+ assertEquals("Main.java", s1b.getFilename());
+ assertEquals("Main.java", s2.getFilename());
+
+ assertEquals("allocateObjectAtKnownSite1", s1a.getMethodName());
+ assertEquals("allocateObjectAtKnownSite1", s1b.getMethodName());
+ assertEquals("allocateObjectAtKnownSite2", s2.getMethodName());
+
+ // TODO: The following commented out assertion fails due to a problem with
+ // stack frame line numbers - we don't get different line numbers
+ // for the different sites, so they are indistiguishable. The
+ // problem with line numbers should be understood and fixed.
+ // assertNotSame(s1a, s1b);
+
+ assertSame(s1a.getParent(), s1b.getParent());
+
+ assertSame(s, snapshot.getSite(s.getId()));
+ assertSame(s1a, snapshot.getSite(s1a.getId()));
+ assertSame(s1b, snapshot.getSite(s1b.getId()));
+ assertSame(s2, snapshot.getSite(s2.getId()));
+ }
+}
diff --git a/tools/ahat/test/Tests.java b/tools/ahat/test/Tests.java
index a1e3246..cd33a90 100644
--- a/tools/ahat/test/Tests.java
+++ b/tools/ahat/test/Tests.java
@@ -33,6 +33,7 @@
"com.android.ahat.RootedHandlerTest",
"com.android.ahat.QueryTest",
"com.android.ahat.SiteHandlerTest",
+ "com.android.ahat.SiteTest",
};
}
JUnitCore.main(args);