Re: RFR 8080225: FileInputStream cleanup should be improved

[Bernd Eckenfels]

Hello,

if UnreferencedRAFClosesFd.java is supposed to test the behavior of unreachable 
ˋrafˋ I wonder how it works as it hold on raf with a reachabilityFence at the 
end of the main method? Maybe that asks for an explanative comment?

Gruss
Bernd
--
http://bernd.eckenfels.net

From: core-libs-dev  on behalf of Roger 
Riggs 
Sent: Friday, September 29, 2017 7:17:34 PM
To: Core-Libs-Dev
Subject: RFR 8080225: FileInputStream cleanup should be improved

Replacing finalizers in FileInputStream, FileOutputStream, and adding
cleanup to RandomAccessFile
and NIO File Channels with Cleaner based implementations required
changes to FileDescriptor.

The specification of FileInputStream and FileOutputStream is changed to
remove the finalizer
behavior that required their respective close methods to be called.
This change to the behavior is tracked with CSR 8187325 [3].

The FileDescriptor implementations (Unix and Windows) provide a cleanup
function that is now used by
FIS, FOS, RAF, and async and normal FileChannels.  Each requests
FileDescriptor to register a cleanup function
when the fd or handle is assigned and delegates to
FileDescriptor.close.  If the respective
FileDescriptor.close method is not called, the fd or handle will be
closed when the FileDescriptor
is determined to be phantom reachable.

The other uses of FileDescriptor are not intended to be changed, for
example in sockets and datagrams.

Some tests were modified to not rely on finalization; new tests are
provided.

Comments are appreciated on both the CSR [3] and the implementation [1].

[1] webrev: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/

[2] Issue:   https://bugs.openjdk.java.net/browse/JDK-8080225

[3] CSR:  8187325  FileInputStream/FileOutputStream should use the
Cleaner instead of finalize
   https://bugs.openjdk.java.net/browse/JDK-8187325

Thanks, Roger

RFR: 8187954 Update JAX-WS RI integration to latest version

[Jack Li]

Hi,

Please review standalone JAXB/JAXWS changes, synced to jdk/jaxws repo.

JBS: https://bugs.openjdk.java.net/browse/JDK-8187954 

Webrev: http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8187954/10/00/ 


Summary of changes:

jaxws/src/java.xml.bind/share/classes/javax/xml/bind/*
JDK-8186946 - Fix accessibility and other issues in the java.xml.bind module

jaxws/src/java.xml.ws/share/classes/com/sun/xml/internal/messaging/saaj/**
JDK-8186314 - code at c.s.x.i.m.saaj.soap.MessageImpl must be modified to avoid 
crash after javac change
And also contains the fixes for importing nodes for SOAPDocumentFragment


Patch also contains several small bugfixes, not tracked in JBS.


Best regards
Jack Li

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

[Xueming Shen]

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 thrown
 super.finalize();
   ^
(the overridden finalize() in InstrumentedZipFile should now declare 
throws Throwable, since it overrides Object.finalize() and not 
ZipFile.finalize() which is gone).



The other thing I noticed is that Releaser 1st closes the streams 
(that are still reachable via streams WeakHashMap) and also ends the 
associated inflaters. But closing the stream will already release the 
inflater (in case it is a ZipFileInflaterInputStream) into the 
inflaters cache and the cache is scanned and inflaters ended later.


So we don't need a stream -> inflater association outside the stream 
in the form of WeekHashMap. But we still need to keep the set of input 
streams weakly reachable from ZipFile in case we want to close the 
ZipFile explicitly (and there is no harm although useless if this also 
happens as a result of automatic ZipFile cleaner processing).


This could be implemented in a form of:

final Set 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 ends inflaters while they are still 
in cache or associated with the open stream (the only place where 
inflaters should be ended explicitly is in the Releaser). To make this 
even more obvious, it might be good to move the obtaining/releasing 
logic directly into the ZipFileInflaterInputStream constructor which 
would be passed a reference to the inflatersCache instead of the 
Inflater instance.


Here's what I have in mind (I cahnged just the ZipFile and 
FinalizeZipFile):


http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.01/

What do you think?


Peter,

I read those old emails back to 2011 on why we put the "Inflater" into 
the streams as
the value of the map entry. It appears the main (only) purpose is to 
keep the finalization
of  in order. As I explained in previous email, stream 
and its inflater
can be phantom reachable at same round, if inflater gets finalized/ended 
first, then it
can no longer be reused/cached and has to be thrown away, which means 
the caching
mechanism is actually broken/not functioning. We do have use scenario 
depends on it
to avoid too many "new Inflater()' that might pressure the memory usage. 
Putting the
pair in a weakhashmap appears to solve this problem back then (the 
ended() checks are
still there just in case there is rare case, the inflater still gets 
cleaned up first).


The situation might be changed now in Cleaner case, as the cleaner 
object itself now
holds a strong reference, which in theory will/should prevent the 
inflater from being

phantom reachable before stream is cleaned, so we might no longer need this
 pair in a map to have a "ordered" cleanup. Just 
wanted to make sure

my assumption is correct here.

sherman

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

[Xueming Shen]

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 ends inflaters while they are still 
in cache or associated with the open stream (the only place where 
inflaters should be ended explicitly is in the Releaser). To make this 
even more obvious, it might be good to move the obtaining/releasing 
logic directly into the ZipFileInflaterInputStream constructor which 
would be passed a reference to the inflatersCache instead of the 
Inflater instance.




If my memory serves me correctly, this "ended()" check was for the 
situation that the
ZipfileInflaterStream is getting finalized (close() is not called 
explicitly/directly) and its
inflater is also getting finalized at the same time/pass, and the 
inflater.end() gets called
(by its finalize()) first, so when the zfis tries to release the 
inflater back to the cache, it has
been "ended" already. We had some nasty finalize() and object 
resurrection timing
issue in earlier releases, that probably is part of the fix (since it's 
a timing issue, we also
do the same "ended()" check in getInflater() when lease it out) So my 
question is that do
we have the similar situation under the Cleaner? mean the zfis and its 
inflater both are

phantom reachable and the inflater's cleaner gets called first?

I'm still reading your proposed change to remove the "inflater" from the 
stream->inflater

mapping to see if there is any loophole, will reply that separately later.

-sherman

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

[Remi Forax]

Very cool,
i learn something today :)

Rémi

- Mail original -
> De: "mandy chung" 
> À: "core-libs-dev" 
> Envoyé: Vendredi 29 Septembre 2017 23:45:18
> Objet: 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
> 
>   // Check if this klass has an empty finalize method (i.e. one with
> return bytecode only),
>   // in which case we don't have to register objects as finalizable
>   if (!_has_empty_finalizer) {
>     if (_has_finalizer ||
>     (super != NULL && super->has_finalizer())) {
>   ik->set_has_finalizer();
>     }
>   }
> 
> Mandy

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

[mandy chung]

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

  // Check if this klass has an empty finalize method (i.e. one with 
return bytecode only),

  // in which case we don't have to register objects as finalizable
  if (!_has_empty_finalizer) {
    if (_has_finalizer ||
    (super != NULL && super->has_finalizer())) {
  ik->set_has_finalizer();
    }
  }

Mandy

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

[Peter Levart]

On 09/29/17 23:23, Remi Forax wrote:

- Mail original -

De: "Peter Levart" 
À: "Roger Riggs" , "core-libs-dev" 

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 empty bodies) to mitigate source
compatibility.

The problem is that empty finalize() method that throws anything other
than Throwable will not compile.

Correction - it will compile (I was thinking about a method calling just
super.finalize() which is not empty, of course). Yes, this is the
solution to source compatibility.


Overriding finalize() will make the object not to be GCed as soon as it should.


I think the hotspot has an optimization and detects that finalize() has 
no bytecodes and treats the object as not needing finalization.


Regards, Peter


Regards, Peter


Rémi


Roger


[1]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049351.html

On 9/29/2017 4: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 Throwable; must be caught or declared to be
thrown
  super.finalize();
    ^
(the overridden finalize() in InstrumentedZipFile should now
declare throws Throwable, since it overrides Object.finalize() and
not ZipFile.finalize() which is gone).



Yes, it's the expected source incompatibility issue specified in the
CSR request.
I think I had it changed somewhere but obviously it's not in the
webrev. Thanks
for catching it. Yes, the test needs to update to be catch the
throwable.

Will return to the other comments later.

Thanks!
-sherman

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

[Remi Forax]

- Mail original -
> De: "Peter Levart" 
> À: "Roger Riggs" , "core-libs-dev" 
> 
> 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 empty bodies) to mitigate source
>>> compatibility.
>>
>> The problem is that empty finalize() method that throws anything other
>> than Throwable will not compile.
> 
> Correction - it will compile (I was thinking about a method calling just
> super.finalize() which is not empty, of course). Yes, this is the
> solution to source compatibility.
> 

Overriding finalize() will make the object not to be GCed as soon as it should.

> 
>>
>> Regards, Peter
>>

Rémi

>>>
>>> Roger
>>>
>>>
>>> [1]
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049351.html
>>>
>>> On 9/29/2017 4: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 Throwable; must be caught or declared to be
> thrown
>  super.finalize();
>    ^
> (the overridden finalize() in InstrumentedZipFile should now
> declare throws Throwable, since it overrides Object.finalize() and
> not ZipFile.finalize() which is gone).
>
>
 Yes, it's the expected source incompatibility issue specified in the
 CSR request.
 I think I had it changed somewhere but obviously it's not in the
 webrev. Thanks
 for catching it. Yes, the test needs to update to be catch the
 throwable.

 Will return to the other comments later.

 Thanks!
 -sherman


>>>

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

[Peter Levart]

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 empty bodies) to mitigate source 
compatibility.


The problem is that empty finalize() method that throws anything other 
than Throwable will not compile.


Correction - it will compile (I was thinking about a method calling just 
super.finalize() which is not empty, of course). Yes, this is the 
solution to source compatibility.





Regards, Peter



Roger


[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049351.html


On 9/29/2017 4: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 Throwable; must be caught or declared to be 
thrown

 super.finalize();
   ^
(the overridden finalize() in InstrumentedZipFile should now 
declare throws Throwable, since it overrides Object.finalize() and 
not ZipFile.finalize() which is gone).



Yes, it's the expected source incompatibility issue specified in the 
CSR request.
I think I had it changed somewhere but obviously it's not in the 
webrev. Thanks
for catching it. Yes, the test needs to update to be catch the 
throwable.


Will return to the other comments later.

Thanks!
-sherman

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

[Xueming Shen]

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 now, at least 
for me.


https://bugs.openjdk.java.net/browse/JDK-8187485
http://cr.openjdk.java.net/~sherman/8185582/webrev
||


   76  * specified to call the {@code end} method to close the {@code deflater} 
and

s/deflater/Deflater
   80  * The recommended cleanup for compressor is to explicitly call {@code 
end}
   81  * method when it is no longer in use. Existing subclasses of {@code 
Deflater}
   82  * that override {@code end} and require {@code end} to be invoked when 
the
   83  * instance is unreachable should explicitly override {@link 
Object#finalize}
   84  * and call {@code end}.
I suggest not to recommend "explicitly override Object.finalize" (in 
fact, we should discourage it) and the overridden end method should be 
called explicitly.  This was what I suggested in the previous mail:


"calling end() directly/explicitly to release the resource" is being 
recommended at
the first sentence. The second sentence here is meant to provide a 
possible alternative
if any existing subclass is implemented the way that it has the 
dependency on the
old mechanism that its own "end()" being called, for situation that the 
Cleaner is

not possible, or difficult to implement. No value at all?

-Sherman



||
|  * It is strongly recommended to explicitly call {@code end} to
||  * discard any unprocessed input promptly to free up resources
|  * when|||the compressor|is no longer in use.|

||Same comment applies to Inflater.

75  * specified to call the {@code end} method to close the {@code inflater} and
|

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

[Peter Levart]

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


The problem is that empty finalize() method that throws anything other 
than Throwable will not compile.


Regards, Peter



Roger


[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049351.html


On 9/29/2017 4: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 Throwable; must be caught or declared to be thrown

 super.finalize();
   ^
(the overridden finalize() in InstrumentedZipFile should now declare 
throws Throwable, since it overrides Object.finalize() and not 
ZipFile.finalize() which is gone).



Yes, it's the expected source incompatibility issue specified in the 
CSR request.
I think I had it changed somewhere but obviously it's not in the 
webrev. Thanks
for catching it. Yes, the test needs to update to be catch the 
throwable.


Will return to the other comments later.

Thanks!
-sherman

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

[forax]

> De: "Peter Levart" 
> À: "Remi Forax" , "mandy chung" 
> Cc: "Xueming Shen" , "core-libs-dev"
> 
> Envoyé: Vendredi 29 Septembre 2017 22:56:26
> Objet: Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not
> finalizers

> Hi Remi,
Hi Peter, 

> On 09/29/17 22:49, Remi Forax wrote:

>> - Mail original -

>>> De: "mandy chung" [ mailto:mandy.ch...@oracle.com | 
>>>  ]
>>> À: "Peter Levart" [ mailto:peter.lev...@gmail.com | 
>>>  ]
>>> , "Xueming Shen" [ mailto:xueming.s...@oracle.com | 
>>>  ]
>>> , "core-libs-dev" [ mailto:core-libs-dev@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 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 calling setDictionaly on the
 obtained address may be racing with Cleaner thread invoking
 ZStreamRef.run():

>>> What about making the native setDictionary method as an instance method
>>> (currently it's a static method) so that this object remains strongly
>>> reachable until the method returns?

>> Mandy,
>> unlike in C or C++, in Java a reference is garbage collected as soon as you 
>> do
>> not need it anymore,
>> so using an instance method will not change the issue here.

> I might be wrong, but native instance method is an exception. It can't be
> inlined. The preparation for native method invocation makes sure 'this' is 
> kept
> reachable because it can be dereferenced from the native code then and native
> code is out-of-bounds for JIT optimization.
I do not think that you are wrong now but this is a property (a side effect) of 
the current implementation, Panama and Metropolis may change that. 
I think it's better to keep the reference available with a reachability fence. 

> Regards, Peter
Rémi 

>> one way to be sure that a referenced object is not garbage collected is to 
>> use [
>> http://docs.oracle.com/javase/9/docs/api/java/lang/ref/Reference.html#reachabilityFence-java.lang.Object
>> |
>> http://docs.oracle.com/javase/9/docs/api/java/lang/ref/Reference.html#reachabilityFence-java.lang.Object
>> ] -

>>> Mandy

>> Rémi

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

[mandy chung]

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.


https://bugs.openjdk.java.net/browse/JDK-8187485
http://cr.openjdk.java.net/~sherman/8185582/webrev
||


76 * specified to call the {@code end} method to close the {@code 
deflater} and s/deflater/Deflater


80 * The recommended cleanup for compressor is to explicitly call {@code 
end}
81 * method when it is no longer in use. Existing subclasses of {@code 
Deflater}
82 * that override {@code end} and require {@code end} to be invoked 
when the
83 * instance is unreachable should explicitly override {@link 
Object#finalize}

84 * and call {@code end}.

I suggest not to recommend "explicitly override Object.finalize" (in 
fact, we should discourage it) and the overridden end method should be 
called explicitly.  This was what I suggested in the previous mail:||


|* It is strongly recommended to explicitly call {@code end} to ||* discard any unprocessed input promptly to free up resources |* when 
|||the compressor |is no longer in use.| ||Same comment applies to Inflater. 75 * specified to call the {@code end} 
method to close the {@code inflater} and |


s/inflater/Inflater

FinalizeZipFile.zip (I have mentioned this in the other mail):

I suggest to update InstrumentedZipFile to migrate away from the 
finalizer.   I can override the close method instead of the finalize 
method.   It can test explicitly calling close (that's what the test is 
currently doing) and try-with-resource.


TestCleaner.java
 130 throw new RuntimeException("'ZipFile.Source.zfile' is not 
accesible");

s/accesible/accessible


Mandy

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

[Peter Levart]

Hi Remi,

On 09/29/17 22:49, Remi Forax wrote:

- Mail original -

De: "mandy chung" 
À: "Peter Levart" , "Xueming Shen" , 
"core-libs-dev"

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 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 calling setDictionaly on the
obtained address may be racing with Cleaner thread invoking
ZStreamRef.run():

What about making the native setDictionary method as an instance method
(currently it's a static method) so that this object remains strongly
reachable until the method returns?

Mandy,
unlike in C or C++, in Java a reference is garbage collected as soon as you do 
not need it anymore,
so using an instance method will not change the issue here.


I might be wrong, but native instance method is an exception. It can't 
be inlined. The preparation for native method invocation makes sure 
'this' is kept reachable because it can be dereferenced from the native 
code then and native code is out-of-bounds for JIT optimization.


Regards, Peter


one way to be sure that a referenced object is not garbage collected is to use
http://docs.oracle.com/javase/9/docs/api/java/lang/ref/Reference.html#reachabilityFence-java.lang.Object-


Mandy

Rémi

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

[Roger Riggs]

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] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049351.html


On 9/29/2017 4: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 Throwable; must be caught or declared to be thrown

 super.finalize();
   ^
(the overridden finalize() in InstrumentedZipFile should now declare 
throws Throwable, since it overrides Object.finalize() and not 
ZipFile.finalize() which is gone).



Yes, it's the expected source incompatibility issue specified in the 
CSR request.
I think I had it changed somewhere but obviously it's not in the 
webrev. Thanks

for catching it. Yes, the test needs to update to be catch the throwable.

Will return to the other comments later.

Thanks!
-sherman

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

[mandy chung]

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 point, obtaining zsRef.address() and calling setDictionaly on the
obtained address may be racing with Cleaner thread invoking
ZStreamRef.run():

What about making the native setDictionary method as an instance method
(currently it's a static method) so that this object remains strongly
reachable until the method returns?

Mandy,
unlike in C or C++, in Java a reference is garbage collected as soon as you do 
not need it anymore,
so using an instance method will not change the issue here.



The case that Peter observed is when "this" is being optimized out and 
becomes unreachable before setDictionary is called.  Since setDictionary 
is a JNI function, the caller has to pass this (as jobject) to the 
native function.   Would that cover this special case?


Mandy

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

[Peter Levart]

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 that point, obtaining zsRef.address() and calling setDictionaly 
on the obtained address may be racing with Cleaner thread invoking 
ZStreamRef.run():
What about making the native setDictionary method as an instance 
method (currently it's a static method) so that this object remains 
strongly reachable until the method returns?


Mandy


This is a possibility too, yes. In general there might be other places 
where the same could be performed. It is equivalent to puting 
Reference.reachabilityFence(this) after the invocation of 
setDictionary() static native method. But this would only fix public 
setDictionary() instance method. There might be other public methods 
with similar problems. Synchronizing the ZStreamRef.run() method fixes 
all of them in one place.


Regards, Peter

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

[mandy chung]

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 Throwable; must be caught or declared to be thrown
 super.finalize();
   ^
(the overridden finalize() in InstrumentedZipFile should now declare 
throws Throwable, since it overrides Object.finalize() and not 
ZipFile.finalize() which is gone).



Yes, it's the expected source incompatibility issue specified in the 
CSR request.
I think I had it changed somewhere but obviously it's not in the 
webrev. Thanks

for catching it. Yes, the test needs to update to be catch the throwable.



I would suggest to update InstrumentedZipFile to migrate away from the 
finalizer.   I can override the close method instead of the finalize 
method.   It can test explicitly calling close (that's what the test is 
currently doing) and try-with-resource.


Mandy

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

[Remi Forax]

- Mail original -
> De: "mandy chung" 
> À: "Peter Levart" , "Xueming Shen" 
> , "core-libs-dev"
> 
> 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 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 calling setDictionaly on the
>> obtained address may be racing with Cleaner thread invoking
>> ZStreamRef.run():
> What about making the native setDictionary method as an instance method
> (currently it's a static method) so that this object remains strongly
> reachable until the method returns?

Mandy,
unlike in C or C++, in Java a reference is garbage collected as soon as you do 
not need it anymore,
so using an instance method will not change the issue here.

one way to be sure that a referenced object is not garbage collected is to use
http://docs.oracle.com/javase/9/docs/api/java/lang/ref/Reference.html#reachabilityFence-java.lang.Object-

> 
> Mandy

Rémi

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

[Xueming Shen]

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 thrown
 super.finalize();
   ^
(the overridden finalize() in InstrumentedZipFile should now declare 
throws Throwable, since it overrides Object.finalize() and not 
ZipFile.finalize() which is gone).



Yes, it's the expected source incompatibility issue specified in the CSR 
request.
I think I had it changed somewhere but obviously it's not in the webrev. 
Thanks

for catching it. Yes, the test needs to update to be catch the throwable.

Will return to the other comments later.

Thanks!
-sherman

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

[mandy chung]

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 calling setDictionaly on the 
obtained address may be racing with Cleaner thread invoking 
ZStreamRef.run():
What about making the native setDictionary method as an instance method 
(currently it's a static method) so that this object remains strongly 
reachable until the method returns?


Mandy

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

[Peter Levart]

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();
  ^

(the overridden finalize() in InstrumentedZipFile should now declare 
throws Throwable, since it overrides Object.finalize() and not 
ZipFile.finalize() which is gone).



The other thing I noticed is that Releaser 1st closes the streams (that 
are still reachable via streams WeakHashMap) and also ends the 
associated inflaters. But closing the stream will already release the 
inflater (in case it is a ZipFileInflaterInputStream) into the inflaters 
cache and the cache is scanned and inflaters ended later.


So we don't need a stream -> inflater association outside the stream in 
the form of WeekHashMap. But we still need to keep the set of input 
streams weakly reachable from ZipFile in case we want to close the 
ZipFile explicitly (and there is no harm although useless if this also 
happens as a result of automatic ZipFile cleaner processing).


This could be implemented in a form of:

final Set 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 ends inflaters while they are still in 
cache or associated with the open stream (the only place where inflaters 
should be ended explicitly is in the Releaser). To make this even more 
obvious, it might be good to move the obtaining/releasing logic directly 
into the ZipFileInflaterInputStream constructor which would be passed a 
reference to the inflatersCache instead of the Inflater instance.


Here's what I have in mind (I cahnged just the ZipFile and FinalizeZipFile):

http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.01/

What do you think?

Because Inflaters used in ZipFile will never be automatically ended by 
their own cleaners (they are kept strongly reachable in the cache of 
inflaters, which is strongly reachable from the registered ZipFile 
cleanup function), it might be useful to add a special package-private 
constructor to Inflater which would not register its own cleaner. But 
this could be left as an exercise for some later time.


Regards, Peter


On 09/28/17 01:41, 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.


https://bugs.openjdk.java.net/browse/JDK-8187485
http://cr.openjdk.java.net/~sherman/8185582/webrev

-Sherman

On 9/27/17, 3:08 PM, mandy chung wrote:


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 call the {@link #end} method when a {@code Deflater} object
  * becomes unreachable.
  * The {@link #finalize} method is no longer defined.  If a subclass
  * overrides||the {@code end} method, the overridden {@code end} method
  * is no longer invoked.
  *
  * It is strongly recommended to explicitly call {@code end} to
||  * discard any unprocessed input promptly to free up resources
|  * when|||the compressor|is no longer in use.|


|ZipFile
    * @apiNote
|  * In earlier versions, the {@link Object#finalize} method was 
overridden

  * to call the {@link #close} method when a {@code ZipFile} object
  * becomes unreachable.|
|  * The {@link #finalize} method is no longer defined.  If a subclass
  * overrides||the {@code close} method, the overridden {@code close} 
method

  * is no longer invoked.|
  *
|  * The recommended cleanup for|||{@code ZipFile}|  is to explicitly 
call {@code close}

  * or use try-with-resources.|

  657  *
  658  * Since the time when the resources held by this object 
will be released
  659  * is undetermined, if this method is not invoked 
explicitly, it is strongly
  660  * recommended that applications invoke this method as soon 
they have
  661  * finished accessing this {@code ZipFile}. This will 
prevent holding up

  662  * system resources for an undetermined length of time.
  663  *

I would suggest to drop this paragraph.  @apiNote and @implNote in
class spec cover that.

Mandy
|| 

Re: RFR 8080225: FileInputStream cleanup should be improved

[Brian Burkhalter]

Hi Roger,

Looks good overall; total nitpicks here:

FileInputStream (similar story in FileOutputStream)
48-49: “explicitly" is used twice
53: could probably drop “explicitly” here altogether.

The “Shd” in a couple of test names is kind of annoying; perhaps s/Shd/Should/ ?

Copyright dates are not update in a few places.

Thanks,

Brian

On Sep 29, 2017, at 10:17 AM, Roger Riggs  wrote:

> Comments are appreciated on […] the implementation [1].
> 
> [1] webrev: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/

RFR 8080225: FileInputStream cleanup should be improved

[Roger Riggs]

Replacing finalizers in FileInputStream, FileOutputStream, and adding 
cleanup to RandomAccessFile
and NIO File Channels with Cleaner based implementations required 
changes to FileDescriptor.


The specification of FileInputStream and FileOutputStream is changed to 
remove the finalizer

behavior that required their respective close methods to be called.
This change to the behavior is tracked with CSR 8187325 [3].

The FileDescriptor implementations (Unix and Windows) provide a cleanup 
function that is now used by
FIS, FOS, RAF, and async and normal FileChannels.  Each requests 
FileDescriptor to register a cleanup function
when the fd or handle is assigned and delegates to 
FileDescriptor.close.  If the respective
FileDescriptor.close method is not called, the fd or handle will be 
closed when the FileDescriptor

is determined to be phantom reachable.

The other uses of FileDescriptor are not intended to be changed, for 
example in sockets and datagrams.


Some tests were modified to not rely on finalization; new tests are 
provided.


Comments are appreciated on both the CSR [3] and the implementation [1].

[1] webrev: http://cr.openjdk.java.net/~rriggs/webrev-fis-cleanup-8080225/

[2] Issue:   https://bugs.openjdk.java.net/browse/JDK-8080225

[3] CSR:  8187325  FileInputStream/FileOutputStream should use the 
Cleaner instead of finalize

  https://bugs.openjdk.java.net/browse/JDK-8187325

Thanks, Roger

Re: RFR: jsr166 jdk10 integration wave 3

[Martin Buchholz]

On Wed, Sep 27, 2017 at 1:44 PM, Doug Lea  wrote:

> On 09/27/2017 02:38 PM, Paul Sandoz wrote:
> >> Looks good, i just need to look a little more closely at the
> ConcurrentSkipListMap changes.
> >>
> >
> >
> >  328  * Note that, with VarHandles, a boolean result of
> >  329  * compareAndSet must be declared even if not used.
> > This should not be necessary, since for such methods the return is not
> polymorphic. Did you observe some performance or code generation issues?
> >
>
> It was necessary at the time I started rewriting this in June,
> but apparently not now. I'll get rid of the annoying unnecessary
> declarations. Thanks to whoever fixed this!


The latest webrev no longer has the unneeded boolean locals.

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Claes Redestad]

On 2017-09-28 16:22, Jason Mehrens wrote:

Admittedly this is more of a Claes Redestad faster boot up type change but, the 
use of the AlphaDecimalComparator constructor in Comparator might force the 
bytecode verifier to force load AlphaDecimalComparator just to check the return 
type.

If you make factory methods in AlphaDecimalComparator you can usually craft the 
return type to match and the verify of Comparator will not force load the 
AlphaDecimalComparator.
Otherwise, the more ugly solution is to wrap 'new' with Comparator.class.cast.

Jason


... you called? :-)

You're probably right in the general case, but I think we avoid this 
particular startup issue here since the (HotSpot) bytecode verifier by 
default doesn't check classes defined to the the boot loader.


/Claes

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Jason Mehrens]

Makes sense on the sub-sequences having to be string.  Looks good.

Jason


From: Ivan Gerasimov 
Sent: Thursday, September 28, 2017 3:19 PM
To: Jason Mehrens; core-libs-dev
Subject: Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

Thank you Jason for valuable input!


On 9/28/17 7:22 AM, Jason Mehrens wrote:
> Ivan,
>
> Am I correct that subtraction of code points is safe from underflow due to 
> the range of Character.MIN_CODE_POINT and Character.MAX_CODE_POINT?  That 
> would explain why you are not using Integer.compare.
Right.  Since codepoints are all positive, it should be safe to subtract
them.

> One alternative to calling CharSequence.subSequence is to use 
> CharBuffer.wrap(CharSequence csq, int start, int end).  The sub-sequences are 
> short lived so that may be faster in the String case.
Yes, it's a good point, given how expensive String.subSequence() is.
The disadvantage of using CharBuffer.wrap() is that it wouldn't be
possible to use the standard String comparators as the custom
alphaComparator.
With the current implementation, it is possible (with additional casts,
of course) because we know that String.subSequence returns a String object.

To be honest, I see no easy way to optimize the String case with the use
of a custom comparator because we need to pass to it portions of the
input as String objects.
One possibility might be to recognize common cases, like using the
standard comparators (for example, String.CASE_INSENSITIVE_ORDER) in
place of the custom alphaComparator, and then execute special variant of
the routine, which doesn't create explicit subsequences, but works on
the entire sequence instead.

At this point, however, it's probably too early to do such optimizations.

> Admittedly this is more of a Claes Redestad faster boot up type change but, 
> the use of the AlphaDecimalComparator constructor in Comparator might force 
> the bytecode verifier to force load AlphaDecimalComparator just to check the 
> return type.
Hm, interesting.
We can introduce another factory method in the Comparators class, so
that AlphaDecimalComparator won't be mentioned in the Comparator at all.

Please see the updated webrev:
http://cr.openjdk.java.net/~igerasim/8134512/05/webrev/

With kind regards,
Ivan


> If you make factory methods in AlphaDecimalComparator you can usually craft 
> the return type to match and the verify of Comparator will not force load the 
> AlphaDecimalComparator.
> Otherwise, the more ugly solution is to wrap 'new' with Comparator.class.cast.
>
> Jason
>
>
>
> 
> From: core-libs-dev  on behalf of 
> Ivan Gerasimov 
> Sent: Monday, September 25, 2017 12:49 PM
> To: core-libs-dev
> Subject: Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator
>
> Hello!
>
> Could you please review at your convenience?
>
> In the latest webrev I took all suggestions into account (unless I
> missed something.)
>
> http://cr.openjdk.java.net/~igerasim/8134512/04/webrev/
>
>
> I think, if the suggested comparator is found useful by the users, then
> it may make sense to create the String-oriented variant, which can be
> implemented through the CharSequence-oriented one as:
>
> class String {
>   ...
>   @SuppressWarnings("unchecked")
>   public static  Comparator
>   comparingAlphaDecimal(Comparator alphaComparator) {
>   return (Comparator) (Comparator)
>   new
> Comparators.AlphaDecimalComparator<>(Objects.requireNonNull(
>   (Comparator) alphaComparator),
> false);
>   }
> }
>
> This will be safe, since the specification guarantees that
> String.subSequence() returns a String.
>
> Then in the application code it would be possible to instantiate the
> comparators as
>
>   String.comparingAlphaDecimal(String::compareTo);
>
>   String.comparingAlphaDecimal(String::compareToIgnoreCase);
>
> or, alternatively,
>   String.comparingAlphaDecimal(Comparator.naturalOrder());
>
> String.comparingAlphaDecimal(String.CASE_INSENSITIVE_ORDER);
>
> But this could be deferred for later, of course.
>
> With kind regards,
> Ivan
>
>
> On 8/27/17 1:38 PM, Ivan Gerasimov wrote:
>> Hello everyone!
>>
>> Here's another iteration of the comparator with suggested improvements.
>>
>> Now, there is the only input argument -- the alpha-comparator for
>> comparing the non-decimal-digit sub-sequences.
>>
>> For the javadoc I used the text suggested by Peter with some
>> modifications, additional example and API/implementation notes.
>> Overall, the javadoc looks heavier than need to me, so I'd love to
>> hear comments about how to make it shorter and cleaner.
>>
>> Also, I adopted the name AlphaDecimal, suggested by Peter.  This name
>> is one of popular in the list of variants found in the wild. So, there
>> are higher chances 

Re: RFR(XS): 8188135: Fix VS 2010 build after "8187631: Refactor FileDescriptor close implementation"

[Roger Riggs]

+1

On 9/29/2017 4:43 AM, Alan Bateman wrote:



On 29/09/2017 09:05, Lindenmaier, Goetz wrote:

Hi,

please review this tiny fix for the build with Visual Studio 2010:
http://cr.openjdk.java.net/~goetz/wr17/8188135-winBuild/webrev.01/


Looks okay although I thought VS 2010 was dropped a long time ago.

-Alan

RE: RFR(XS): 8188135: Fix VS 2010 build after "8187631: Refactor FileDescriptor close implementation"

[Lindenmaier, Goetz]

Thanks, Alan!

Unfotunately SAP has to stick to VS 2010 a while.

Best regards,
  Goetz.

> -Original Message-
> From: Alan Bateman [mailto:alan.bate...@oracle.com]
> Sent: Freitag, 29. September 2017 10:43
> To: Lindenmaier, Goetz ; core-libs-
> d...@openjdk.java.net
> Subject: Re: RFR(XS): 8188135: Fix VS 2010 build after "8187631: Refactor
> FileDescriptor close implementation"
> 
> 
> 
> On 29/09/2017 09:05, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > please review this tiny fix for the build with Visual Studio 2010:
> > http://cr.openjdk.java.net/~goetz/wr17/8188135-winBuild/webrev.01/
> >
> Looks okay although I thought VS 2010 was dropped a long time ago.
> 
> -Alan

RE: RFR(XS): 8188135: Fix VS 2010 build after "8187631: Refactor FileDescriptor close implementation"

[Lindenmaier, Goetz]

Thanks David!

... at least SAP got rid of gcc 4.1.2 :)

Best regards,
 Goetz.

> -Original Message-
> From: David Holmes [mailto:david.hol...@oracle.com]
> Sent: Freitag, 29. September 2017 10:33
> To: Lindenmaier, Goetz ; core-libs-
> d...@openjdk.java.net
> Subject: Re: RFR(XS): 8188135: Fix VS 2010 build after "8187631: Refactor
> FileDescriptor close implementation"
> 
> On 29/09/2017 6:05 PM, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > please review this tiny fix for the build with Visual Studio 2010:
> > http://cr.openjdk.java.net/~goetz/wr17/8188135-winBuild/webrev.01/
> 
> Looks fine.
> 
> One day we'll stop getting bitten by archaic compilers :(
> 
> Cheers,
> David
> 
> > Best regards,
> >Goetz.
> >

Re: RFR(XS): 8188135: Fix VS 2010 build after "8187631: Refactor FileDescriptor close implementation"

[Alan Bateman]

On 29/09/2017 09:05, Lindenmaier, Goetz wrote:

Hi,

please review this tiny fix for the build with Visual Studio 2010:
http://cr.openjdk.java.net/~goetz/wr17/8188135-winBuild/webrev.01/


Looks okay although I thought VS 2010 was dropped a long time ago.

-Alan

Re: RFR(XS): 8188135: Fix VS 2010 build after "8187631: Refactor FileDescriptor close implementation"

[David Holmes]

On 29/09/2017 6:05 PM, Lindenmaier, Goetz wrote:

Hi,

please review this tiny fix for the build with Visual Studio 2010:
http://cr.openjdk.java.net/~goetz/wr17/8188135-winBuild/webrev.01/


Looks fine.

One day we'll stop getting bitten by archaic compilers :(

Cheers,
David


Best regards,
   Goetz.

RFR(XS): 8188135: Fix VS 2010 build after "8187631: Refactor FileDescriptor close implementation"

[Lindenmaier, Goetz]

Hi,

please review this tiny fix for the build with Visual Studio 2010:
http://cr.openjdk.java.net/~goetz/wr17/8188135-winBuild/webrev.01/

Best regards,
  Goetz.