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

[Roger Riggs]

Hi Mandy,

On 2/22/2016 4:41 PM, Mandy Chung wrote:
>> On Feb 21, 2016, at 10:55 AM, Peter Levart <peter.levart at gmail.com> wrote:
>>

>> I added new method to Cleaner:
>>
>>     public boolean helpClean() { }
>>
> I’m in two minds of making this a public method. An explicit way to enqueue pending references as well as invoke Cleanable::clean.  If it goes in, the method name needs to be renamed.
I would suggest a name related to cleaning a single Cleanable.
>
> 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

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