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

Daniel D. Daugherty daniel.daugherty at oracle.com
Fri Jun 11 07:45:34 PDT 2010


On 6/11/2010 7:18 AM, Alan Bateman wrote:
> Daniel D. Daugherty wrote:
>> :
>>
>> Here is the URL for the webrev:
>>
>>    http://cr.openjdk.java.net/~dcubed/6942989-webrev/0/
>>
> I went through the changes too.

Thanks!


> I agree with the performance concern with expunging stale entries each 
> time a LogManager is created.

I think you mean Logger object here. Typically, there is only one
LogManager...


> Would it make sense to do this once every N times rather than every 
> time? Or maybe keep a timestamp so that you limit the number of times 
> your attempt to expunge in some time period? (say a maximum of once a 
> minute).

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.


> Does LogManager.deleteStaleWeakRefs need to be synchronized with any 
> other operations?

That's actually LogManager.LogNode.deleteStaleWeakRefs(). It's
only called from LogManager.addLogger() which is synchronized.
So the LogManager instance should be locked and no other thread
can be adding a Logger at the same time that we're cleaning up
any stale weak refs. Similarly, walkAndSetParent() is only called
from addLogger() so that (opportunistic) cleanup won't conflict
with the more complete deleteStaleWeakRefs().


> I wonder if Logger.kidsCleanupMarker is the best name for this [ kids 
> and markers usually involve clean-up but that's not what is meant here 
> :-)  ]. Maybe rename to something that includes the word "sentinel"?

So would "kidsCleanupSentinel" be better? (I actually rather liked
the implied humor of kids and markers... :-))


> Is is necessary to use jmap in the regression test? Would it be 
> simpler to have a pure java test that runs with a small heap size? If 
> it leaks then it fails with OOME.

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.

Can I put you down as a "thumbs up"? And do you want me to do the
kidsCleanupMarker to kidsCleanupSentinel rename?

Dan

P.S.
The built-in spellchecker wants to rename you to "Batman". :-)


More information about the hotspot-runtime-dev mailing list