RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

Roger Riggs Roger.Riggs at Oracle.com
Thu Feb 18 21:13:53 UTC 2016


Hi Peter,

Nice to see this improvement.

I would support adding the drainQueue method to java.lang.ref.Cleaner; 
it provides
a mechanism to share the cleanup load (as in the case of Direct buffers) 
that may be useful
to other use cases.  It is preferable to hacking in to the CleanerImpl.

Making CleanerImpl implement Runnable is ok as long as CleanerImpl is 
limited to *only*
be the implementation of Cleaner and is not expected to be used directly 
even internal to the base module.

The conversions of lambdas to classes is necessary, unfortunately.

The names of the Cleaner threads could use the threadId instead of 
introducing a counter,
but probably not a big difference either way.  The numbers are not going 
to be significant.

Roger



On 2/17/2016 4:06 AM, Peter Levart wrote:
> Hi Kim,
>
> Thanks for looking into this. Answers inline...
>
> On 02/17/2016 01:20 AM, Kim Barrett wrote:
>>> On Feb 16, 2016, at 11:15 AM, Peter Levart <peter.levart at gmail.com> 
>>> wrote:
>>>
>>> Hello everybody,
>>>
>>> Thanks for looking into this and for all your comments. Now that it 
>>> seems we have a consensus that replacing internal Cleaner with 
>>> public Cleaner is not a wrong idea, I created an issue for it [1] so 
>>> that we may officially review the implementation. I also created an 
>>> updated webrev [2] which fixes some comments in usages that were not 
>>> correct any more after the change. I also took the liberty to modify 
>>> the CleanerImpl.InnocuousThreadFactory to give names to threads in 
>>> accordance to the style used in some other thread factories 
>>> (Timer-0, Timer-1, ..., Cleaner-0, Cleaner-1, ..., etc.). This gives 
>>> the thread of internal Cleaner instance, which is now constructed as 
>>> part of the boot-up sequence, a stable name: "Cleaner-0".
>>>
>>> If you feel that internal Cleaner instance should have a Thread with 
>>> MAX_PRIORITY, I can incorporate that too.
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8149925
>>> [2] 
>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.03/
>>>
>>> I re-ran the java/lang/ref and java/nio tests and all pass except 
>>> two ignored tests:
>> I think some of the existing uses of sun.misc.Cleaner were really just
>> a matter of convenience and the previous lack of
>> java.lang.ref.Cleaner.  An example of this that I have personal
>> knowledge of the history for is the CallSite cleanup in
>> MethodHandleNatives.  It's good to see those converted.
>>
>> As for the others, if nobody wants to defend the special handling by
>> the reference processing thread, I'm certainly happy to see it
>> removed. However, I recall Roger saying there were existing tests that
>> failed when certain uses of sun.misc.Cleaner were replaced with
>> java.lang.ref.Cleaner. I don't remember the details, but maybe Roger
>> does.
>
> If the failing test was the following:
>
> java/nio/Buffer/DirectBufferAllocTest.java
>
> ...then it has been taken care of (see Bits.java). All java/lang/ref 
> and java/nio tests pass on my PC. Are there any internal Oracle tests 
> that fail?
>
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/java.base/share/classes/java/lang/ref/Reference.java
>>
>> After removal of the special handling of removing Cleaner objects from
>> the pending list, do we still need to catch OOMEs from the pending
>> list?  If not sure, feel free to defer that in another RFE for cleanup.
>
> As you have already noticed it is still possible for a lock.wait() to 
> throw OOME because of not being able to allocate InterruptedException.
>
> The other place where OOME could still be thrown (and has not been 
> fixed yet) is from sun.misc.Cleaner.clean() special handling. I even 
> constructed a reproducer for it (see the last comment in JDK-8066859). 
> So removing special handling of sun.misc.Cleaner would finally put the 
> JDK-8066859 to the rest.
>
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java
>>
>> I don't understand why CleanerImpl needs to be changed to public in
>> order to provide access to the new drainQueue. Wouldn't it be better
>> to add Cleaner.drainQueue?
>
> An interesting idea. But I don't know if such functionality is 
> generally useful enough to commit to it in a public API.
>
> I have another idea where java.lang.ref.Cleaner would use an Executor 
> instead of ThreadFactory. The default would be an instance of a 
> single-threaded executor per Cleaner instance, but if user passed in a 
> ForkJoinPool, he could use it's method ForkJoinPool.awaitQuiescence() 
> to help the executor's thread(s) do the cleaning.
>
>>
>> Some of the other changes here don't seem related to the problem at
>> hand. Are these proposed miscellaneous cleanups? I'm specifically
>> looking at the CleanerCleanable class. And I'm not sure why the
>> thread's constructor argument is being changed, requiring CleanerImpl
>> to implement Runnable.
>
> One thing that this change unfortunately requires is to get rid of 
> lambdas and method references in the implementation and dependencies 
> of java.land.ref.Cleaner API, because it gets used early in the 
> boot-up sequence when java.lang.invoke infrastructure is not ready yet.
>
> The alternative to CleanerCleanable is a no-op Runnable implementation 
> passed to PhantomCleanableRef constructor. I opted for a subclass of 
> abstract PhantomCleanable class with an empty performCleanup() method. 
> Both approaches require a new class, but the CleanerCleanable is a 
> more direct way to express the intent.
>
> Making CleanerImpl implement Runnable and consequently making its 
> run() method public is also just a way to avoid introducing another 
> anonymous inner class as an adapter between Runnable.run() and 
> CleanerImpl.run(). CleanerImpl is in an internal concealed package, so 
> there's no danger of abuse. But I can revert it back to using an 
> anonymous inner adapter class (as was done in webrev.02) if desired or 
> I can just add a comment to public CleanerImpl.run() to warn against 
> it's direct use.
>
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/java.base/share/classes/sun/nio/fs/NativeBuffer.java
>>    76     Cleaner.Cleanable cleanable() {
>>    77         return cleanable;
>>    78     }
>>
>> [pre-existing, but if we're changing things anyway...]
>>
>> I'm kind of surprised by an accessor function (both here and in the
>> original code) rather than a cleanup function.  Is there a use case
>> for anything other than buffer.cleanable().clean()?
>
> It can't be, since both old Cleaner and new Cleanable have only got a 
> single method - clean().
>
>>
>> Similarly for the DirectBuffer interface.
>
> This one is a critical method, used by various 3rd party softwares. In 
> order to make it less painful to support the change from 
> sun.misc.Cleaner to java.lang.ref.Cleaner.Cleanable, I think the 
> method should retain its name and semantics - it only changes it's 
> return type from a public type to another public type that both 
> contain a method with same signature - clean(). This way one can 
> construct a simple reflective adapter that is compatible with both JDK 
> 8- and JDK 9+.
>
> Regards, Peter
>




More information about the core-libs-dev mailing list