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