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