Hi Sherman,
I think this looks good now. Thanks.
Regards, Peter
On 11/01/2017 08:17 PM, Xueming Shen wrote:
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
