Cleaner cleanup

[Peter Levart]

Correction in line...


On 05/16/2016 12:54 PM, Peter Levart wrote:
> Hi Christoph,
>
>
> On 05/16/2016 10:37 AM, Christoph Engelbert wrote:
>> Hey Peter,
>>
>> First of all, I really love this Cleanup API!
>> Anyhow I think it would make sense to have Cleanable extend 
>> AutoCloseable for exactly the use case you rendered in your JavaDoc 
>> of the Cleaner class.
>>
>> Made up example:
>> public <T> T execute(BufferAction bufferAction) {
>>    Buffer buffer = allocateBuffer();
>>    try (Cleanable wrapper = cleaner.register(buffer, 
>> Buffer::deallocate) {
>>      bufferAction.execute(buffer);
>>    }
>> }
>>
>> I agree that you probably want to encapsulate the cleaner 
>> registration and cleaning action but for frameworks it might be 
>> beneficial to have a common utility method implementing a pattern 
>> such as the one above.
>>
>> Chris
>
> I think the Cleaner API is more suitable to be used inside various 
> classes that themselves implement AutoCloseable rather than as an 
> (optional) aspect on top of them. So your example would simply read:
>
> public <T> T execute(BufferAction bufferAction) {
>   try (Buffer buffer = allocateBuffer()) {
>     bufferAction.execute(buffer);
>   }
> }
>
> There's no point in doing both things as in your example: using 
> try-with-resources + the Cleaner API on top. try-with-resources 
> already guarantees cleanup. 

Sorry, the following:
> *Cleanable* API is meant to be used inside otherwise *AutoCleanable* 
> implementations to shield the user from native resource leaks if the 
> user forgets to use or can't use try-with-resources.

Should read:

*Cleaner* API is meant to be used inside otherwise *AutoCloseable* 
implementations to shield the user from native resource leaks if the 
user forgets to use or can't use try-with-resources.

>
> Regards, Peter
>
>>
>>> On 16 May 2016, at 00:08, Peter Levart <peter.levart at gmail.com> wrote:
>>>
>>> Hi Roger and others,
>>>
>>> When the new Cleaner API was created the implementation of 
>>> Cleanable(s) was split into the low-level abstract 
>>> [Soft|Weak|Phantom]Cleanable classes to be used internally for 
>>> purposes where the footprint matters and their corresponding 
>>> CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses used as 
>>> implementations that take a Runnable cleanup action and are exposed 
>>> via the public Cleaner API.
>>>
>>> When thinking of possible JDK internal use cases for the low-level 
>>> API, I came to the conclusion that [Soft|Weak|Phantom]Cleanable 
>>> classes are not suitable as is, because in cases where footprint 
>>> matters, it is usually also the case that the number of 
>>> [Soft|Weak|Phantom]Cleanable instances created is larger and that 
>>> construction performance also matters. Especially multi-threaded 
>>> construction. I'm thinking of the use cases of auto-cleanable 
>>> concurrent data structures. In such use cases, the present features 
>>> of [Soft|Weak|Phantom]Cleanable classes, namely the guaranteed 
>>> just-once cleanup action invocation and keeping the Cleanable 
>>> instance reachable until the cleanup action is performed, are 
>>> actually not needed and just present footprint and performance 
>>> (contention) overhead. They also present an overhead as they don't 
>>> allow GC to automatically collect the Cleanable instances if the 
>>> data structure containing them becomes unreachable and corresponding 
>>> registered cleanup actions obsolete.
>>>
>>> The mentioned features are important for public Cleaner.Cleanable 
>>> instances as they are usually used for cleanup of native resources 
>>> where the performance of their creation is not so drastically 
>>> important and where there is no intrinsic data structure to hold 
>>> them reachable.
>>>
>>> I propose to move those features from the 
>>> [Soft|Weak|Phantom]Cleanable classes down the hierarchy to the 
>>> CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses:
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/Cleaner.cleanup/webrev.01/
>>>
>>>
>>> In this change I also removed the 
>>> CleanerImpl.[Soft|Weak]CleanableRef subclasses as they are not 
>>> needed and I believe will never be. I also renamed the 
>>> CleanerImpl.PhantomCleanableRef subclass to 
>>> CleanerImpl.PhantomCleanableImpl.
>>>
>>> Changes to the implementation are straightforward. The most work was 
>>> put into the corresponding test. I did some clean-up to it and also 
>>> changed it to accommodate for the new behavior of 
>>> [Soft|Weak|Phantom]Cleanable classes. The changes speak for itself. 
>>> One of the not-so obvious changes was to replace the 
>>> CleanableCase.clearRef() action with the 
>>> CleanableCase.releaseReferent() action. The old clearRef() action 
>>> did not serve any purpose. Whether this method was called or not, 
>>> the behavior of the corresponding Cleanable was unchanged as the 
>>> Reference instance (referenced from the 'ref' field) was always of 
>>> the same strength as the Cleanable itself. So clearing it could not 
>>> affect the behavior of the Cleanable.
>>>
>>> I changed 'ref' to hold a direct reference to the referent and 
>>> renamed the field to 'referent'. I changed the EV_XXX int constants 
>>> to Event enum constants with helper methods used in 
>>> CleanableCase.expectedCleanups() method that now returns the number 
>>> of expected cleanup invocations - in the PhantomCleanableImpl case 
>>> this is the number of expected cleanup action invocations while in 
>>> the plain XxxCleanable subclass cases it is the number of 
>>> Cleanable.clean() method invocations. I added the no-actions case to 
>>> both PhantomCleanableImpl and XxxCleanable cases and extended the 
>>> number and combinations of XxxCleanable cases.
>>>
>>> The checkCleaned() method was extended to verify that the number of 
>>> cleanup invocations is *no more* and no less then the expected.
>>>
>>> See how WeakKey test is now simplified. This is the typical use-case 
>>> for WeakCleanable I was talking about.
>>>
>>>
>>> So, what do you think of this cleanup?
>>>
>>>
>>> Regards, Peter
>>>
>
>