Detect cycles in WeakHashMaps

[eclipsewebmaster]

| --- | --- |
| Bugzilla Link | 296826 |
| Status | ASSIGNED |
| Importance | P3 enhancement |
| Reported | Dec 03, 2009 11:22 EDT |
| Modified | Feb 25, 2020 16:12 EDT |
| Version | 1.1 |
| Depends on | 297052 |
| See also | Gerrit change https://git.eclipse.org/r/158295, Git commit e6465836, Gerrit change https://git.eclipse.org/r/158301, Git commit a78a213e, Gerrit change https://git.eclipse.org/r/158350, Git commit d8e9667e |
| Reporter | Andrew Johnson |

Description

WeakHashMaps have entries with a key held by the reference of a WeakReference and a value. If the key is no longer accessible and is only weakly reachable then it can be cleared and then the hash map entry can be cleared.

A user error is to have a path from the value to the key. The key is then strongly referenced via the WeakReference value field and won't be cleared when otherwise inaccessible.

We should detect this problem.

[eclipsewebmaster]

By Andrew Johnson on Dec 03, 2009 15:33

I've got a prototype for solving this, based on weak_reference_query.

Current steps:
1.Generate histogram of referent objects of weak references
2.Generate retained set of weakly reachable objects from weak references

Next steps:
3.Generate retained set of all objects retained by weak references
4.Remove weakly reachable retained objects 2. from 3. leaving strongly reachable retained set.
5.Find all referent objects from 1. contained in 4. (i.e. which are strongly reachable from the weak reference)

This should should show leaks where in a WeakHashMap the value retains the key.

Is there a method to get a strongly retained set?

Is there a method to get the reachable set? This could be used to show potential rather than actual problems.

[eclipsewebmaster]

By Andrew Johnson on Dec 10, 2009 05:00

I've committed some changes. There is now a general references query on selected objects.

The component report now produces a report for SoftReferences and WeakReferences. The GC paths view doesn't come out properly because of 297052.

Once the report is working well then we could do a more general query for leaking references and if there is a problem then include it in the leak suspects.

How could this reference query and the component report be improved?

[eclipsewebmaster]

By Krum Tsvetkov on Dec 16, 2009 03:38

I tested this with the last heap dump we showed at JavaOne this year. I ran the "reference statistics" query and the "component report" on all instances of java.util.WeakHashMap$Entry

The results were very good I think. Both showed me the leaking classloaders we demonstrated, but also some additional problems which I haven't noticed before :)

However, I think it will be hard for non-experts to use it:

1. the query works best if you run it against the the WeakHashMap$Entry instances, but people should come to the idea to do it exaxtly on the WeakMaps :)
2. when I ran the component report not on the Weak entries, but on my component which suffered from the leak, then I didn't get the suspects. The reason is that the WeakHashMaps where my components were kept belonged to some other component. It gave me though, a small part of the suspects where the Weak maps belonged to my component. And this is good. Nevertheless, I think we have to report also instances of my component, which are affected by the weak map value -> ref problem in other components (loggers, registries, etc...).
3. Finding the problematic path (or part of the path) should be improved.
   3.1) just running the Reference statistics I get a long list of suspects, and no paths. May be we could add them as a separate tab.
   3.2) Having all paths expanded in the component report was also tough (I described it in more details in the referenced message).
   3.3) Having the whole path has both positive (you see also where the WeakMap entry is referenced), and negative sides (you don't see the important part from the value to the referent at a glance)

What could make it a bit simpler is to have a dedicated query for this, sth. like "Find possible leaks in WeakHashMaps" (with blinking pink title :))
If it is a separate query which we document well the chances that people understand the use of it are much higher.
And also if we manage to provide only the most interesting paths and highlight the part from the value to the referent, then I think people will see at a glance what is wrong.
I know my comments are so far not very constructive so far, but I hope these could be some ideas we could use as a beginning, and then come with some concrete solution.

What do you think about the suggestions?

[eclipsewebmaster]

By Krum Tsvetkov on Apr 23, 2010 04:07

We can't finish this one for the 1.0 release. I move it to 1.1 and remove the "blocks" attribute for the central graduation ticket.

[eclipsewebmaster]

By Andrew Johnson on Feb 25, 2020 05:57

Created attachment 281924
Paths to referent

pathstoreferent.png

[eclipsewebmaster]

By Andrew Johnson on Feb 25, 2020 05:58

Created attachment 281925
Common paths

commonpaths.png

[eclipsewebmaster]

By Andrew Johnson on Feb 25, 2020 06:39

I have written a query that processes references one by one looking for referents which also have a strong path
back to the reference.
It produces a path for the first 5 (configurable) and then a common path by class for all of them.
This query generates a composite result with trees as a result.

The component report can then call this query and generate HTML.
I thought it would be better if the component report processed the referent objects by class, collected the
references and ran the query on those. This might isolate any problems as it is likely the referents for a
particular leak will be of the same class.

I needed a minor modification to displaying trees to make sure the selection was honoured.

[eclipsewebmaster]

Feb 25, 2020 06:48

New Gerrit change created: https://git.eclipse.org/r/158295

[eclipsewebmaster]

Feb 25, 2020 06:50

Gerrit change https://git.eclipse.org/r/158295 was merged to [master].
Commit: http://git.eclipse.org/c/mat/org.eclipse.mat.git/commit/?id=e646583637913aaf5f8133d15dd711be8cdb8f87

[eclipsewebmaster]

Feb 25, 2020 08:08

New Gerrit change created: https://git.eclipse.org/r/158301

[eclipsewebmaster]

Feb 25, 2020 08:09

Gerrit change https://git.eclipse.org/r/158301 was merged to [master].
Commit: http://git.eclipse.org/c/mat/org.eclipse.mat.git/commit/?id=a78a213ef16199cd202c17e0beb05d3a4d8702a7

[eclipsewebmaster]

By Andrew Johnson on Feb 25, 2020 08:25

I have a slightly unusual result with the test dump sun_jdk6_32.hprof

reference_leak java.lang.ref.WeakReference -include_subclasses -maxpaths 10 -factor 0.0

This includes java.util.WeakHashMap$Entry @ 0x22eb76c0 -> com.sun.jmx.interceptor.DefaultMBeanServerInterceptor$ListenerWrapper
because there is another path avoiding the referent to com.sun.jmx.interceptor.DefaultMBeanServerInterceptor$ListenerWrapper

That path goes through another WeakReference. The query considers each reference in turn, ignoring the others (presuming them as strong),
so flags it as a problem. That might be a false positive, but perhaps a complicated arrangement of WeakReferences could cause a problem.

The component report does not flag it as a problem because across all the weak references there are no referents strongly retained via weak references.

Perhaps this needs some help or documentation to let the user decide what is happening.

Class Name | Shallow Heap | Retained Heap
-----------------------------------------------------------------------------------------------------------------------------
java.util.WeakHashMap @ 0x22e755b0 | 48 | 352
'- java.util.WeakHashMap$Entry[16] @ 0x22e7bc50 | 80 | 272
'- java.util.WeakHashMap$Entry @ 0x22eb76c0 | 40 | 64
|- com.sun.jmx.interceptor.DefaultMBeanServerInterceptor$ListenerWrapper @ 0x22ebf7e8 | 24 | 24
|- java.lang.ref.WeakReference @ 0x22ebf7d0 | 24 | 24
| '- com.sun.jmx.interceptor.DefaultMBeanServerInterceptor$ListenerWrapper @ 0x22ebf7e8| 24 | 24\

[eclipsewebmaster]

Feb 25, 2020 16:12

New Gerrit change created: https://git.eclipse.org/r/158350

[eclipsewebmaster]

Feb 25, 2020 16:12

Gerrit change https://git.eclipse.org/r/158350 was merged to [master].
Commit: http://git.eclipse.org/c/mat/org.eclipse.mat.git/commit/?id=d8e9667ef3bbdc2b3d9f669fdf7133fd0c3c452b