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

[Daniel D. Daugherty]

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