Second Code Review for WeakReference leak in the Logging API (6942989)

[Jeremy Manson]

Hi Daniel,

I'm sorry I missed this (I heavily filter these lists, and check rarely).

My main feeling is that you are missing a good bet by not
reconstructing the Hashtable in LogManager and the ArrayList in Logger
every so often when you remove the loggers.  In a test case where
there are a LOT of short-lived loggers, the backing array for the
Hashtable can get very big.  It is permanent, and doesn't go anywhere.
 You can end up with a lot of extra memory lying around that way.

Specifically, when I didn't reconstruct those data structures, the
test case listed in the bug (where it just creates lots and lots of
anonymous loggers) killed the Java instance with an OOM, even if I
*did* clean up the weakreferences to the loggers.

I'm assuming you have a customer waiting for this - if that is similar
to their usage pattern, this fix may not fix their problem.

You obviously don't want to rebuild those structures every time,
though.  What I did in my change was to reconstruct the backing data
structures every time ~as many loggers were collected as were present
in the data structure.

Jeremy

On Fri, Jun 18, 2010 at 12:25 PM, Daniel D. Daugherty
<daniel.daugherty at oracle.com> wrote:
> Greetings,
>
> I have a new version of my fix for the WeakReference leak in the
> Logging API done. This version uses ReferenceQueues; thanks to Eamonn
> McManus, Jeremy Manson and Tony Printezis for their insights on using
> ReferenceQueues. Here's a pointer to Tony's paper for background info:
>
>    http://java.sun.com/developer/technicalArticles/javase/finalization/
>
> This version also has limits on the number of dead Loggers that are
> cleaned up per call; thanks to Alan Bateman for politely pushing me in
> that direction.
>
> The webrev is again relative to OpenJDK7, but the bug is escalated so
> the fix will be backported to the JDK6-Update train. So again, I'll
> need a minimum of two code reviewers.
>
> Here is the URL for the webrev:
>
>    http://cr.openjdk.java.net/~dcubed/6942989-webrev/1/
>
> Thanks, in advance, for any reviews.
>
> Dan
>
>