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

[Daniel D. Daugherty]

On 6/14/2010 12:36 PM, Jeremy Manson wrote:
> Daniel,
>
> We're happy to contribute.  Like you, we had a customer complaint,
> which is why this happened.
>   

And I see that you did this work against an earlier bug. I've
made myself the RE for 6931561 and I've update the evaluation
to indicate that I'm likely to close it as a dup of the escalated
bug (6942989).


> My suspicion is that we don't have access to the VM/NSK test suite.
>   

You're right. I think that is currently an internal test suite.


> Feel free to run the patch against it.
>   

I've just finished my code review of the patch and I have some
issues with it. In the interests of making forward progress on my
escalation, I'm going to investigate creating a hybrid solution
of some of your changes, some of Eamonn's ideas about creating
WeakReference subclasses, and Tony P's ideas in the following
http://java.sun.com/developer/technicalArticles/javase/finalization/

In particular, Tony's article points out that a strong reference
to the WeakReference subclass needs to be held to make sure that
the WeakReference subclass object is still around when we want to
process it off the ReferenceQueue. Of course, that assumes I
understood what Tony said :-)

Dan



> Jeremy
>
> On Mon, Jun 14, 2010 at 10:45 AM, Daniel D. Daugherty
> <daniel.daugherty at oracle.com> wrote:
>   
>> On 6/14/2010 11:30 AM, Jeremy Manson wrote:
>>     
>>> Daniel,
>>>
>>> The fix hasn't made it to OpenJDK6.  We were planning on pushing it to
>>> OpenJDK6/7, but we haven't had time for it yet.  If your fix is better
>>> (I haven't had a chance to look at it), then we'll happily drop ours
>>> in favor of yours.
>>>
>>>       
>> I will be looking at your fix today. Mine is a brute force big hammer
>> style fix that I'm not fond of, but does the job. Your fix will likely
>> address Eamonn's review comments and also solve the potential performance
>> impact that mine would have in systems with lots of Loggers.
>>
>>     
>>> For testing: I hand tested it with the "create lots of anonymous
>>> loggers" test.  My major observation was that creating that many weak
>>> references and rebuilding the internal data structures from scratch
>>> repeatedly can bring a system to its knees.  This is why the
>>> weakReferencesProcessed variable exists - we don't rebuild the
>>> internal data structures with every additional logger that we lose.
>>>
>>>       
>> I'll be sure to keep my eyes open for that part of the fix.
>>
>>
>>     
>>> We also ran jtreg, which didn't seem to have any problems.
>>>
>>>       
>> There are only 10 or so tests in JTREG. The VM/NSK logging test suite
>> has 550+ tests and it caught my breakage due to the currently legal
>> Logger loop in one of the tests.
>>
>>
>>     
>>> The doc fixes: Our secret goal was to sneak those in without having to
>>> file a separate bug, but I guess you caught us. ;)
>>>
>>>       
>> Perhaps I can sweep those up for you. We'll have to see how it goes.
>>
>> Thanks again for contributing the fix!
>>
>> Dan
>>
>>
>>     
>>> Jeremy
>>>
>>> On Mon, Jun 14, 2010 at 9:46 AM, Daniel D. Daugherty
>>> <daniel.daugherty at oracle.com> wrote:
>>>
>>>       
>>>> On 6/11/2010 4:41 PM, Martin Buchholz wrote:
>>>>
>>>>         
>>>>> On Fri, Jun 11, 2010 at 14:46, Daniel D. Daugherty
>>>>> <daniel.daugherty at oracle.com> wrote:
>>>>>
>>>>>
>>>>>           
>>>>>> Jeremy,
>>>>>>
>>>>>> I'm definitely interested in learning about your approach to this
>>>>>> issue.
>>>>>>
>>>>>>
>>>>>>             
>>>>> Here's the patch against openjdk6 by Jeremy.
>>>>> http://cr.openjdk.java.net/~martin/WeakLogger-jeremymanson.patch
>>>>> (It would take a bit of merging to port to openjdk7)
>>>>>
>>>>> Feel free to take anything from our change.
>>>>> Apologies for the perforce-isms.
>>>>>
>>>>> Martin
>>>>>
>>>>>
>>>>>           
>>>> Jeremy and Martin,
>>>>
>>>> Thanks for the proposed fix. A couple of questions:
>>>>
>>>> - This changeset is private to Google right now, correct? As in
>>>>  it hasn't made it into OpenJDK6 yet.
>>>> - Do you plan on pushing this changeset to OpenJDK6?
>>>> - What kind of testing has been done on it?
>>>>
>>>> Thanks for the offer for the code. I'll start wading through the
>>>> diffs today. Because this is an escalated issue, I will likely
>>>> be taking just the code and comments directly related to the
>>>> problem at hand. The JavaDoc fixes, even though they are useful,
>>>> will have to wait for a different changeset.
>>>>
>>>> Thanks for jumping in on this thread.
>>>>
>>>> Dan
>>>>
>>>>
>>>>
>>>>         
>
>