Hi Mark,
Thanks for the review and comments.
On 10/12/2015 3:30 PM, mark.reinh...@oracle.com wrote:
2015/10/8 1:41 -0700, roger.ri...@oracle.com:
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-cleaner-extensible-8138696/
javadoc:
http://cr.openjdk.java.net/~rriggs/cleaner-doc2/
Roger -- thanks for taking this on. The more we can do to get people
to stop using sun.misc APIs, the better.
A couple of comments/questions:
First, I think the approach in your first version is, well, cleaner.
The additional abstract classes in the second version look like a
classic case of implementation inheritance that's not subtype
inheritance, what with the overrides of the original enqueue and
isEnqueued methods to throw UnsupportedOperationException.
The simple use is preferred for most uses and is present using only the
Cleaner.
Collapsing into a single instance provided some benefits in the size
too, fewer object
headers and references between them.
It seemed safer for evolutionary purposes to expose only the necessary
Cleanup
functions and hide or disable the rest.
I understand the desire to avoid allocating too many objects, but
do we have actual use cases where that's critical? The original
sun.misc.Cleaner has been used by both internal and external code
to do relatively heavy-weight things like unmap direct buffers and
release other kinds of native resources, which suggests that
allocating three small objects per cleaner is not a problem.
The reduction in the number of objects was an opportunity to look
closely at what
was needed and the use cases where it would be applied.
Peter's email gives a few detailed cases where this would be used.
When considering how to update the existing classes with finalize
methods, it became clear that
refactoring would be needed to separate out the state needed for the
cleanup from public object
that was being finalized into a separate object it would provide the
cleanup behavior as well.
In simple cases, it is just a copy of the native address or file
descriptor. In other cases, like
FileIn/OutputStream it is references to other objects. In cases like
FileDescriptor and Zip streams
the cleanup state is not just a native resource.
The abstract XXXCleanup types make it easy to encapsulate the needed
behavior and state.
Second, the original sun.misc.Cleaner only handles phantom references.
What are the use cases for weak and soft cleaners?
WeakReferences are sometimes used but since it is extra coding to clean
them it tends
not to be implemented.
For example, sun.beans.WeakCache; it was just not worth the code to
handle the weak refs completely.
In another case, the ldap connection pool only cleans up connections
when opening
a new pooled connection.
Having easy to use and more active cleanup would release resources
sooner and
reduce overall resource requirements.
Finally, how important is it to be able to unregister a cleaner? In
all the years we've had sun.misc.Cleaner that capability has never
been needed, and leaving it out would simplify the API.
Recently there was a long discussion on core-libs [1] about making the
cleanup of MappedByteBuffers
more timely to free up the memory sooner. If there was an explicit
release operation on
either of those, then the cleaner would want to be unregistered or it
would need to be at-most-once.
Encapsulating the cleanup state and behavior allows it to be invoked
consistently both when the
object is closed or is found to be unreachable. The cleanup code is not
repeated, and it is
easy to ensure that is invoked at most once.
The other important aspect is that when the cleanup is performed
explicitly (on close)
the ref is cleared and the overhead associated with reference processing
is eliminated.
Currently, with finalizers, the finalizable refs still have to be
processed, even if object has been closed.
Thanks, Roger
[1]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035055.html
- Mark
