Hi Peter,
I like the idea of moving get/releaseInflter() into CleanableResource,
though I doubt
how much it can really help the GC it should be a good thing to do to
remove the strong
reference of ZipFile from stream's cleaner, and the code appears a
little cleaner as well.
I was debating with myself whether or not the ZipFile.close() should
throw an
UncheckedIOException (like those java.nio.file.Files methods do). But
you're right it's
not good to simply ignoring them silently. Now I catch/unwarp the potential
UncheckedIOException from res.clean() and re-throw the embedded ioe.
I need to dig a little to recall the real reason of zsrc==null check in
ensureOpen()
the comment does not sound updated. res.zsrc is not final and it is set
to null
when clean up/close. So I keep it for now.
Most (if not all?) "minor nit" has been updated accordingly.
It seems like we might have have put the "finalize()" method back as an
empty-body
method for compatibility reason. So not done yet :-)
http://cr.openjdk.java.net/~sherman/8185582/webrev/
thanks,
sherman
On 11/1/17, 5:04 AM, Peter Levart wrote:
Hi Sherman,
On 11/01/17 00:25, Xueming Shen wrote:
Hi Peter,
After tried couple implementations it seems the most reasonable
approach is to
use the coding pattern you suggested to move all pieces into ZSStream
Ref. Given
we are already using the internal API CleanerFactory it is attractive
to go a little
further to subclass PhantomCleanable directly (in which I would
assume we can
save one extra object), but I think I'd better to follow the
"suggested" clean usage
(to register a runnable into the cleaner) for now.
39 class ZStreamRef implements Runnable {
40
41 private LongConsumer end;
42 private volatile long address;
43 private final Cleanable cleanable;
44
45 ZStreamRef (Object owner, LongSupplier addr, LongConsumer
end) {
46 this.cleanable =
CleanerFactory.cleaner().register(owner, this);
47 this.end = end;
48 this.address = addr.getAsLong();
49 }
50
Similar change has been made for the ZipFile cleaner to follow the
same coding
pattern. The "cleaner" is now renamed from Releaser to
CleanableResource.
http://cr.openjdk.java.net/~sherman/8185582/webrev/
Thanks,
Sherman
This looks mostly fine. One minor nit is that ZStreamRef.address field
does not have to be volatile. I checked all usages of
ZStreamRef.address() and all of them invoke it while holding a lock on
the ZStreamRef instance. The ZStreamRef.run() which modifies the
address is also synchronized. The other minor nit is that the
following ZipFile imports are not needed:
import java.nio.file.Path;
import java.util.Map;
import jdk.internal.misc.JavaIORandomAccessFileAccess;
import static java.util.zip.ZipConstants.*;
(at least my IDEA colors them unused)
Cleaner is modified in this webrev (just one empty line deleted) -
better not touch this file in this changeset
Additional nits in ZipFile:
- in lines 355, 356 you pull out two res fields into locals, but then
not use them consistently (lines 372, 389)
- line 403 has a TAB character (is this OK?) and shows incorrectly
indented in my editor (should I set tab stops differently?)
- line 457 - while should actually be if ?
- in ensureOpen() the check for res.zsrc == null can never succeed
(CleanableResource.zsrc is a final field. If CleanableResource
constructor fails, there's no res object and there's no ZipFile object
either as ZipFile constructor does not do anything for this to escape
prematurely)
- why don't you let IOExceptions thrown from CleanableResource.run()
propagate out of ZipFile.close() ?
I would also rename static method Source.close(Source) to
Source.release(Source) so it would not clash with instance method
Source.close() which makes it ambiguous when one wants to use
Source::close method reference (both methods apply). I would also make
static methods Source.get() and Source.release(Source) package-private
(currently one is public and the other is private, which needs
compiler bridges to be invoked) and both are in a private nested class.
Inflater/Deflater/ZipFile now follow the coding pattern as suggested.
But ZipFileInflaterInputStream still does not. It's not critical since
failing to register cleanup which releases the inflater back into
cache would simply mean that Inflater employs its own cleanup and ends
itself.
And now another thing I would like to discuss. Why an initiative for
using Cleaner instead of finalization()? Among drawbacks finalization
has one of the more troubling is that the tracked referent survives
the GC cycle that initiates its finalization reference processing
pipeline, so the GC may reclaim the object (and referenced objects)
only after the finalize() has finished in yet another GC round.
Cleaner API separates the tracked object from the cleanup function and
state needed to perform it into distinct instances. The tracked object
can be reclaimed and the cleanup reference processing pipeline
initiated in the same GC cycle. More heap may be reclaimed earlier.
Unless we are careful and create a cleaning function for one tracked
object which captures (directly or indirectly) another object which
registers its own cleaning function but we don't deal with explicit
cleaning of the 2nd object in the 1st cleaning function.
Take for example the ZipFileInflaterInputStream's cleaning function.
It captures the ZipFile in order to invoke ZipFile.releaseInflater
instance method. What this means is that ZipFile will be kept
reachable until all ZipFileInflaterInputStream's cleaning functions
have fired. So we are back to the finalization drawback which needs at
least 2 GC cycles to collect and clean-up what might be done in one go.
I suggest moving the getInflater() and releaseInflater() from ZipFile
into the CleanableResource so that ZipFileInflaterInputStream's
cleaning function captures just the CleanableResource and not the
ZipFile. ZipFile therefore may become eligible for cleanup as soon as
all opened input streams become eligible (but their cleaning functions
need not have fired yet). CleanableResource.run() (the ZipFile
cleaning function) and CleanableResource.releaseInflater() (the
ZipFileInflaterInputStream's cleaning function) may therefore be
invoked in arbitrary order (maybe even concurrently if one of them is
explicit cleanup and the other is automatic), so code must be prepared
for that.
I have tried to capture all above in a modified webrev (taking your
last webrev as a base):
http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.03/
Regards, Peter
