Should GraphLayout/parseInstance use IdentityHashMap for GraphPathRecord's?

[Andy Fingerhut]

Thanks for developing JOL and making it so accurate!  I've recently been
building a little utility library in Clojure on top of it, primarily for
learning about Clojure and JVM implementation details.
https://github.com/jafingerhut/cljol

I was looking through the implementation of the object graph walking in
GraphLayout's parseInstance method, and realized that while it might not be
terribly likely, the TreeMap named 'addresses' could have 'addresses.put()'
called on it for the same address multiple times, if a GC happens during
graph walking.

To see if this was happening in a few local tests of my own, I made a
change via the small diff that I will attempt to append to this message
below.  It turns out that one of the tests run via 'mvn package' caused the
new exception to be thrown very consistently, unless changes are made to
that test to force it to GC first.

It seems that perhaps if 'addresses' was replaced with an IdentityHashMap
that this problem could be avoided, since the key is the object's identity
(even across GC moves), not whatever its current address happens to be.
Would there perhaps be any interest in a patch like that?  One down side is
that it would change the API for those that use GraphLayout.parseInstance.

Thanks,
Andy Fingerhut

In case it makes any difference, this patch is against the 0.9 version of
JOL's source code, not the latest.

diff -r 9a5fff552b23
jol-core/src/main/java/org/openjdk/jol/info/GraphLayout.java
--- a/jol-core/src/main/java/org/openjdk/jol/info/GraphLayout.java Fri Sep
22 17:29:07 2017 +0200
+++ b/jol-core/src/main/java/org/openjdk/jol/info/GraphLayout.java Sat Jul
06 10:36:19 2019 -0700
@@ -106,6 +106,15 @@

     private void addRecord(GraphPathRecord gpr) {
         long addr = gpr.address();
+ if (addresses.containsKey(addr)) {
+    Object cur_val = addresses.get(addr);
+    Object new_val = gpr.obj();
+    if (new_val != cur_val) {
+ String msg = "Internal error - GraphLayout.addRecord called with address
" + addr + " object '" + new_val.getClass() + "' that is a duplicate of an
object with '" + cur_val.getClass() + "' traversed earlier.  Likely a GC
occurred while walking an object graph.";
+ System.out.println(msg);
+ throw new IllegalArgumentException(msg);
+    }
+ }
         addresses.put(addr, gpr);

         Class<?> klass = gpr.klass();
diff -r 9a5fff552b23
jol-core/src/test/java/org/openjdk/jol/info/ClassLayoutPrivatesTest.java
---
a/jol-core/src/test/java/org/openjdk/jol/info/ClassLayoutPrivatesTest.java
Fri Sep 22 17:29:07 2017 +0200
+++
b/jol-core/src/test/java/org/openjdk/jol/info/ClassLayoutPrivatesTest.java
Sat Jul 06 10:36:19 2019 -0700
@@ -26,6 +26,17 @@

     @Test
     public void testClass() {
+ System.gc();
+ try {
+    Thread.sleep(100);
+ } catch (InterruptedException e) {
+ }
+ System.gc();
+ try {
+    Thread.sleep(100);
+ } catch (InterruptedException e) {
+ }
+ System.gc();
         GraphLayout.parseInstance(this.getClass()).toPrintable();
     }