Hi Roger, Mandy,
Here's another webrev:
http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.05/
I renamed the method and reworded the specification. Is this better now?
On 02/22/2016 10:56 PM, Roger Riggs wrote:
Hi Mandy,
On 2/22/2016 4:41 PM, Mandy Chung wrote:
The existing way to do that is to register phantom references in your
own ReferenceQueue and then drain the queue at appropriate point.
Would you consider having a method to return ReferenceQueue
maintained by the cleaner instead?
If the queue is exposed, there is no assurance that the cleanable
function would be called.
Any caller could introduce a bug by not doing the proper cleaning.
I was more concerned with the crossing of Reference.tryHandlePending
with the cleaning thread.
The method description does not mention anything related to the
Reference processing thread
though that is all implementation. The @implNote might be a bit more
concise and less informal.
Roger
Yes, ReferenceHandler thread is just an implementation detail. The
specification of java.lang.Reference subclasses doesn't even mention it.
It talks about GC enqueueing Reference objects:
"Suppose that the garbage collector determines at a certain point in time...
...At the same time or at some later time it will enqueue those
newly-cleared weak references that are registered with reference queues."
So in this respect ReferenceHandler is just an extension of GC Reference
discovery/enqueuing logic, delegated to a background thread on Java
side. The Cleaner.cleanNextPending() method tries to be true to its
specification - it tries to invoke next cleanup action for which GC has
determined that its monitored object is phantom reachable without
waiting for ReferenceHandler thread to transfer it from pending list to
the queue.
Since Reference.tryHandlePending is just transfering Reference objects
from one list to another and does that using two locks (the
Reference.lock and the ReferenceQueue.lock) but never holds them both
together or calls any outside code while holding any of the locks,
there's no danger of dead-locking, if that was your concern.
Regards, Peter
Other than this new method, the change looks good to me.
Mandy
I think this form of method that just does one quantum of work and
returns a boolean indicating whether there's more work waiting is a
better fit for some clients that might want to do just a limited
amount of work at once (like for example
sun.java2d.Disposer.pollRemove that you mentioned). This method also
deals with helping the ReferenceHandler thread, which is necessary
to be able to "squeeze" out all outstanding work. As Cleaner is in
the same package as Reference and helping ReferenceHandler thread is
implicitly included in Cleaner.helpClean(), the JavaLangRefAccess
interface and a field in SharedSecrets can be removed.
I also simplified the API in sun.nio.fs.NativeBuffer and replaced
the accessor of Cleanable with a simple void free() method (called
free because it deallocates memory).
I think this will have to be submitted to CCC for approval, right?
Can you help me with it?
Regards, Peter
On 02/17/2016 11:34 PM, Kim Barrett wrote:
On Feb 17, 2016, at 4:06 AM, Peter Levart <peter.lev...@gmail.com>
wrote:
Hi Kim,
Thanks for looking into this. Answers inline…
Peter,
Thanks for the explanations.
On 02/17/2016 01:20 AM, Kim Barrett wrote:
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).
That looks familiar. And yes, I see what you did there, and I don't
think Roger's initial prototype testing did anything similar, so
indeed this is likely the failure he encountered.
Though I'm still inclined to object to that form of drainQueue (see
below).
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.
java.desktop:sun.java2d.Disposer.pollRemove seems to me to be
addressing an essentially similar requirement, with
java.desktop:sun.font.StrikeCache being the user of that API.
Of course, that's already got all the scaffolding for using phantom
references, and there's no need to rewrite it to use
java.lang.ref.Cleaner. But maybe there are others like this?
In any case, I really dislike the approach of exposing the CleanerImpl
object to get at this functionality.
Some of the other changes here don't seem related to the problem at
hand. ...
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.
Oh, right! Bootstrapping issues!
The alternative to CleanerCleanable is a no-op Runnable
implementation passed to PhantomCleanableRef constructor. …
OK. Now I understand.
Making CleanerImpl implement Runnable …
I'd be fine with this if the CleanerImpl wasn't exposed as part of the
drainQueue functionality.
------------------------------------------------------------------------------
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().
So this could be replaced with
void clean() {
cleanable.clean();
}
To me, that seems better.
Similarly for the DirectBuffer interface.
This one is a critical method, used by various 3rd party softwares...
I want to cover my ears when people start talking about some of the
things that have done… OK.
