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

[Jeremy Manson]

Daniel,

We're happy to contribute.  Like you, we had a customer complaint,
which is why this happened.

My suspicion is that we don't have access to the VM/NSK test suite.
Feel free to run the patch against it.

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