> On Feb 22, 2016, at 1:56 PM, Roger Riggs <roger.ri...@oracle.com> wrote:
>
> Hi Mandy,
>
> On 2/22/2016 4:41 PM, Mandy Chung wrote:
>>> On Feb 21, 2016, at 10:55 AM, Peter Levart <peter.lev...@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.
>
I agree the spec needs some work if it’s a public method. This seems better to
file a RFE to follow up.
Mandy
> 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.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.
>>>>
>>>>
>>>>
>>>>
>
