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

[Jeremy Manson]

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.

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.

We also ran jtreg, which didn't seem to have any problems.

The doc fixes: Our secret goal was to sneak those in without having to
file a separate bug, but I guess you caught us. ;)

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