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

[Daniel D. Daugherty]

On 6/23/2010 12:38 PM, Jeremy Manson wrote:
> Hi Daniel,
>
> I'm sorry I missed this (I heavily filter these lists, and check rarely).
>   

This time I specifically left you on the "To" list rather than
editing down to just the list aliases.


> 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.
>   

It's possible that is a good bet. However, that wasn't mentioned
as one of the issues in either bug report (I think) so I didn't
write a test for that.


> 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'll create a variant of the anon logger test that I put in the
changeset and checkout if I can kill the Java instance with an OOM.
I'll keep you posted.


> 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.
>   

Thanks for the heads up. The JDK6 version of the fix hasn't been tested
by the customer yet so you might be right.


> 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.
>   

Yup. I caught that part of the rebuild algorithm. It's just that the
reason for doing the rebuild didn't jump out at me.

Dan


> 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
>>
>>
>>