RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Xueming Shen xueming.shen at oracle.com
Wed Nov 1 19:17:39 UTC 2017

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 
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 
method for compatibility reason. So not done yet :-)



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

More information about the core-libs-dev mailing list