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

[Peter Levart]

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
>>>
>>
>