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

Eamonn McManus Eamonn.McManus at Sun.COM
Fri Jun 11 09:36:49 PDT 2010


I think an alternative approach to the one here would be to use a global 
ReferenceQueue and a subclass of WeakReference that has a pointer back 
to the Logger or LogNode that contains this WeakReference. Then, in the 
cases where you currently check for stale entries, you could simply poll 
the ReferenceQueue and visit any Logger or LogNode that it is telling 
you has a cleared WeakReference.
There are a couple of subtle points, though. First, you'll need to be 
careful about synchronization before clearing WeakReferences, since the 
Logger you're being told about isn't necessarily related to the current 
one. Logger.kids is protected by a global lock treeLock but 
LogManager.loggers is protected by the LogManager's lock, so when 
logManager1.addLogger detects that a logger belonging to logManager2 has 
a stale WeakReference, it needs to drop the logManager1 lock and acquire 
the logManager2 one before scanning the reference list. (This means that 
addLogger has to change from being a synchronized method to have a big 
synchronized(this) block with a non-synchronized tail-end.)
Second, since apparently the user can introduce loops in the Logger 
hierarchy, the reference from the WeakReference subclass back to the 
parent Logger will itself need to be a WeakReference.
Nevertheless, I think this approach is likely to be less complicated 
than the proposed one, notably because there's no need to protect 
against infinite recursion; and the performance concerns should be less 
because you only look for cleared WeakReferences when you know they are 
there, and you know where to look.
Éamonn

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.
>
> I agree with the performance concern with expunging stale entries each 
> time a LogManager is created. 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).
>
> Does LogManager.deleteStaleWeakRefs need to be synchronized with any 
> other operations?
>
> 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"?
>
> 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.
>
> -Alan.
>
>
>
>


More information about the hotspot-runtime-dev mailing list