[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Code Review for WeakReference leak in the Logging API (6942989)
From:       daniel.daugherty () oracle ! com (Daniel D !  Daugherty)
Date:       2010-06-16 4:27:04
Message-ID: 4C185298.8040409 () oracle ! com
[Download RAW message or body]

Revisiting this e-mail now that I've completed a first pass
at a ReferenceQueue implementation.

On 6/11/2010 10:36 AM, Eamonn McManus wrote:
> 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.

The WeakReference subclass that I created manages weaks refs
in LogManager.loggers (now named LogManager.namedLoggers),
LogManager.LogNode.loggerRef and Logger.kids. I got the idea
of managing LogManager.loggers from Jeremy's fix.


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

I created a LogManager.drainLoggerRefQueueBounded() method based on
Tony's paper:

http://java.sun.com/developer/technicalArticles/javase/finalization/

and I'm calling it on the LogManager.addLogger() and the
Logger.getAnonymousLogger(String) code paths.

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

There is only one LogManager so I don't have to worry about redoing
the LogManager synchronization stuff. drainLoggerRefQueueBounded()
isn't called from the context of a Logger object so I don't have to
worry about a current Logger versus other Logger context collision.


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

I did make the parent Logger reference a weak reference, but not
because of loops. I did it because I didn't want to add a strong
reference to the parent Logger and keep it from being GC'ed. Sorry,
I still can't figure out what loops have to do with it.


> Nevertheless, I think this approach is likely to be less complicated 
> than the proposed one,

I'm still not convinced this approach is less complicated, but
it is definitely a much better approach :-)


> notably because there's no need to protect against infinite recursion;

No more need for protecting 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.

Definitely less performance concerns. I also only cleanup a set
maximum of GC'ed Logger objects per call. Tony P's paper also
recommended this and the drain*Bounded() name too.


> ?amonn

Thanks again for the ReferenceQueue idea and for your thorough review.

I'm running my new version through VM/NSK logging tests right now;
it passed the SDK/JDK LOGGING_REGRESSION tests. I'll make one more
sanity check code review after that and send the revised bits out
for a re-review...

Dan


[prev in list] [next in list] [prev in thread] [next in thread]