I think I took care of the DefinitionsPane memory leak that we’ve seen occasionally in our tests during the last years (actually, the last 4 years, 9 months, judging by the repository). The NULL_DOCUMENT instance in AbstractDJPane.java had listeners, and those listeners held references to the DefinitionsPane or the OpenDefinitionsDocument.
For those of you who aren’t that familiar with the code base, the NULL_DOCUMENT is an example of the “null object design pattern”. There is the notion of a “current document”, but in some cases, e.g. during
initialization, there may not be a current document. We could set the field for the current document to null, but then we would have to check for null everywhere the field is used. That’s not very robust. Instead, we created an instance of a document, called NULL_DOCUMENT, that isn’t really used as a document at all. It’s just used to signal that there is no current document, but to act graciously if the field is used.
The document still had listeners, though, even though it should really just be a dummy. It should be as much a dummy as null, without the side effects of a NullPointerException. After tracing the problem to
NULL_DOCUMENT (more on how i did that below), I overrode the addXXXListener
methods for NULL_DOCUMENT to be no-ops. Now listeners just cannot be added to this instance. The memory leak was only intermittent, so it’s difficult to say for sure that the problem has been remedied, but I’ve
run 120 repetitions of the DefinitionsPaneTest and not seen the problem again (before the change, I couldn’t run five repetitions without noticing it).
I want to discuss a little bit how I spotted this problem: There are two tools that Java provides, jmap and jhat (really starting with JDK6; unfortunately not supported on the Mac). jmap dumps the entire heap of a JVM into a file. jhat then analyzes that file and provides a HTML interface to browse the results. For more information, look at this article: Finding Memory Leaks in Java Apps.
The biggest problem here was that the problem wasn’t visible all the time, and when we noticed it (because the unit test failed), it was really too late already to execute jmap to retrieve a heap dump. So I wrote a dumpHeap() method that executes jmap automatically, from within the program; that way, I can dump it only if the memory leak has been detected:
/** Dumps the current heap to a file. */
public static File dumpHeap() throws IOException, InterruptedException {
String javaHome = System.getenv("JAVA_HOME");
char SEP = File.separatorChar;
// try jps first
File jps = new File(javaHome+SEP+"bin"+SEP+"jps");
// if that doesn't work, try jps.exe
if (!jps.exists()) jps = new File(javaHome+SEP+"bin"+SEP+"jps.exe");
// execute jps
ProcessBuilder pb = new ProcessBuilder(jps.getAbsolutePath());
LOG.log(java.util.Arrays.toString(pb.command().toArray()));
Process jpsProc = pb.start();
jpsProc.waitFor();
LOG.log("jps returned "+jpsProc.exitValue());
// read the output of jps
BufferedReader br = new BufferedReader(new InputStreamReader(jpsProc.getInputStream()));
Integer pid = null;
String line = null;
while((pid==null) && (line=br.readLine())!=null) {
LOG.log(line);
// find the PID of JUnitTestRunner, i.e. the PID of the current process
if (line.indexOf("JUnitTestRunner")>=0) {
pid = new Integer(line.substring(0,line.indexOf(' ')));
}
}
if (pid==null) throw new FileNotFoundException("Could not detect PID");
LOG.log("PID is "+pid);
// try jmap first
File jmap = new File(javaHome+SEP+"bin"+SEP+"jmap");
// if that doesn't work, try jmap.exe
if (!jmap.exists()) jmap = new File(javaHome+SEP+"bin"+SEP+"jmap.exe");
// execute jmap -heap:format=b PID
pb = new ProcessBuilder(jmap.getAbsolutePath(),
"-heap:format=b",
pid.toString());
LOG.log(java.util.Arrays.toString(pb.command().toArray()));
Process jmapProc = pb.start();
jmapProc.waitFor();
LOG.log("jmap returned "+jmapProc.exitValue());
// read the output of jmap
br = new BufferedReader(new InputStreamReader(jmapProc.getInputStream()));
while((line=br.readLine())!=null) {
LOG.log(line);
}
// rename the file
File dump = new File("heap.bin");
if (!dump.exists()) { throw new FileNotFoundException("heap.bin not found"); }
File newDump = new File("heap-DefinitionsPaneTest-"+pid+"-"+System.currentTimeMillis()+".bin");
dump.renameTo(newDump);
return newDump;
}
Then I still had to associate the instances of DefinitionsPane and OpenDefinitionsDocument with the instances reported in the HTML output. The contents of integer fields is displayed in the output, so I added a field that contains the identity hash code.
When I looked at the reference chains from the root set (which cannot be garbage-collected) to an instance of DefinitionsPane, I noticed that they all end with the following part:
--> class edu.rice.cs.drjava.ui.AbstractDJPane (84 bytes) (static field NULL_DOCUMENT:) --> edu.rice.cs.util.text.SwingDocument@0x72b4b840 (78 bytes) (field listenerList:) --> javax.swing.event.EventListenerList@0x72b4d450 (12 bytes) (field listenerList:) --> [Ljava.lang.Object;@0x74f52968 (136 bytes) (Element 23 of [Ljava.lang.Object;@0x74f52968:) --> javax.swing.plaf.basic.BasicTextUI$UpdateHandler@0x74f6b578 (17 bytes) (field this$0:) --> javax.swing.plaf.basic.BasicTextPaneUI@0x74f6b5b0 (29 bytes) (field editor:) --> edu.rice.cs.drjava.ui.DefinitionsPane@0x74f67eb8 (559 bytes)
That means that if we can break this part of the chain, the DefinitionsPane can be deallocated. By not allowing listeners, that’s what I have done.
and I thought maybe that was supposed to be the symbol for antisymmetry, but it wasn’t defined anywhere.
crossed out. Durrh.
