> 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.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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?
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.
------------------------------------------------------------------------------
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()?
Similarly for the DirectBuffer interface.
------------------------------------------------------------------------------
