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.lev...@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
