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

[Peter Levart]

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