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

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

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

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

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

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

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

Hi Peter, The Supplier/Consumer allocator/deallocator version is useful and interesting encapsulation. We should be able to do better than exposing raw native pointers for other resources. They should have better more complete encapsulation including their cleanup. For example, the recent

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

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

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

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

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

Hi Mandy, Sherman, Roger, On 10/31/17 00:25, mandy chung wrote: On 10/30/17 1:49 PM, Peter Levart wrote: ...above example lends itself as a use case for the following equivalent alternative using internal low-level API where ZStreamRef becomes the Cleanable itself: class ZStreamRef

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

Hi Sherman, On 10/31/17 00:32, Xueming Shen wrote: On 10/30/2017 01:34 PM, Peter Levart wrote: The Inflater and Deflater now look fine (except you don't have to check for cleanable != null any more in Inflater.end()). But what shall we do with ZipFile? Peter, The fundamental issue here

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

On 10/30/2017 01:34 PM, Peter Levart wrote: The Inflater and Deflater now look fine (except you don't have to check for cleanable != null any more in Inflater.end()). But what shall we do with ZipFile? Peter, The fundamental issue here is the gap between the resource allocation and the

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

On 10/30/17 1:49 PM, Peter Levart wrote: ...above example lends itself as a use case for the following equivalent alternative using internal low-level API where ZStreamRef becomes the Cleanable itself: class ZStreamRef extends PhantomCleanable {     private final LongConsumer end;    

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

Hi again, On 10/30/17 21:34, Peter Levart wrote: To mimic the finalization registration, it might be a good to encourage the following coding pattern (using Inflater/ZStreamRef just as an example, not suggesting to do that here unless you like it): class ZStreamRef implements Runnable {    

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

Hi Sherman, On 10/30/17 19:45, Xueming Shen wrote: Peter, Given we have to put in those "make it more robust" code in ZipFile to make cleaner work correctly with the zipfile/inflater in those vm error circumstances I would assume it is a wrong design decision to have the short-cut Inflater

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

... long t0 = System.nanoTime(); this.zsrc = Source.get(file, (mode & OPEN_DELETE) != 0); this.streams = Collections.newSetFromMap(new WeakHashMap<> this.inflaterCache = new ArrayDeque<>(); this.cleanable = CleanerFactory.cleaner().register(this, new Releaser(this.streams,

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

Peter, Given we have to put in those "make it more robust" code in ZipFile to make cleaner work correctly with the zipfile/inflater in those vm error circumstances I would assume it is a wrong design decision to have the short-cut Inflater constructor to try to save some runtime circle for

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

* Peter Levart: > Simply saying that "vm is in a more critical status than a inflater is > getting leaked out" is, in my opinion, covering the problem under the > rug. The VM is not in critical state - the program is. VM is robust > enough to recover from OOMEs. The program might be in

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

Hi Sherman, On 10/28/2017 09:01 PM, Xueming Shen wrote: On 10/28/17, 10:47 AM, Peter Levart wrote: Hi Florian, On 10/28/17 16:16, Florian Weimer wrote: * Xueming Shen: https://bugs.openjdk.java.net/browse/JDK-8187485 http://cr.openjdk.java.net/~sherman/8185582/webrev In ZipFile: 387

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

* Xueming Shen: > It might be possible to try-catch to expect Cleaner.register might > fail, but I doubt it's really necessary here and it is the > recommended usage of Cleaner? That is actually why I posted this. 8-) For similar functionality in other languages, it is often necessary to

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

On 10/28/17, 10:47 AM, Peter Levart wrote: Hi Florian, On 10/28/17 16:16, Florian Weimer wrote: * Xueming Shen: https://bugs.openjdk.java.net/browse/JDK-8187485 http://cr.openjdk.java.net/~sherman/8185582/webrev In ZipFile: 387 Inflater inf = getInflater(); 388

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

Hi Florian, On 10/28/17 16:16, Florian Weimer wrote: * Xueming Shen: https://bugs.openjdk.java.net/browse/JDK-8187485 http://cr.openjdk.java.net/~sherman/8185582/webrev In ZipFile: 387 Inflater inf = getInflater(); 388 InputStream is = new

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

On 10/28/17, 7:16 AM, Florian Weimer wrote: * Xueming Shen: I removed the ln#657-#663, and updated the @apiNote in deflter, inflater and zipfile accordingly, mainly combined your comment and the approach for the fis/fo. they are "simpler" and straightforward now, at least for me.

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

* Xueming Shen: > I removed the ln#657-#663, and updated the @apiNote in deflter, inflater > and zipfile accordingly, mainly combined your comment and the approach > for the fis/fo. they are "simpler" and straightforward now, at least for me. > > https://bugs.openjdk.java.net/browse/JDK-8187485 >

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

Hi Sherman, On 10/11/2017 12:22 AM, Xueming Shen wrote: Peter, Webrev has been updated accordingly to remove the "inflater" from the "streams" as the "value" of the map. As suggested, the "streams" now itself is a "set". And, a package private "Infater()" has been added to avoid registering

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

Peter, Webrev has been updated accordingly to remove the "inflater" from the "streams" as the "value" of the map. As suggested, the "streams" now itself is a "set". And, a package private "Infater()" has been added to avoid registering the inflater to Cleaner for those inflaters from ZipFile.

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

Hi Sherman, On 09/30/17 02:02, Xueming Shen wrote: On 9/29/17, 1:18 PM, Peter Levart wrote: Hi Sherman, I looked into ZipFile as promised. One thing I noticed is that FinalizeZipFile.java test fails compilation: test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported

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

On 9/29/17, 1:18 PM, Peter Levart wrote: Hi Sherman, I looked into ZipFile as promised. One thing I noticed is that FinalizeZipFile.java test fails compilation: test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be

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

On 9/29/17, 1:18 PM, Peter Levart wrote: Hi Sherman, putStream> streams = Collections.newSetFromMap(new WeakHashMap<>()); I also noticed that it is useless to test whether the inflater is ended() when obtaining it from or releasing it into cache if the code keeps the invariant that it never

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

Very cool, i learn something today :) Rémi - Mail original - > De: "mandy chung" <mandy.ch...@oracle.com> > À: "core-libs-dev" <core-libs-dev@openjdk.java.net> > Envoyé: Vendredi 29 Septembre 2017 23:45:18 > Objet: Re: RFR JDK-81855

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

On 9/29/17 2:38 PM, Peter Levart wrote: I think the hotspot has an optimization and detects that finalize() has no bytecodes and treats the object as not needing finalization. http://hg.openjdk.java.net/jdk10/master/file/7d67bb6b0599/src/hotspot/share/classfile/classFileParser.cpp#l4250   //

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

On 09/29/17 23:23, Remi Forax wrote: - Mail original - De: "Peter Levart" <peter.lev...@gmail.com> À: "Roger Riggs" <roger.ri...@oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Vendredi 29 Septembre 2017

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

- Mail original - > De: "Peter Levart" <peter.lev...@gmail.com> > À: "Roger Riggs" <roger.ri...@oracle.com>, "core-libs-dev" > <core-libs-dev@openjdk.java.net> > Envoyé: Vendredi 29 Septembre 2017 23:14:33 > Objet

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

On 09/29/17 23:11, Peter Levart wrote: Hi Roger, On 09/29/17 22:55, Roger Riggs wrote: fyi, The proposed[1]  changes to FileInputStream and FileOutputStream remove the finalize method exposing Object.finalize (throws Throwable) to subclasses.  We may need retain the finalize methods (with

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

On 9/29/17, 1:58 PM, mandy chung wrote: On 9/27/17 4:41 PM, Xueming Shen wrote: Thanks Mandy! I removed the ln#657-#663, and updated the @apiNote in deflter, inflater and zipfile accordingly, mainly combined your comment and the approach for the fis/fo. they are "simpler" and straightforward

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

Hi Roger, On 09/29/17 22:55, Roger Riggs wrote: fyi, The proposed[1]  changes to FileInputStream and FileOutputStream remove the finalize method exposing Object.finalize (throws Throwable) to subclasses.  We may need retain the finalize methods (with empty bodies) to mitigate source

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

t;> <xueming.s...@oracle.com> ] >>> , "core-libs-dev" [ mailto:core-libs-dev@openjdk.java.net | >>> <core-libs-dev@openjdk.java.net> ] Envoyé: Vendredi 29 Septembre 2017 >>> 22:34:52 >>> Objet: Re: RFR JDK-8185582, Update Zip implementat

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

On 9/27/17 4:41 PM, Xueming Shen wrote: Thanks Mandy! I removed the ln#657-#663, and updated the @apiNote in deflter, inflater and zipfile accordingly, mainly combined your comment and the approach for the fis/fo. they are "simpler" and straightforward now, at least for me.

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

openjdk.java.net> Envoyé: Vendredi 29 Septembre 2017 22:34:52 Objet: Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers On 9/27/17 2:31 AM, Peter Levart wrote: Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instanc

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

fyi, The proposed[1]  changes to FileInputStream and FileOutputStream remove the finalize method exposing Object.finalize (throws Throwable) to subclasses.  We may need retain the finalize methods (with empty bodies) to mitigate source compatibility. Roger [1]

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

On 9/29/17 1:49 PM, Remi Forax wrote: On 9/27/17 2:31 AM, Peter Levart wrote: Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After that

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

On 09/29/17 22:34, mandy chung wrote: On 9/27/17 2:31 AM, Peter Levart wrote: Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After

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

On 9/29/17 1:49 PM, Xueming Shen wrote: On 9/29/17, 1:18 PM, Peter Levart wrote: Hi Sherman, I looked into ZipFile as promised. One thing I noticed is that FinalizeZipFile.java test fails compilation: test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception

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

nvoyé: Vendredi 29 Septembre 2017 22:34:52 > Objet: Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not > finalizers > On 9/27/17 2:31 AM, Peter Levart wrote: >> >> Up to a point where 'this' is dereferenced to obtain the 'zsRef' value >> (line 261), the De

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

On 9/29/17, 1:18 PM, Peter Levart wrote: Hi Sherman, I looked into ZipFile as promised. One thing I noticed is that FinalizeZipFile.java test fails compilation: test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be

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

On 9/27/17 2:31 AM, Peter Levart wrote: Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After that point, obtaining zsRef.address() and

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

Hi Sherman, I looked into ZipFile as promised. One thing I noticed is that FinalizeZipFile.java test fails compilation: test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be thrown super.finalize();

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

Thanks Mandy! I removed the ln#657-#663, and updated the @apiNote in deflter, inflater and zipfile accordingly, mainly combined your comment and the approach for the fis/fo. they are "simpler" and straightforward now, at least for me. https://bugs.openjdk.java.net/browse/JDK-8187485

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

Comment on the CSR: On 9/26/17 11:35 PM, Xueming Shen wrote: csr:   https://bugs.openjdk.java.net/browse/JDK-8187485 I think the @apiNote can be simpler. Deflater (similar comment for Inflater) |* @apiNote * In earlier versions, the {@link Object#finalize} method was overridden * to

Re: Phantom Referencesvs finalizers (was: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers)

> On Sep 27, 2017, at 1:55 PM, Hans Boehm wrote: > > "And of course, it guarantees that the tracked referent can not be > resurrected as a result of cleanup code execution." > > True, but it seems to me that the real property you want is "it guarantees > that the tracked

Re: Phantom Referencesvs finalizers (was: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers)

On 9/27/17 10:55 AM, Hans Boehm wrote: "And of course, it guarantees that the tracked referent can not be resurrected as a result of cleanup code execution." True, but it seems to me that the real property you want is "it guarantees that the tracked referent can not be resurrected". Full stop.

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

Hi Peter, Sorry, I might not understand your use scenario correctly. Let me try :-) If clean() is invoked via Deflater.end() first, my reading of the Cleaner code suggests that the clean()->ZStreamRef.run() is run by the thread calling deflater.end() directly, not the Cleaner thread. In this

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

Hi David, On 09/27/2017 11:52 AM, David Holmes wrote: Hi Peter, I thought Cleaner was supposed to avoid all these unpredictable reachability races? Otherwise why switch from using a finalizer ?? Unfortunately, there is no hidden magic in Cleaner. It is better than finalize() mostly because

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

Hi Peter, I thought Cleaner was supposed to avoid all these unpredictable reachability races? Otherwise why switch from using a finalizer ?? David On 27/09/2017 7:31 PM, Peter Levart wrote: Hi Sherman, At first I checked the Deflater/Inflater changes. I'll come back with ZipFile later.

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

Hi Sherman, At first I checked the Deflater/Inflater changes. I'll come back with ZipFile later. I think the code is mostly correct, but I have a concern. If clean() is invoked via Deflater.end(), then the Deflater instance is still alive and synchronization is necessary as other threads

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

Hi, Please help review the change for JDK-8185582. issue:https://bugs.openjdk.java.net/browse/JDK-8185582 webrev: http://cr.openjdk.java.net/~sherman/8185582/webrev/ csr: https://bugs.openjdk.java.net/browse/JDK-8187485 The proposed change here is to replace the finalize() cleanup