On 11/03/17 09:48, Peter Levart wrote:
Hi Sherman,
I think this looks good now. Thanks.
Regards, Peter
Just one more thing. Re-reading the code once more after a few days made
me re-think about why in my last version I did what I did with
CleanableResource.inflaters field. In CleanableResource.run() I reset it
to null after 1st ending all cached inflaters. This makes
ZipFileInflaterInputStream(s) that are cleaned after the ZipFile has
been cleaned, release the inflaters immediately and not in next GC round:
623 void releaseInflater(Inflater inf) {
624 Deque<Inflater> inflaters = this.inflaters;
625 if (inflaters != null) {
626 synchronized (inflaters) {
627 // double checked!
628 if (this.inflaters == inflaters) {
629 inf.reset();
630 inflaters.add(inf);
631 return;
632 }
633 }
634 }
635 // inflaters cache already closed - just end late comers
636 inf.end();
637 }
639 public void run() {
640 // Release cached inflaters and close inflaters cache 1st
641 Deque<Inflater> inflaters = this.inflaters;
642 if (inflaters != null) {
643 synchronized (inflaters) {
644 // no need to double-check as only one thread
gets a chance
645 // to execute run() (Cleaner guarantee)...
646 Inflater inf;
647 while ((inf = inflaters.poll()) != null) {
648 inf.end();
649 }
650 // close inflaters cache
651 this.inflaters = null;
652 }
653 }
For example, in the following code:
{
ZipFile zf = ...
ZipEntry ze = zf.getEntry(...);
InputStream is = zf.getInputStream(ze);
...
}
// 1st GC round
...we want all native resources to be released in 1st GC round after
'zf' and 'is' become unreachable.
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
