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

[Alan Bateman]

Daniel D. Daugherty wrote:
> :
> Either of those schemes would be fine, but not for this fix and
> not without a good reason to do so. The theory is that there
> shouldn't be too many Logger objects in a normal system and
> once they have been added, then this fix doesn't come into play.
> I would be surprised if a real system had more than 100 Logger
> objects.
I'm sure this is mostly true but there will be extreme cases where it 
might be a problem. I remember Mandy discovering that AWT has a Logger 
per class so you might want to check that the changes don't impact 
startup -  as I recall all the AWT Loggers are created in static 
initializers (discussed on awt-dev a while back).

> :
>
> So would "kidsCleanupSentinel" be better? (I actually rather liked
> the implied humor of kids and markers... :-))
It is funny.  I don't have a strong preference. One idea is to give it a 
simple name and declare it close to the method so that it is clear that 
this is the only place where it is used (your call).

>
> :
> By using "jmap -histo:live" I'm doing a targeted sample of just
> WeakReference leaks and that's what I want for this regression
> test. If I was creating a general "does the Logging API leak
> memory?" test, then the pure Java test with a small/limited heap
> size would be a good approach.
I understand but just concerned that these multi-process tests have a 
tendency to be problematic.  A simple pure-java test that specifies a 
small maximum heap size in the @run tag may prove to be more robust.

>
> Can I put you down as a "thumbs up"? 
Yes, I'm okay with what you have assuming that it doesn't impact AWT 
startup.  Also Éamonn makes a good point (so maybe create a bug so that 
they code can be replaced/rewritten in jdk7?).

-Alan.