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 <[email protected]> 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 >> <[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 >>>>> >>>>> >>>>> >>>>> >> >> >
