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

[Daniel D. Daugherty]

On 6/10/2010 7:08 PM, David Holmes wrote:
> Hi Dan,
>
> I'm surprised to see that the VM team "owns" this one :)

I'm on the "Serviceability Team". We get all the strange
technology areas... :-)


> Daniel D. Daugherty said the following on 06/11/10 03:13:
>> I need a couple of code reviews for my fix for the WeakReference leak
>> in the Logging API. The webrev is relative to OpenJDK7, but the bug
>> is escalated so the fix will be backported to the JDK6-Update train.
>> That's why I need at least two code reviewers.
>>
>> Here is the URL for the webrev:
>>
>>    http://cr.openjdk.java.net/~dcubed/6942989-webrev/0/
>
> I looked at the code and I read the CR and I understand what you are 
> doing - and in that sense it seems ok.
>
> It does seem very complicated (but the logger management is 
> complicated :( ).

You're being way too polite about the Logger API... :-)


> I wonder about the performance implications of doing this search each 
> time a logger is added? These types of cleanups are always a fine line 
> between minimizing cleanup up on the main path, and ensuring too much 
> garbage doesn't accumulate. I'm a little concerned this is a lot of 
> clean up code (potentially) on the main path.

Agreed. In this particular case, I'm slamming down hard on the
correctness side. The theory is that there shouldn't be too many
Logger objects in the normal system and once you've added them,
then this fix doesn't come into play.

Can I put you down as a "thumbs up"?

Dan