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
<[email protected]> 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
<[email protected]> wrote:
On 6/11/2010 4:41 PM, Martin Buchholz wrote:
On Fri, Jun 11, 2010 at 14:46, Daniel D. Daugherty
<[email protected]> 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
