Re: Improve root set used by hat

[Kelly O'Hair]

CC to the serviceability-dev alias.

---

I have no objection to this change, anybody else care? Sundar? Alan?
If not I can file a bug, and we can decide who gets to push the change
into jdk6, and probably jdk7 too.

-kto

Keith Randall wrote:

> I'm working on tracing down a ClassLoader leak in a web application I'm
> working on (see
> http://blogs.sun.com/fkieviet/entry/classloader_leaks_the_dreaded_java 
> for a description of the kind of leak I'm talking about).  I'm using hat
> to find sources of the leak, and I have run into a problem where hat
> generates a lot of false positives of a particular form.  When I list
> the reference chains from the rootset for a ClassLoader, it lists lots
> of chains which start at a static field of a class which was loaded by
> that classloader.  This isn't a real leak, however, because if the
> ClassLoader is otherwise dead, the class containing that static field
> will be GCed.
>
> I propose that hat shouldn't consider static fields of classes which
> aren't loaded by the root classloader as GC roots.  These roots will be
> found during the normal heap walk (classloader -> class -> static
> field).  Unless of course the classloader isn't reachable, but that is
> exactly when we don't want to consider static fields in these classes as
> roots.
>
> Does this sound like a reasonable thing to do?  It works for me, just
> want to make sure it won't break some other use case for hat.
>
> Possible patch (to openjdk6 build b16):
>
> --- com/sun/tools/hat/internal/model/Snapshot.java
> ** 331,336 ****
> --- 331,345 ----
>           System.out.println();
>           for (Root r : roots) {
>               r.resolve(this);
> +             if (r.getType() == Root.JAVA_STATIC) {
> +                 JavaClass clazz = (JavaClass) r.getReferer();
> +                 if (clazz.getLoader() != nullThing) {
> +                     // this "root" will be discovered through a reference
> +                     // to its classloader, so we don't need to treat it as
> +                     // an explicit root.
> +                     continue;
> +                 }
> +             }
>               JavaHeapObject t = findThing(r.getId());
>               if (t != null) {
>                   t.addReferenceFromRoot(r);
>

[A. Sundararajan]

I agree. Static fields of classes loaded by non-bootstrap loaders should
not be part of root set.

PS. I am not sure  if this is a bug with hat or with heap dumpers (the
hotspot built-in dumper and SA's dumper). I don't remember how "roots"
is filled. May be, these static fields should not be flagged as roots?

-Sundar

Kelly O'Hair wrote:

> CC to the serviceability-dev alias.
>
> ---
>
> I have no objection to this change, anybody else care? Sundar? Alan?
> If not I can file a bug, and we can decide who gets to push the change
> into jdk6, and probably jdk7 too.
>
> -kto
>
> Keith Randall wrote:
>> I'm working on tracing down a ClassLoader leak in a web application
>> I'm working on (see
>> http://blogs.sun.com/fkieviet/entry/classloader_leaks_the_dreaded_java 
>> for a description of the kind of leak I'm talking about).  I'm using
>> hat to find sources of the leak, and I have run into a problem where
>> hat generates a lot of false positives of a particular form.  When I
>> list the reference chains from the rootset for a ClassLoader, it
>> lists lots of chains which start at a static field of a class which
>> was loaded by that classloader.  This isn't a real leak, however,
>> because if the ClassLoader is otherwise dead, the class containing
>> that static field will be GCed.
>>
>> I propose that hat shouldn't consider static fields of classes which
>> aren't loaded by the root classloader as GC roots.  These roots will
>> be found during the normal heap walk (classloader -> class -> static
>> field).  Unless of course the classloader isn't reachable, but that
>> is exactly when we don't want to consider static fields in these
>> classes as roots.
>>
>> Does this sound like a reasonable thing to do?  It works for me, just
>> want to make sure it won't break some other use case for hat.
>>
>> Possible patch (to openjdk6 build b16):
>>
>> --- com/sun/tools/hat/internal/model/Snapshot.java
>> ** 331,336 ****
>> --- 331,345 ----
>>           System.out.println();
>>           for (Root r : roots) {
>>               r.resolve(this);
>> +             if (r.getType() == Root.JAVA_STATIC) {
>> +                 JavaClass clazz = (JavaClass) r.getReferer();
>> +                 if (clazz.getLoader() != nullThing) {
>> +                     // this "root" will be discovered through a
>> reference
>> +                     // to its classloader, so we don't need to
>> treat it as
>> +                     // an explicit root.
>> +                     continue;
>> +                 }
>> +             }
>>               JavaHeapObject t = findThing(r.getId());
>>               if (t != null) {
>>                   t.addReferenceFromRoot(r);
>>

[Tomas Hurka]

Hi Sundar,

On 7 Oct 2009, at 08:28, A. Sundararajan wrote:

> I agree. Static fields of classes loaded by non-bootstrap loaders  
> should not be part of root set.
>
> PS. I am not sure  if this is a bug with hat or with heap dumpers  
> (the hotspot built-in dumper and SA's dumper). I don't remember how  
> "roots" is filled. May be, these static fields should not be flagged  
> as roots?
I think this is in hat only, since HeapWalker in Java VIsualVM does it  
correctly.

Bye,
--
Tomas Hurka   <mailto:tomas.hurka@...>
NetBeans Profiler http://profiler.netbeans.org
VisualVM http://visualvm.dev.java.net
Software Engineer, Developer Platforms Group
Sun Microsystems, Praha Czech Republic

[Alan Bateman]

A. Sundararajan wrote:
> I agree. Static fields of classes loaded by non-bootstrap loaders
> should not be part of root set.
>
> PS. I am not sure  if this is a bug with hat or with heap dumpers (the
> hotspot built-in dumper and SA's dumper). I don't remember how "roots"
> is filled. May be, these static fields should not be flagged as roots?
>
> -Sundar
At least for the built-in heap dumper, static fields aren't generated as
roots (instead it's system classes and temporary placeholders in the
dictionary that are generated to the dump as "sticky classes", to use a
HPROF term). So I suspect this is more likely to be a jhat issue.

-Alan.

[Keith Randall-3]

Here's a simpler patch that never makes Roots for these static fields in the first place:

--- com/sun/tools/hat/internal/model/JavaStatic.java
***************
*** 57,64 ****
              id = ((JavaObjectRef)value).getId();
          }
          value = value.dereference(snapshot, field);
!         if (value.isHeapAllocated() &&
!             clazz.getLoader() == snapshot.getNullThing()) { // static fields are only roots if they are in classes loaded by the root classloader.
              JavaHeapObject ho = (JavaHeapObject) value;
              String s = "Static reference from " + clazz.getName()
                         + "." + field.getName();
--- 57,63 ----
              id = ((JavaObjectRef)value).getId();
          }
          value = value.dereference(snapshot, field);
!         if (value.isHeapAllocated()) {
              JavaHeapObject ho = (JavaHeapObject) value;
              String s = "Static reference from " + clazz.getName()
                         + "." + field.getName();

On Wed, Oct 7, 2009 at 1:49 AM, Alan Bateman <Alan.Bateman@...> wrote:

A. Sundararajan wrote:

I agree. Static fields of classes loaded by non-bootstrap loaders should not be part of root set.

PS. I am not sure  if this is a bug with hat or with heap dumpers (the hotspot built-in dumper and SA's dumper). I don't remember how "roots" is filled. May be, these static fields should not be flagged as roots?

-Sundar

At least for the built-in heap dumper, static fields aren't generated as roots (instead it's system classes and temporary placeholders in the dictionary that are generated to the dump as "sticky classes", to use a HPROF term). So I suspect this is more likely to be a jhat issue.

-Alan.

[A. Sundararajan]

Hi,

This change looks good to me. Thanks for finding this issue and the fix!

Thanks,
-Sundar

Keith Randall wrote:

> Here's a simpler patch that never makes Roots for these static fields
> in the first place:
>
> --- com/sun/tools/hat/internal/model/JavaStatic.java
> ***************
> *** 57,64 ****
>               id = ((JavaObjectRef)value).getId();
>           }
>           value = value.dereference(snapshot, field);
> !         if (value.isHeapAllocated() &&
> !             clazz.getLoader() == snapshot.getNullThing()) { //
> static fields are only roots if they are in classes loaded by the root
> classloader.
>               JavaHeapObject ho = (JavaHeapObject) value;
>               String s = "Static reference from " + clazz.getName()
>                          + "." + field.getName();
> --- 57,63 ----
>               id = ((JavaObjectRef)value).getId();
>           }
>           value = value.dereference(snapshot, field);
> !         if (value.isHeapAllocated()) {
>               JavaHeapObject ho = (JavaHeapObject) value;
>               String s = "Static reference from " + clazz.getName()
>                          + "." + field.getName();
>
>
> On Wed, Oct 7, 2009 at 1:49 AM, Alan Bateman <Alan.Bateman@...
> <mailto:Alan.Bateman@...>> wrote:
>
>     A. Sundararajan wrote:
>
>         I agree. Static fields of classes loaded by non-bootstrap
>         loaders should not be part of root set.
>
>         PS. I am not sure  if this is a bug with hat or with heap
>         dumpers (the hotspot built-in dumper and SA's dumper). I don't
>         remember how "roots" is filled. May be, these static fields
>         should not be flagged as roots?
>
>         -Sundar
>
>     At least for the built-in heap dumper, static fields aren't
>     generated as roots (instead it's system classes and temporary
>     placeholders in the dictionary that are generated to the dump as
>     "sticky classes", to use a HPROF term). So I suspect this is more
>     likely to be a jhat issue.
>
>     -Alan.
>
>

[joe.darcy]

Sundar,

If you think this change should go back to OpenJDK 6 and other releases,
please work with Keith to sponsor the change.

Thanks,

-Joe

A. Sundararajan wrote:

> Hi,
>
> This change looks good to me. Thanks for finding this issue and the fix!
>
> Thanks,
> -Sundar
>
> Keith Randall wrote:
>> Here's a simpler patch that never makes Roots for these static fields
>> in the first place:
>>
>> --- com/sun/tools/hat/internal/model/JavaStatic.java
>> ***************
>> *** 57,64 ****
>>               id = ((JavaObjectRef)value).getId();
>>           }
>>           value = value.dereference(snapshot, field);
>> !         if (value.isHeapAllocated() &&
>> !             clazz.getLoader() == snapshot.getNullThing()) { //
>> static fields are only roots if they are in classes loaded by the
>> root classloader.
>>               JavaHeapObject ho = (JavaHeapObject) value;
>>               String s = "Static reference from " + clazz.getName()
>>                          + "." + field.getName();
>> --- 57,63 ----
>>               id = ((JavaObjectRef)value).getId();
>>           }
>>           value = value.dereference(snapshot, field);
>> !         if (value.isHeapAllocated()) {
>>               JavaHeapObject ho = (JavaHeapObject) value;
>>               String s = "Static reference from " + clazz.getName()
>>                          + "." + field.getName();
>>
>>
>> On Wed, Oct 7, 2009 at 1:49 AM, Alan Bateman <Alan.Bateman@...
>> <mailto:Alan.Bateman@...>> wrote:
>>
>>     A. Sundararajan wrote:
>>
>>         I agree. Static fields of classes loaded by non-bootstrap
>>         loaders should not be part of root set.
>>
>>         PS. I am not sure  if this is a bug with hat or with heap
>>         dumpers (the hotspot built-in dumper and SA's dumper). I don't
>>         remember how "roots" is filled. May be, these static fields
>>         should not be flagged as roots?
>>
>>         -Sundar
>>
>>     At least for the built-in heap dumper, static fields aren't
>>     generated as roots (instead it's system classes and temporary
>>     placeholders in the dictionary that are generated to the dump as
>>     "sticky classes", to use a HPROF term). So I suspect this is more
>>     likely to be a jhat issue.
>>
>>     -Alan.
>>
>>
>

[Kelly O'Hair]

I'll test this change:

diff --git a/src/share/classes/com/sun/tools/hat/internal/model/JavaStatic.java
b/src/share/classes/com/sun/tools/hat/internal/model/JavaStatic.java
--- a/src/share/classes/com/sun/tools/hat/internal/model/JavaStatic.java
+++ b/src/share/classes/com/sun/tools/hat/internal/model/JavaStatic.java
@@ -57,7 +57,10 @@ public class JavaStatic {
              id = ((JavaObjectRef)value).getId();
          }
          value = value.dereference(snapshot, field);
-        if (value.isHeapAllocated()) {
+        if (value.isHeapAllocated() &&
+            clazz.getLoader() == snapshot.getNullThing()) {
+            // static fields are only roots if they are in classes
+            //    loaded by the root classloader.
              JavaHeapObject ho = (JavaHeapObject) value;
              String s = "Static reference from " + clazz.getName()
                         + "." + field.getName();


Let me know if that seems right.
Unfortunately,
we haven't many jhat tests, did you have any small testcase scenarios?

The few tests for jhat are at jdk/tests/sun/tools/jhat, I can help craft one
if you give me something to start with.

-kto

Keith Randall wrote:

> Here's a simpler patch that never makes Roots for these static fields in
> the first place:
>
> --- com/sun/tools/hat/internal/model/JavaStatic.java
> ***************
> *** 57,64 ****
>               id = ((JavaObjectRef)value).getId();
>           }
>           value = value.dereference(snapshot, field);
> !         if (value.isHeapAllocated() &&
> !             clazz.getLoader() == snapshot.getNullThing()) { // static
> fields are only roots if they are in classes loaded by the root classloader.
>               JavaHeapObject ho = (JavaHeapObject) value;
>               String s = "Static reference from " + clazz.getName()
>                          + "." + field.getName();
> --- 57,63 ----
>               id = ((JavaObjectRef)value).getId();
>           }
>           value = value.dereference(snapshot, field);
> !         if (value.isHeapAllocated()) {
>               JavaHeapObject ho = (JavaHeapObject) value;
>               String s = "Static reference from " + clazz.getName()
>                          + "." + field.getName();
>
>
> On Wed, Oct 7, 2009 at 1:49 AM, Alan Bateman <Alan.Bateman@...
> <mailto:Alan.Bateman@...>> wrote:
>
>     A. Sundararajan wrote:
>
>         I agree. Static fields of classes loaded by non-bootstrap
>         loaders should not be part of root set.
>
>         PS. I am not sure  if this is a bug with hat or with heap
>         dumpers (the hotspot built-in dumper and SA's dumper). I don't
>         remember how "roots" is filled. May be, these static fields
>         should not be flagged as roots?
>
>         -Sundar
>
>     At least for the built-in heap dumper, static fields aren't
>     generated as roots (instead it's system classes and temporary
>     placeholders in the dictionary that are generated to the dump as
>     "sticky classes", to use a HPROF term). So I suspect this is more
>     likely to be a jhat issue.
>
>     -Alan.
>
>

[Kelly O'Hair]

FYI...

I haven't forgotten about this, still trying to fit it into my
schedule. I'll do openjdk6 too.

I'd like to create a testcase, but I may need to give up on that,
takes too much time to get it right, and make it test the right thing.
Any ideas on a testcase would be welcome.

-kto


Joseph D. Darcy wrote:

> Sundar,
>
> If you think this change should go back to OpenJDK 6 and other releases,
> please work with Keith to sponsor the change.
>
> Thanks,
>
> -Joe
>
> A. Sundararajan wrote:
>> Hi,
>>
>> This change looks good to me. Thanks for finding this issue and the fix!
>>
>> Thanks,
>> -Sundar
>>
>> Keith Randall wrote:
>>> Here's a simpler patch that never makes Roots for these static fields
>>> in the first place:
>>>
>>> --- com/sun/tools/hat/internal/model/JavaStatic.java
>>> ***************
>>> *** 57,64 ****
>>>               id = ((JavaObjectRef)value).getId();
>>>           }
>>>           value = value.dereference(snapshot, field);
>>> !         if (value.isHeapAllocated() &&
>>> !             clazz.getLoader() == snapshot.getNullThing()) { //
>>> static fields are only roots if they are in classes loaded by the
>>> root classloader.
>>>               JavaHeapObject ho = (JavaHeapObject) value;
>>>               String s = "Static reference from " + clazz.getName()
>>>                          + "." + field.getName();
>>> --- 57,63 ----
>>>               id = ((JavaObjectRef)value).getId();
>>>           }
>>>           value = value.dereference(snapshot, field);
>>> !         if (value.isHeapAllocated()) {
>>>               JavaHeapObject ho = (JavaHeapObject) value;
>>>               String s = "Static reference from " + clazz.getName()
>>>                          + "." + field.getName();
>>>
>>>
>>> On Wed, Oct 7, 2009 at 1:49 AM, Alan Bateman <Alan.Bateman@...
>>> <mailto:Alan.Bateman@...>> wrote:
>>>
>>>     A. Sundararajan wrote:
>>>
>>>         I agree. Static fields of classes loaded by non-bootstrap
>>>         loaders should not be part of root set.
>>>
>>>         PS. I am not sure  if this is a bug with hat or with heap
>>>         dumpers (the hotspot built-in dumper and SA's dumper). I don't
>>>         remember how "roots" is filled. May be, these static fields
>>>         should not be flagged as roots?
>>>
>>>         -Sundar
>>>
>>>     At least for the built-in heap dumper, static fields aren't
>>>     generated as roots (instead it's system classes and temporary
>>>     placeholders in the dictionary that are generated to the dump as
>>>     "sticky classes", to use a HPROF term). So I suspect this is more
>>>     likely to be a jhat issue.
>>>
>>>     -Alan.
>>>
>>>
>>
>