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.lev...@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
