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

[Jeremy Manson]

Okay.  It sounds as if the changes were helpful, anyway.  I'll be
interested to see what you do.

Jeremy

On Mon, Jun 14, 2010 at 4:20 PM, Daniel D. Daugherty
<daniel.daugherty at oracle.com> wrote:
> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>
>>
>