On 8 Jan 2016, at 19:57, Roger Riggs <roger.ri...@oracle.com> wrote: > Hi Chris, > > Updated webrev: > http://cr.openjdk.java.net/~rriggs/webrev-cleaning-factory-8146028/
Thanks Roger. This is starting to look like a nice, and useful, package ;-) -Chris. > On 1/8/2016 11:08 AM, Chris Hegarty wrote: >> Hi Roger, >> >> On 08/01/16 15:51, Roger Riggs wrote: >>> Thanks for the review and comments: >>> >>> While integrating the comments, some comments identified finalizers that >>> were unneeded should be removed. >>> I split that into a separate bug, as they are not being replaced, just >>> removed. [2] >>> >>> The CleaningFactory is moved to jdk.internal.ref so it can be exported >>> selectively to specific >>> OpenJDK modules. The suggested refactoring of the subclassable >>> XXXCleanables can be done separately >>> if it turns out to be useful outside the base module. >>> >>> Webrev for shared CleaningFactory and its uses: >>> http://cr.openjdk.java.net/~rriggs/webrev-cleaning-factory-8146028/ >> >> Thanks for moving this into its own package. Does is make sense to >> move CleanerImpl? Not for it to be used by other modules, just >> logically seems to belong there. > Ok, moved CleanerImpl to jdk.internal.ref; in the the process refactored it > to expose fewer > of the implementation methods as public to avoid temptations of misuse. > The XXXCleanable classes are top level classes and not nested in CleanerImpl. >> >> CleanerTest should have its @modules tag updated to include >> java.base/jdk.internal.ref. > Added >> >> Trivially, "Cleaner for use within the java.base module" -> >> 'OpenJDK modules'. > Fixed > > Thanks, Roger > >> >> Otherwise, looks good to me. >> >> Thanks, >> -Chris. >> >>> Webrev for removed finalizers: >>> http://cr.openjdk.java.net/~rriggs/webrev-rmfinalizers-8146567/ >>> >>> Thanks, Roger >>> >>> >>> [1] https://bugs.openjdk.java.net/browse/JDK-8146028 Common Cleaner for >>> finalization replacements in OpenJDK >>> [2] https://bugs.openjdk.java.net/browse/JDK-8146567 Remove dead code >>> finalizer methods >>> >>> On 1/6/2016 2:02 PM, Peter Levart wrote: >>>> >>>> >>>> On 01/05/2016 10:16 PM, Roger Riggs wrote: >>>>> Hi Daniel, >>>>> >>>>> webrev updated to revert changes to MeteredStream as too risky. >>>>> >>>>> http://cr.openjdk.java.net/~rriggs/webrev-cleaning-finalizers/index.html >>>> >>>> >>>> Hi Roger, >>>> >>>> I briefly skimmed over the webrev, and found the following issue in >>>> ProcessImpl: >>>> >>>> 420 // Register a cleaning function to close the handle >>>> 421 CleanerFactory.getCleaner().register(this, () -> >>>> closeHandle(handle)); >>>> 422 >>>> >>>> ... 'handle' is an instance variable, which means that 'this' is >>>> captured by the lambda. You will have to assign handle to a local var >>>> and capture it instead to prevent 'this' to be captured. >>>> >>>> Regards, Peter >>>> >>>> >>>>> >>>>> On 1/5/2016 1:45 PM, Daniel Fuchs wrote: >>>>>> Hi Roger, >>>>>> >>>>>> Some early feedback: >>>>>> >>>>>> I see that prior to your changes, MeteredStream.close() would >>>>>> be called by finalize. >>>>>> This will no longer be the case after >>>>>> http://cr.openjdk.java.net/~rriggs/webrev-cleaning-finalizers/index.html >>>>>> >>>>> In the case where finalize would call close(), there can be no >>>>> queuedForCleanup activity >>>>> because there can be no strong reference from the >>>>> KeepAliveStreamCleaner thread >>>>> or the queue that holds the pending cleanups. >>>>> It cannot be in the Cache of connections being kept alive or the >>>>> thread that keeps them alive. >>>>> >>>>> It might be possible that the underlying connection is still open; >>>>> but there is no advantage >>>>> of trying to drain it. There is the possibility of resurrecting it >>>>> by virtue of deciding that >>>>> it should be queued for cleanup and starting a new thread to do the >>>>> cleanup. >>>>> >>>>> All in all, there is a risk of misunderstanding the dynamic behavior >>>>> and it would be safer >>>>> to leave this using finalize. >>>>> >>>>>> >>>>>> I see that MeteredStream has a subclass (KeepAliveStream) that >>>>>> overrides close() - so KeepAliveStream probably requires a cleanup >>>>>> as well - doesn't it? >>>>>> >>>>>> Finally I'd suggest making the variable 'pi' final - since you're >>>>>> passing that to the lambda you don't want to allow its value to >>>>>> be modified afterwards (fortunately it's not - and I believe it >>>>>> should be good to ensure it won't be). >>>>> ok, it would not go unnoticed, the compiler should complain if the >>>>> variable is no effectively final. >>>>> >>>>> Thanks, Roger >>>>> >>>>>> >>>>>> I haven't looked at the other classes yet... >>>>>> >>>>>> best regards, >>>>>> >>>>>> -- daniel >>>>>> >>>>>> On 05/01/16 19:24, Roger Riggs wrote: >>>>>>> The follow on work to adding the Cleaner is to replace uses of >>>>>>> finalization with uses of the Cleaner. >>>>>>> For the 'easy' cases in the java.base module, it is useful to >>>>>>> introduce >>>>>>> a private Cleaner for the >>>>>>> java.base module. It is proposed to be held weakly, to allow it to >>>>>>> terminate on a lightly loaded >>>>>>> system. >>>>>>> >>>>>>> Webrev for Review: >>>>>>> http://cr.openjdk.java.net/~rriggs/webrev-cleaning-factory-8146028/ >>>>>>> >>>>>>> >>>>>>> The 2nd step is using the Cleaner. >>>>>>> - Empty finalize methods should (I think) be removed; but since they >>>>>>> are part of the public spec >>>>>>> the process needs two full releases; so the proposal is to >>>>>>> deprecate >>>>>>> them first. >>>>>>> (The JEP 277 necessary changes will be updated when JEP 277 >>>>>>> semantics are finalized) >>>>>>> >>>>>>> - In a few cases, the code in the finalizer is redundant and if >>>>>>> removed >>>>>>> would allow >>>>>>> an optimization of the handling of the finalizer and future removal >>>>>>> of the finalize method. >>>>>>> >>>>>>> Webrev: >>>>>>> http://cr.openjdk.java.net/~rriggs/webrev-cleaning-finalizers/index.html >>>>>>> >>>>>>> >>>>>>> >>>>>>> Thanks for comments and suggestions, Roger
