Re: [jdk17] RFR: 8269185: Directories in /opt/runtimepackagetest and /path/to/jdk-17 are different

[Alexander Matveev]

On Fri, 2 Jul 2021 22:17:09 GMT, Alexey Semenyuk  wrote:

> Disable stripping to prevent rpmbuild from modifying executables

Marked as reviewed by almatvee (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/207

Integrated: Merge jdk17

[Jesper Wilhelmsson]

On Fri, 2 Jul 2021 19:34:35 GMT, Jesper Wilhelmsson  
wrote:

> Forwardport JDK 17 -> JDK 18

This pull request has now been integrated.

Changeset: 17f53f2f
Author:Jesper Wilhelmsson 
URL:   
https://git.openjdk.java.net/jdk/commit/17f53f2f9c5928395eff9186160924e9a8e9a794
Stats: 341 lines in 12 files changed: 266 ins; 34 del; 41 mod

Merge

-

PR: https://git.openjdk.java.net/jdk/pull/4673

Re: RFR: 8268974: GetJREPath() JLI function fails to locate libjava.so if not standard Java launcher is used

[Alexey Semenyuk]

On Sun, 20 Jun 2021 07:25:54 GMT, Alan Bateman  wrote:

>> GetApplicationHomeFromDll() fails if the path to libjli.so contains "bin" 
>> component (/tmp/bin/HelloWorld/lib/runtime/lib/libjli.so). TruncatePath() 
>> looks for "/bin/" first in "/tmp/bin/HelloWorld/lib/runtime/lib/libjli.so" 
>> string and then it looks for "/lib/". But this is wrong order as it should 
>> look for "/lib/" first. I.e. TruncatePath() should look for "/bin/" and then 
>> for "/lib/" if called from GetApplicationHome() and for "/lib/" first and 
>> then for "/bin/" if called from GetApplicationHomeFromDll().
>
> Is it possible to add a test for this that is completely independent of 
> jpackage? I think there are a few existing tests that copy the run-time image 
> to a new location for testing purposes.
> 
> We may need to rename the JBS description to make it clearer what this issue 
> is about.
> 
> A minor nit is that "pathisso" will be confusing to anyone looking at this 
> code, maybe find a better name or put a comment in TruncatePath to explain 
> it. I assume the comments at the findLastPathComponent use site will also 
> need to be clarified.

@AlanBateman any input on this issue from you?

-

PR: https://git.openjdk.java.net/jdk/pull/4534

Re: RFR: 8214761: Bug in parallel Kahan summation implementation

[stefan-zobel]

On Fri, 2 Jul 2021 20:30:24 GMT, Andrew Haley  wrote:

> Crikey, how did we get that wrong?
> It'd be nice if we had a regression test for this. Can you provide one, 
> please?

I found this: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2018-December/057239.html
Ivan Gerasimov already tackled this back then. His webrev still exists and it 
contains a simple regression test.

-

PR: https://git.openjdk.java.net/jdk/pull/4674

[jdk17] RFR: 8269185: Directories in /opt/runtimepackagetest and /path/to/jdk-17 are different

[Alexey Semenyuk]

Disable stripping to prevent rpmbuild from modifying executables

-

Commit messages:
 - Disable executables stripping

Changes: https://git.openjdk.java.net/jdk17/pull/207/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=207=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8269185
  Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/207.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/207/head:pull/207

PR: https://git.openjdk.java.net/jdk17/pull/207

Re: RFR: 8214761: Bug in parallel Kahan summation implementation

[stefan-zobel]

On Fri, 2 Jul 2021 20:12:39 GMT, Ian Graves  wrote:

> 8214761: Bug in parallel Kahan summation implementation

What about `Collectors.computeFinalSum()` - should this be `double tmp = 
summands[0] + summands[1];` or `double tmp = summands[0] - summands[1];` ?

-

PR: https://git.openjdk.java.net/jdk/pull/4674

Re: RFR: 8214761: Bug in parallel Kahan summation implementation

[stefan-zobel]

On Fri, 2 Jul 2021 20:12:39 GMT, Ian Graves  wrote:

> 8214761: Bug in parallel Kahan summation implementation

src/java.base/share/classes/java/util/DoubleSummaryStatistics.java line 161:

> 159: 
> 160: //Negating this value because low-order bits are in negated form
> 161: sumWithCompensation(-other.sumCompensation);

Wouldn't that be `double tmp =  sum - sumCompensation;` in getSum() in line 246 
too?

-

PR: https://git.openjdk.java.net/jdk/pull/4674

Re: [jdk17] RFR: 8269281: java/foreign/TestUpcall.java times out

[Maurizio Cimadamore]

On Thu, 1 Jul 2021 21:11:44 GMT, Maurizio Cimadamore  
wrote:

> The previous fix to this test which used removed the `VerifyDependency` flag 
> worked well, but we're still observing rare timeouts. Looking at the test 
> reports, it seems likely that slightly increasing the timeout would do the 
> trick.

After an offline discussion we agreed to leave the test unchanged at the 
moment. It is not immediately clear if the problem is the test (which runs in 
~40 seconds) or with specific machines, as we're seeing some very wild spikes. 
The test uses some logic to prod the GC (so that cleaners are run and off-heap 
resources are released) - but the same logic is also present in the dual 
TestDowncall which has never timed out.

-

PR: https://git.openjdk.java.net/jdk17/pull/198

[jdk17] Withdrawn: 8269281: java/foreign/TestUpcall.java times out

[Maurizio Cimadamore]

On Thu, 1 Jul 2021 21:11:44 GMT, Maurizio Cimadamore  
wrote:

> The previous fix to this test which used removed the `VerifyDependency` flag 
> worked well, but we're still observing rare timeouts. Looking at the test 
> reports, it seems likely that slightly increasing the timeout would do the 
> trick.

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk17/pull/198

Re: RFR: 8214761: Bug in parallel Kahan summation implementation

[Andrew Haley]

On Fri, 2 Jul 2021 20:12:39 GMT, Ian Graves  wrote:

> 8214761: Bug in parallel Kahan summation implementation

Crikey, how did we get that wrong?
It'd be nice if we had a regression test for this. Can you provide one, please?

-

PR: https://git.openjdk.java.net/jdk/pull/4674

RFR: 8214761: Bug in parallel Kahan summation implementation

[Ian Graves]

8214761: Bug in parallel Kahan summation implementation

-

Commit messages:
 - 8214761: Bug in parallel Kahan summation implementation

Changes: https://git.openjdk.java.net/jdk/pull/4674/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4674=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8214761
  Stats: 18 lines in 3 files changed: 11 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4674.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4674/head:pull/4674

PR: https://git.openjdk.java.net/jdk/pull/4674

Integrated: 8268664: The documentation of the Scanner.hasNextLine is incorrect

[Ian Graves]

On Tue, 22 Jun 2021 04:22:34 GMT, Ian Graves  wrote:

> 8268664: The documentation of the Scanner.hasNextLine is incorrect

This pull request has now been integrated.

Changeset: 0d0f6a4b
Author:Ian Graves 
URL:   
https://git.openjdk.java.net/jdk/commit/0d0f6a4becfb14304f6cea9d3a1d113f049214c0
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod

8268664: The documentation of the Scanner.hasNextLine is incorrect

Reviewed-by: rriggs, bpb, iris

-

PR: https://git.openjdk.java.net/jdk/pull/4547

RFR: Merge jdk17

[Jesper Wilhelmsson]

Forwardport JDK 17 -> JDK 18

-

Commit messages:
 - Merge
 - 8269768: JFR Terminology Refresh
 - 8269775: compiler/codegen/ClearArrayTest.java failed with "assert(false) 
failed: bad AD file"
 - 8269543: The warning for System::setSecurityManager should only appear once 
for each caller
 - 8262017: C2: assert(n != __null) failed: Bad immediate dominator info.
 - 8269771: assert(tmp == _callprojs.fallthrough_catchproj) failed: allocation 
control projection
 - 8265132: C2 compilation fails with assert "missing precedence edge"

The webrevs contain the adjustments done while merging with regards to each 
parent branch:
 - master: https://webrevs.openjdk.java.net/?repo=jdk=4673=00.0
 - jdk17: https://webrevs.openjdk.java.net/?repo=jdk=4673=00.1

Changes: https://git.openjdk.java.net/jdk/pull/4673/files
  Stats: 341 lines in 12 files changed: 266 ins; 34 del; 41 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4673.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4673/head:pull/4673

PR: https://git.openjdk.java.net/jdk/pull/4673

Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]

[Joe Wang]

On Fri, 2 Jul 2021 19:08:09 GMT, Brian Burkhalter  wrote:

>> Right. I understand. The intention was, unlike the overridden method that 
>> returns 0 if len == 0 and -1 if at the end of the stream, this method 
>> returns -1 in both cases. A careful reader, after comparing both methods, 
>> would understand correctly that the difference is in the case of "len==0". 
>> I'm fine if you think this is good enough. Just a thought though that the 
>> statement could be interpreted as if both conditions need to be met at the 
>> same time (if "and" is taken as "&&", e.g. if (pos>==count && len==0) ). 
>> 
>> Something like the following might be clearer?
>>  * Unlike the {@link InputStream#read(byte[],int,int) overridden method}
>>  * of {@code InputStream} that returns {@code -1} if the end of the 
>> stream
>>  * has been reached and {@code 0} if {@code len == 0}, this method 
>> returns
>>  * {@code -1} in both cases.
>> 
>> Just my 2 cents.
>
> Thanks for the suggestion. I am happy with it as is, but I'll hold off 
> integrating it for now and rethink it later.

Ok, good to know. Have a great weekend!

-

PR: https://git.openjdk.java.net/jdk17/pull/189

Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]

[Joe Wang]

On Fri, 2 Jul 2021 16:58:18 GMT, Brian Burkhalter  wrote:

>> Modify the specification of 
>> `java.io.ByteArrayInputStream#read(byte[],int,int)` to indicate that `-1` is 
>> returned instead of `0` when the stream is at its end and the third 
>> parameter, `len`, is zero.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   6766844: Correct error messages in test

Marked as reviewed by joehw (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/189

Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]

[Brian Burkhalter]

On Fri, 2 Jul 2021 18:43:36 GMT, Joe Wang  wrote:

>> Both conditions of the statement are intended to be met. If the stream is at 
>> its end then
>> 
>> if (pos >= count) {
>> return -1;
>> }
>> 
>> will cause `-1` to be returned because at end of stream the condition `pos 
>> >= count` is met.
>> 
>> The overridden method always returns `0` if `len == 0`.:
>> 
>> 
>> public int read(byte b[], int off, int len) throws IOException {
>> Objects.checkFromIndexSize(off, len, b.length);
>> if (len == 0) {
>> return 0;
>> }
>
> Right. I understand. The intention was, unlike the overridden method that 
> returns 0 if len == 0 and -1 if at the end of the stream, this method returns 
> -1 in both cases. A careful reader, after comparing both methods, would 
> understand correctly that the difference is in the case of "len==0". I'm fine 
> if you think this is good enough. Just a thought though that the statement 
> could be interpreted as if both conditions need to be met at the same time 
> (if "and" is taken as "&&", e.g. if (pos>==count && len==0) ). 
> 
> Something like the following might be clearer?
>  * Unlike the {@link InputStream#read(byte[],int,int) overridden method}
>  * of {@code InputStream} that returns {@code -1} if the end of the stream
>  * has been reached and {@code 0} if {@code len == 0}, this method returns
>  * {@code -1} in both cases.
> 
> Just my 2 cents.

Thanks for the suggestion. I am happy with it as is, but I'll hold off 
integrating it for now and rethink it later.

-

PR: https://git.openjdk.java.net/jdk17/pull/189

RE: Methods removeFirstIf in Collection and skipFirst in Stream

[Alberto Otero Rodríguez]

Hi,

In my case, I have a list of entities related to another entity. In that list 
obviously there weren't repeated entities (entities with the same id). And I 
have the id of an entity to be removed from that list.

So, I have the following alternatives which are pretty inefficient:

1) entityList.removeIf(entity -> entityIdToRemove.equals(entity.getId()));

2) entityList = entityList.stream()
.filter(entity -> !entityIdToRemove.equals(entity.getId()))
.collect(Collectors.toList());

And I have this other alternative which is too much code:

Iterator iterator = entityList.iterator();
while (iterator.hasNext()) {
Entity entity = iterator.next();
if(entityIdToRemove.equals(entity.getId())) {
itr.remove();
break;
}
}

So, what I propose would be something like:

1) entityList.removeFirstIf(entity -> entityIdToRemove.equals(entity.getId()));

2) entityList = entityList.stream()
.skipFirst(entity -> entityIdToRemove.equals(entity.getId()))
.collect(Collectors.toList());

Regards,

Alberto.

De: Tagir Valeev 
Enviado: viernes, 2 de julio de 2021 19:59
Para: Alberto Otero Rodríguez 
Cc: core-libs-dev 
Asunto: Re: Methods removeFirstIf in Collection and skipFirst in Stream

Hello!

It's unclear why one might need methods like this. Could you please provide any 
real life use cases when such methods could be useful? Thanks.

With best regards,
Tagir Valeev.

пт, 2 июл. 2021 г., 21:28 Alberto Otero Rodríguez 
mailto:albest...@hotmail.com>>:
Sorry for the links, the methods would be:

1) In Collection:
   default boolean removeFirstIf​(Predicatehttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/Collection.html>>
 filter)

2) In Stream:
   Stream skipFirst(Predicatehttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/Stream.html>>
 predicate)

De: Alberto Otero Rodríguez
Enviado: viernes, 2 de julio de 2021 16:22
Para: core-libs-dev@openjdk.java.net 
mailto:core-libs-dev@openjdk.java.net>>
Asunto: Methods removeFirstIf in Collection and skipFirst in Stream

Hi,

I think it would be interesting adding the following methods:

1) In Collection:
   default boolean 
removeFirstIf​(Predicatehttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/Collection.html>>
 filter)

2) In Stream:
   
Streamhttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/Stream.html>>
 
skipFirst(Predicatehttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/Stream.html>>
 predicate)

The purpose of both methods would be the same. Deleting the first element in 
the collection that fulfills the predicate.

This could be useful in collections/streams with no duplicate elements where 
performance would be better than using removeIf from Collection of filter from 
Stream.

Regards,

Alberto.

Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]

[Joe Wang]

On Fri, 2 Jul 2021 17:40:08 GMT, Brian Burkhalter  wrote:

>> src/java.base/share/classes/java/io/ByteArrayInputStream.java line 161:
>> 
>>> 159:  * Unlike the {@link InputStream#read(byte[],int,int) overridden 
>>> method}
>>> 160:  * of {@code InputStream}, this method returns {@code -1} instead 
>>> of zero
>>> 161:  * if the end of the stream has been reached and {@code len == 0}.
>> 
>> The statement "return -1 if the end of the stream has been reached and len 
>> == 0" gives an impression that it requires both conditions to be met: end of 
>> the stream && len==0,  but the tests show -1 is expected if len == 0 without 
>> an attempt to read the stream. 
>> 
>> The overridden method stated that "If len is zero, then no bytes are read 
>> and 0 is returned", the above note looks like was meant for this statement 
>> since the overridden method also return -1 if the stream is at end of file.
>
> Both conditions of the statement are intended to be met. If the stream is at 
> its end then
> 
> if (pos >= count) {
> return -1;
> }
> 
> will cause `-1` to be returned because at end of stream the condition `pos >= 
> count` is met.
> 
> The overridden method always returns `0` if `len == 0`.:
> 
> 
> public int read(byte b[], int off, int len) throws IOException {
> Objects.checkFromIndexSize(off, len, b.length);
> if (len == 0) {
> return 0;
> }

Right. I understand. The intention was, unlike the overridden method that 
returns 0 if len == 0 and -1 if at the end of the stream, this method returns 
-1 in both cases. A careful reader, after comparing both methods, would 
understand correctly that the difference is in the case of "len==0". I'm fine 
if you think this is good enough. Just a thought though that the statement 
could be interpreted as if both conditions need to be met at the same time (if 
"and" is taken as "&&", e.g. if (pos>==count && len==0) ). 

Something like the following might be clearer?
 * Unlike the {@link InputStream#read(byte[],int,int) overridden method}
 * of {@code InputStream} that returns {@code -1} if the end of the stream
 * has been reached and {@code 0} if {@code len == 0}, this method returns
 * {@code -1} in both cases.

Just my 2 cents.

-

PR: https://git.openjdk.java.net/jdk17/pull/189

Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]

[Alan Bateman]

On 02/07/2021 18:16, Lance Andersen wrote:

:

That being said, if we want to follow Alan’s suggestion and throw an Exception, 
I am OK with that as well.

Either way, we currently cannot access the file via Zip FS due to the call to 
ZipPath::getResolvedPath() for all access and the path is only normalized when 
the Inodes are created.

Alan, do you have a specific preference?

Not yet but if you tolerate these hostile entries then it means that all 
access with relative paths containing a "." or ".." element will be 
ambiguous. It will break relativize, normalize, and maybe other path 
operations. I assume it will break copy/move operations when the source 
is in a zip file and the target is in a non-zip file. I suspect it would 
also need to encode file names when open zip file for read/write access 
because the native file system is used for caching.


-Alan

Integrated: 8188046: java.lang.Math.mutliplyHigh does not run in constant time

[Brian Burkhalter]

On Thu, 1 Jul 2021 23:46:00 GMT, Brian Burkhalter  wrote:

> Please consider this proposal to remove the fast path in 
> `java.lang.Math.multiplyHigh()` the small performance improvement of which is 
> eclipsed by the branch penalty.

This pull request has now been integrated.

Changeset: cb795893
Author:Brian Burkhalter 
URL:   
https://git.openjdk.java.net/jdk/commit/cb795893be8e6dcf725d8022aca16f657d3cc03c
Stats: 25 lines in 1 file changed: 0 ins; 11 del; 14 mod

8188046: java.lang.Math.mutliplyHigh does not run in constant time

Reviewed-by: rriggs, darcy

-

PR: https://git.openjdk.java.net/jdk/pull/4660

Integrated: 8188044: We need Math.unsignedMultiplyHigh

[Brian Burkhalter]

On Wed, 30 Jun 2021 23:13:06 GMT, Brian Burkhalter  wrote:

> Please consider this proposal to add a method 
> `unsignedMultiplyHigh(long,long)` to each of `java.lang.Math` and 
> `java.lang.StrictMath`.

This pull request has now been integrated.

Changeset: ca4bea46
Author:Brian Burkhalter 
URL:   
https://git.openjdk.java.net/jdk/commit/ca4bea466581217cae2278c98c0fdc568c043818
Stats: 81 lines in 3 files changed: 71 ins; 0 del; 10 mod

8188044: We need Math.unsignedMultiplyHigh

Reviewed-by: rriggs, aph, darcy

-

PR: https://git.openjdk.java.net/jdk/pull/4644

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v4]

[Joe Darcy]

On Fri, 2 Jul 2021 16:41:16 GMT, Brian Burkhalter  wrote:

>> Please consider this proposal to add a method 
>> `unsignedMultiplyHigh(long,long)` to each of `java.lang.Math` and 
>> `java.lang.StrictMath`.
>
> Brian Burkhalter has refreshed the contents of this pull request, and 
> previous commits have been removed. The incremental views will show 
> differences compared to the previous content of the PR.

Marked as reviewed by darcy (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4644

Re: Methods removeFirstIf in Collection and skipFirst in Stream

[Tagir Valeev]

Hello!

It's unclear why one might need methods like this. Could you please provide
any real life use cases when such methods could be useful? Thanks.

With best regards,
Tagir Valeev.

пт, 2 июл. 2021 г., 21:28 Alberto Otero Rodríguez :

> Sorry for the links, the methods would be:
>
> 1) In Collection:
>default boolean removeFirstIf​(Predicate https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/Collection.html>>
> filter)
>
> 2) In Stream:
>Stream skipFirst(Predicate https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/Stream.html>>
> predicate)
> 
> De: Alberto Otero Rodríguez
> Enviado: viernes, 2 de julio de 2021 16:22
> Para: core-libs-dev@openjdk.java.net 
> Asunto: Methods removeFirstIf in Collection and skipFirst in Stream
>
> Hi,
>
> I think it would be interesting adding the following methods:
>
> 1) In Collection:
>default boolean removeFirstIf​(Predicate<
> https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/function/Predicate.html> super E<
> https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/Collection.html>>
> filter)
>
> 2) In Stream:
>Stream<
> https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/Stream.html
> > https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/Stream.html>>
> skipFirst(Predicate<
> https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/function/Predicate.html> super T<
> https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/Stream.html>>
> predicate)
>
> The purpose of both methods would be the same. Deleting the first element
> in the collection that fulfills the predicate.
>
> This could be useful in collections/streams with no duplicate elements
> where performance would be better than using removeIf from Collection of
> filter from Stream.
>
> Regards,
>
> Alberto.
>

Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]

[Brian Burkhalter]

On Fri, 2 Jul 2021 17:27:34 GMT, Joe Wang  wrote:

>> Brian Burkhalter has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   6766844: Correct error messages in test
>
> src/java.base/share/classes/java/io/ByteArrayInputStream.java line 161:
> 
>> 159:  * Unlike the {@link InputStream#read(byte[],int,int) overridden 
>> method}
>> 160:  * of {@code InputStream}, this method returns {@code -1} instead 
>> of zero
>> 161:  * if the end of the stream has been reached and {@code len == 0}.
> 
> The statement "return -1 if the end of the stream has been reached and len == 
> 0" gives an impression that it requires both conditions to be met: end of the 
> stream && len==0,  but the tests show -1 is expected if len == 0 without an 
> attempt to read the stream. 
> 
> The overridden method stated that "If len is zero, then no bytes are read and 
> 0 is returned", the above note looks like was meant for this statement since 
> the overridden method also return -1 if the stream is at end of file.

Both conditions of the statement are intended to be met. If the stream is at 
its end then

if (pos >= count) {
return -1;
}

will cause `-1` to be returned because at end of stream the condition `pos >= 
count` is met.

The overridden method always returns `0` if `len == 0`.:


public int read(byte b[], int off, int len) throws IOException {
Objects.checkFromIndexSize(off, len, b.length);
if (len == 0) {
return 0;
}

-

PR: https://git.openjdk.java.net/jdk17/pull/189

Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]

[Joe Wang]

On Fri, 2 Jul 2021 16:58:18 GMT, Brian Burkhalter  wrote:

>> Modify the specification of 
>> `java.io.ByteArrayInputStream#read(byte[],int,int)` to indicate that `-1` is 
>> returned instead of `0` when the stream is at its end and the third 
>> parameter, `len`, is zero.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   6766844: Correct error messages in test

src/java.base/share/classes/java/io/ByteArrayInputStream.java line 161:

> 159:  * Unlike the {@link InputStream#read(byte[],int,int) overridden 
> method}
> 160:  * of {@code InputStream}, this method returns {@code -1} instead of 
> zero
> 161:  * if the end of the stream has been reached and {@code len == 0}.

The statement "return -1 if the end of the stream has been reached and len == 
0" gives an impression that it requires both conditions to be met: end of the 
stream && len==0,  but the tests show -1 is expected if len == 0 without an 
attempt to read the stream. 

The overridden method stated that "If len is zero, then no bytes are read and 0 
is returned", the above note looks like was meant for this statement since the 
overridden method also return -1 if the stream is at end of file.

-

PR: https://git.openjdk.java.net/jdk17/pull/189

Re: RFR: 8188046: java.lang.Math.mutliplyHigh does not run in constant time

[Joe Darcy]

On Thu, 1 Jul 2021 23:46:00 GMT, Brian Burkhalter  wrote:

> Please consider this proposal to remove the fast path in 
> `java.lang.Math.multiplyHigh()` the small performance improvement of which is 
> eclipsed by the branch penalty.

Marked as reviewed by darcy (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4660

Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]

[Lance Andersen]

On Fri, 2 Jul 2021 16:58:18 GMT, Brian Burkhalter  wrote:

>> Modify the specification of 
>> `java.io.ByteArrayInputStream#read(byte[],int,int)` to indicate that `-1` is 
>> returned instead of `0` when the stream is at its end and the third 
>> parameter, `len`, is zero.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   6766844: Correct error messages in test

Marked as reviewed by lancea (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/189

Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]

[Naoto Sato]

On Fri, 2 Jul 2021 16:58:18 GMT, Brian Burkhalter  wrote:

>> Modify the specification of 
>> `java.io.ByteArrayInputStream#read(byte[],int,int)` to indicate that `-1` is 
>> returned instead of `0` when the stream is at its end and the third 
>> parameter, `len`, is zero.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   6766844: Correct error messages in test

Marked as reviewed by naoto (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/189

Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]

[Lance Andersen]

On Jul 2, 2021, at 12:13 PM, Lance Andersen 
mailto:lance.ander...@oracle.com>> wrote:



On Jul 2, 2021, at 8:08 AM, Jaikiran Pai 
mailto:jai.forums2...@gmail.com>> wrote:

Hello Lance,

On 02/07/21 4:42 pm, Lance Andersen wrote:
Hi Jaikiran,

Consider:


try (var os = Files.newOutputStream(ZIPFILE);
 ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
}


With your proposed fix, you will only return "/" when you walk the the Zip file 
and we should also return "/Hello.txt"

Thank you for noticing this issue in my change and bringing this up. I have a 
question around this use case. Please consider a small variation to your 
example as below:

try (var os = Files.newOutputStream(ZIPFILE);
 ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
zos.closeEntry();
zos.putNextEntry(new ZipEntry("/Hello.txt"));
zos.write("Bye bye".getBytes(StandardCharsets.UTF_8));
zos.closeEntry();

}

Notice that I now have a zip/jar which has 2 differently named entries 
"../Hello.txt" and "/Hello.txt". This creates the archive without any issues 
and those 2 entries are noted in its listing. Now assuming someone walks over 
this jar using the ZipFileSystem, starting at root ("/"), what should be the 
expected output? The path(s) returned will end up being "/" and "/Hello.txt" 
but which resource is expected to be served in this case? The one which has 
"Hello World" in it or the one which has "Bye bye"? This example can be 
extended a bit more by introducing a "./Hello.txt", in the jar, with yet 
another different content in that entry. Is there some specification for this 
that I can check and adapt the test case accordingly?


Consider Hello.zip created via:


try (var os = Files.newOutputStream(Path.of("Hello.zip"));
 ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello".getBytes(StandardCharsets.UTF_8));
zos.putNextEntry(new ZipEntry("Hello.txt"));
zos.write("Another Hello".getBytes(StandardCharsets.UTF_8));

}

Winzip does not handle this case well.

InfoZip will :

-
unzip ../Hello.zip
Archive:  ../Hello.zip
warning:  skipped "../" path component(s) in ../Hello.txt
  inflating: Hello.txt
replace Hello.txt? [y]es, [n]o, [A]ll, [N]one, [r]ename: r
new name: foo.txt
  inflating: foo.txt
% ls
Hello.txt   foo.txt
 % cat Hello.txt
Hello
 % cat foo.txt
Another Hello
%


This scenario is not well covered in the Zip spec, so we can do what we think 
is best.

 Personally, I am OK with resolving the path.  The odds of having multiple 
relative paths to the same file would be a mistake in creating the Zip.

That being said, if we want to follow Alan’s suggestion and throw an Exception, 
I am OK with that as well.

Either way, we currently cannot access the file via Zip FS due to the call to 
ZipPath::getResolvedPath() for all access and the path is only normalized when 
the Inodes are created.

Alan, do you have a specific preference?

One more datapoint

Consider:


try (FileSystem zipfs =
 FileSystems.newFileSystem(Path.of("ZipFSHello.zip"), 
Map.of("create", "true"))) {
Files.writeString(zipfs.getPath("HelloZipfs.txt"), "Hello");
Files.writeString(zipfs.getPath("../HelloZipfs.txt"), "A ZipFS Hello");

}


The above will result in a Zip with a single entry of “HelloZipfs.txt” and a 
value of “A ZipFS Hello”

As mentioned in my previous comment, this is due to the use of 
ZipPath::getResolvedPath() which brings me back to the suggestion of doing the 
same when we create the Inodes given this is an a virtual FS.

HTH

Best
Lance


-Jaikiran








Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: [jdk17] RFR: JDK-8225667: Clarify the behavior of System::gc w.r.t. reference processing [v2]

[Mandy Chung]

On Fri, 2 Jul 2021 08:58:35 GMT, Peter Levart  wrote:

>> Well, "determine" seems to have both meanings:
>> https://www.google.com/search?q=determine
>> So by the 2nd meaning, it is good.
>
> ... and considering the "Schrödinger's cat", even the 1st interpretation is 
> correct. You can't know whether the cat is dead or not until you "determine" 
> it's death. This does not mean that you killed it though.

I think "determine" sounds right to me.  "detect" is another possibility but 
does not apply well in "this effort" as the subject.   This is another way to 
explain it.

During the execution, there are a number of reachability decision points.  At 
each reachability decision point, the references are checked. GC determines if 
the reachability of the referent has changed to the value corresponding to the 
type of the reference (softly weakly, or phantom reachable), then it clears and 
enqueues the reference.

-

PR: https://git.openjdk.java.net/jdk17/pull/183

Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]

[Brian Burkhalter]

On Fri, 2 Jul 2021 16:50:11 GMT, Naoto Sato  wrote:

>> Brian Burkhalter has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   6766844: Correct error messages in test
>
> test/jdk/java/io/ByteArrayInputStream/ReadAllReadNTransferTo.java line 57:
> 
>> 55: }
>> 56: if (bais.read(new byte[1], 0, 0) != -1) {
>> 57: throw new RuntimeException("read(byte[],int,int) did not 
>> return 0");
> 
> Should these exception messages be "did not return -1"?

Oh you are correct. Thanks!

-

PR: https://git.openjdk.java.net/jdk17/pull/189

Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]

[Naoto Sato]

On Fri, 2 Jul 2021 16:55:18 GMT, Brian Burkhalter  wrote:

>> Modify the specification of 
>> `java.io.ByteArrayInputStream#read(byte[],int,int)` to indicate that `-1` is 
>> returned instead of `0` when the stream is at its end and the third 
>> parameter, `len`, is zero.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   6766844: Correct error messages in test

test/jdk/java/io/ByteArrayInputStream/ReadAllReadNTransferTo.java line 57:

> 55: }
> 56: if (bais.read(new byte[1], 0, 0) != -1) {
> 57: throw new RuntimeException("read(byte[],int,int) did not 
> return 0");

Should these exception messages be "did not return -1"?

-

PR: https://git.openjdk.java.net/jdk17/pull/189

Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]

[Brian Burkhalter]

> Modify the specification of 
> `java.io.ByteArrayInputStream#read(byte[],int,int)` to indicate that `-1` is 
> returned instead of `0` when the stream is at its end and the third 
> parameter, `len`, is zero.

Brian Burkhalter has updated the pull request incrementally with one additional 
commit since the last revision:

  6766844: Correct error messages in test

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/189/files
  - new: https://git.openjdk.java.net/jdk17/pull/189/files/5b89ab60..528afc5f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17=189=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=189=00-01

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/189.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/189/head:pull/189

PR: https://git.openjdk.java.net/jdk17/pull/189

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v4]

[Brian Burkhalter]

> Please consider this proposal to add a method 
> `unsignedMultiplyHigh(long,long)` to each of `java.lang.Math` and 
> `java.lang.StrictMath`.

Brian Burkhalter has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  8188044: Use multiplyHigh() to leverage the intrinsic

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4644/files
  - new: https://git.openjdk.java.net/jdk/pull/4644/files/2ad5e395..a499b2e5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4644=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4644=02-03

  Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4644.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4644/head:pull/4644

PR: https://git.openjdk.java.net/jdk/pull/4644

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v4]

[Andrew Haley]

On Fri, 2 Jul 2021 16:37:21 GMT, Brian Burkhalter  wrote:

>> Please consider this proposal to add a method 
>> `unsignedMultiplyHigh(long,long)` to each of `java.lang.Math` and 
>> `java.lang.StrictMath`.
>
> Brian Burkhalter has refreshed the contents of this pull request, and 
> previous commits have been removed. The incremental views will show 
> differences compared to the previous content of the PR. The pull request 
> contains one new commit since the last revision:
> 
>   8188044: Use multiplyHigh() to leverage the intrinsic

Marked as reviewed by aph (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4644

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v3]

[Brian Burkhalter]

> Please consider this proposal to add a method 
> `unsignedMultiplyHigh(long,long)` to each of `java.lang.Math` and 
> `java.lang.StrictMath`.

Brian Burkhalter has updated the pull request incrementally with one additional 
commit since the last revision:

  8188044: Use multiplyHigh() to gleverage the inverage the intrinsic

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4644/files
  - new: https://git.openjdk.java.net/jdk/pull/4644/files/0a8fec63..2ad5e395

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4644=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4644=01-02

  Stats: 20 lines in 2 files changed: 9 ins; 5 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4644.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4644/head:pull/4644

PR: https://git.openjdk.java.net/jdk/pull/4644

Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]

[Lance Andersen]

On Jul 2, 2021, at 8:08 AM, Jaikiran Pai 
mailto:jai.forums2...@gmail.com>> wrote:

Hello Lance,

On 02/07/21 4:42 pm, Lance Andersen wrote:
Hi Jaikiran,

Consider:


try (var os = Files.newOutputStream(ZIPFILE);
 ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
}


With your proposed fix, you will only return "/" when you walk the the Zip file 
and we should also return "/Hello.txt"

Thank you for noticing this issue in my change and bringing this up. I have a 
question around this use case. Please consider a small variation to your 
example as below:

try (var os = Files.newOutputStream(ZIPFILE);
 ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
zos.closeEntry();
zos.putNextEntry(new ZipEntry("/Hello.txt"));
zos.write("Bye bye".getBytes(StandardCharsets.UTF_8));
zos.closeEntry();

}

Notice that I now have a zip/jar which has 2 differently named entries 
"../Hello.txt" and "/Hello.txt". This creates the archive without any issues 
and those 2 entries are noted in its listing. Now assuming someone walks over 
this jar using the ZipFileSystem, starting at root ("/"), what should be the 
expected output? The path(s) returned will end up being "/" and "/Hello.txt" 
but which resource is expected to be served in this case? The one which has 
"Hello World" in it or the one which has "Bye bye"? This example can be 
extended a bit more by introducing a "./Hello.txt", in the jar, with yet 
another different content in that entry. Is there some specification for this 
that I can check and adapt the test case accordingly?


Consider Hello.zip created via:


try (var os = Files.newOutputStream(Path.of("Hello.zip"));
 ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello".getBytes(StandardCharsets.UTF_8));
zos.putNextEntry(new ZipEntry("Hello.txt"));
zos.write("Another Hello".getBytes(StandardCharsets.UTF_8));

}

Winzip does not handle this case well.

InfoZip will :

-
unzip ../Hello.zip
Archive:  ../Hello.zip
warning:  skipped "../" path component(s) in ../Hello.txt
  inflating: Hello.txt
replace Hello.txt? [y]es, [n]o, [A]ll, [N]one, [r]ename: r
new name: foo.txt
  inflating: foo.txt
% ls
Hello.txt   foo.txt
 % cat Hello.txt
Hello
 % cat foo.txt
Another Hello
%


This scenario is not well covered in the Zip spec, so we can do what we think 
is best.

 Personally, I am OK with resolving the path.  The odds of having multiple 
relative paths to the same file would be a mistake in creating the Zip.

That being said, if we want to follow Alan’s suggestion and throw an Exception, 
I am OK with that as well.

Either way, we currently cannot access the file via Zip FS due to the call to 
ZipPath::getResolvedPath() for all access and the path is only normalized when 
the Inodes are created.

Alan, do you have a specific preference?


-Jaikiran




[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v2]

[Andrew Haley]

On 7/2/21 4:30 PM, Raffaello Giulietti wrote:
> FWIW, adinn's branchless code together with
> PR https://git.openjdk.java.net/jdk/pull/4660
> make both methods less vulnerable to timing attacks.

I doubt it, because HotSpot generates conditional select instructions for
the popular systems, at least for C2.

I guess it might on a C1-only or pure interpreter system. That certainly
might be an argument for using the shift-and-add magic. With a comment
that would be fine, as the difference in performance doesn't seem to be
worth worrying about. We're looking at sub-nanosecond throughput.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v2]

[Andrew Dinn]

On 02/07/2021 16:30, Raffaello Giulietti wrote:

FWIW, adinn's branchless code together with
PR https://git.openjdk.java.net/jdk/pull/4660
make both methods less vulnerable to timing attacks.
That was indeed the point of the change. However, I doubt the difference 
in timing is going to be significant given Andrew's micro-benchmark results.


regards,


Andrew Dinn
---
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v2]

[Raffaello Giulietti]

FWIW, adinn's branchless code together with
PR https://git.openjdk.java.net/jdk/pull/4660
make both methods less vulnerable to timing attacks.


Greetings
Raffaello


On 2021-07-02 15:50, Andrew Haley wrote:

On Fri, 2 Jul 2021 11:06:06 GMT, Andrew Dinn  wrote:


You can also do that branchlessly which might prove better

```
  long result = Math.multiplyHigh(x, y);
  result += (y & (x >> 63));
  result += (x & (y >> 63));
  return result;
```

I doubt very much that it would be better, because these days branch prediction 
is excellent, and we also have conditional select instructions. Exposing the 
condition helps C2 to eliminate it if the range of args is known. The `if` code 
is easier to understand.

Benchmark results, with one of the operands changing signs every iteration, 
1000 iterations:


Benchmark  Mode  Cnt ScoreError  Units
MulHiTest.mulHiTest1   (aph) avgt3  1570.587 ± 16.602  ns/op
MulHiTest.mulHiTest2   (adinn)   avgt3  2237.637 ±  4.740  ns/op

In any case, note that with this optimization the unsigned mulHi is in the 
nanosecond range, so Good Enough. IMO.

-

PR: https://git.openjdk.java.net/jdk/pull/4644

Re: RFR: 8269124: Update java.time to use switch expressions (part II) [v6]

[Patrick Concannon]

> Hi,
> 
> Could someone please review the second half of my update for the `java.time` 
> package to make use of switch expressions?
> 
> This PR was split into two parts due to the large number of files affected.
> 
> Kind regards,
> 
> Patrick

Patrick Concannon has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains seven additional 
commits since the last revision:

 - Merge remote-tracking branch 'origin/master' into JDK-8269124
 - Merge branch 'JDK-8269124' of github.com:pconcannon/jdk into JDK-8269124
 - 8269124: Added instanceof pattern variables
 - Merge remote-tracking branch 'origin/master' into JDK-8269124
 - 8269124: Added missing return
 - 8269124: Added missing brace; fixed build issue
 - 8269124: Update java.time to use switch expressions (part II)

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4552/files
  - new: https://git.openjdk.java.net/jdk/pull/4552/files/3a1b7161..8dede463

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4552=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4552=04-05

  Stats: 18116 lines in 555 files changed: 8086 ins; 8092 del; 1938 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4552.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4552/head:pull/4552

PR: https://git.openjdk.java.net/jdk/pull/4552

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v2]

[Andrew Haley]

On 7/2/21 12:26 PM, Raffaello Giulietti wrote:
> ... or even as a one liner, like in the test
> 
>   return Math.multiplyHigh(x, y) + ((x >> (Long.SIZE - 1)) & y) + ((y >> 
> (Long.SIZE - 1)) & x);

It was hard to write, so it should be hard to read too! :-)

[jdk17] Integrated: 8269543: The warning for System::setSecurityManager should only appear once for each caller

[Weijun Wang]

On Mon, 28 Jun 2021 21:09:43 GMT, Weijun Wang  wrote:

> Add a cache to record which sources have called `System::setSecurityManager` 
> and only print out warning lines once for each.

This pull request has now been integrated.

Changeset: c4ea13ed
Author:Weijun Wang 
URL:   
https://git.openjdk.java.net/jdk17/commit/c4ea13edd036bd6aeb213bb5391dd374d283d382
Stats: 59 lines in 2 files changed: 40 ins; 6 del; 13 mod

8269543: The warning for System::setSecurityManager should only appear once for 
each caller

Reviewed-by: lancea, alanb, dfuchs

-

PR: https://git.openjdk.java.net/jdk17/pull/166

RE: Methods removeFirstIf in Collection and skipFirst in Stream

[Alberto Otero Rodríguez]

Sorry for the links, the methods would be:

1) In Collection:
   default boolean removeFirstIf​(Predicatehttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/Collection.html>>
 filter)

2) In Stream:
   Stream skipFirst(Predicatehttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/Stream.html>>
 predicate)

De: Alberto Otero Rodríguez
Enviado: viernes, 2 de julio de 2021 16:22
Para: core-libs-dev@openjdk.java.net 
Asunto: Methods removeFirstIf in Collection and skipFirst in Stream

Hi,

I think it would be interesting adding the following methods:

1) In Collection:
   default boolean 
removeFirstIf​(Predicatehttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/Collection.html>>
 filter)

2) In Stream:
   
Streamhttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/Stream.html>>
 
skipFirst(Predicatehttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/Stream.html>>
 predicate)

The purpose of both methods would be the same. Deleting the first element in 
the collection that fulfills the predicate.

This could be useful in collections/streams with no duplicate elements where 
performance would be better than using removeIf from Collection of filter from 
Stream.

Regards,

Alberto.

Methods removeFirstIf in Collection and skipFirst in Stream

[Alberto Otero Rodríguez]

Hi,

I think it would be interesting adding the following methods:

1) In Collection:
   default boolean 
removeFirstIf​(Predicatehttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/Collection.html>>
 filter)

2) In Stream:
   
Streamhttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/Stream.html>>
 
skipFirst(Predicatehttps://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/stream/Stream.html>>
 predicate)

The purpose of both methods would be the same. Deleting the first element in 
the collection that fulfills the predicate.

This could be useful in collections/streams with no duplicate elements where 
performance would be better than using removeIf from Collection of filter from 
Stream.

Regards,

Alberto.

Re: RFR: 8188046: java.lang.Math.mutliplyHigh does not run in constant time

[Roger Riggs]

On Thu, 1 Jul 2021 23:46:00 GMT, Brian Burkhalter  wrote:

> Please consider this proposal to remove the fast path in 
> `java.lang.Math.multiplyHigh()` the small performance improvement of which is 
> eclipsed by the branch penalty.

Marked as reviewed by rriggs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4660

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v2]

[Andrew Haley]

On Fri, 2 Jul 2021 13:47:40 GMT, Andrew Haley  wrote:

>> You can also do that branchlessly which might prove better
>> 
>>  long result = Math.multiplyHigh(x, y);
>>  result += (y & (x >> 63));
>>  result += (x & (y >> 63));
>>  return result;
>
>> You can also do that branchlessly which might prove better
>> 
>> ```
>>  long result = Math.multiplyHigh(x, y);
>>  result += (y & (x >> 63));
>>  result += (x & (y >> 63));
>>  return result;
>> ```
> I doubt very much that it would be better, because these days branch 
> prediction is excellent, and we also have conditional select instructions. 
> Exposing the condition helps C2 to eliminate it if the range of args is 
> known. The `if` code is easier to understand.
> 
> Benchmark results, with one of the operands changing signs every iteration, 
> 1000 iterations:
> 
> 
> Benchmark  Mode  Cnt ScoreError  Units
> MulHiTest.mulHiTest1   (aph) avgt3  1570.587 ± 16.602  ns/op
> MulHiTest.mulHiTest2   (adinn)   avgt3  2237.637 ±  4.740  ns/op
> 
> In any case, note that with this optimization the unsigned mulHi is in the 
> nanosecond range, so Good Enough. IMO.

But weirdly, it's the other way around on AArch64, but there's little in it:


Benchmark Mode  Cnt Score   Error  Units
MulHiTest.mulHiTest1  avgt3  1492.108 ± 0.301  ns/op
MulHiTest.mulHiTest2  avgt3  1219.521 ± 1.516  ns/op


but this is only in the case where we have unpredictable branches. Go with 
simple and easy to understand; it doesn't much matter.

-

PR: https://git.openjdk.java.net/jdk/pull/4644

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v2]

[Andrew Haley]

On Fri, 2 Jul 2021 11:06:06 GMT, Andrew Dinn  wrote:

> You can also do that branchlessly which might prove better
> 
> ```
>  long result = Math.multiplyHigh(x, y);
>  result += (y & (x >> 63));
>  result += (x & (y >> 63));
>  return result;
> ```
I doubt very much that it would be better, because these days branch prediction 
is excellent, and we also have conditional select instructions. Exposing the 
condition helps C2 to eliminate it if the range of args is known. The `if` code 
is easier to understand.

Benchmark results, with one of the operands changing signs every iteration, 
1000 iterations:


Benchmark  Mode  Cnt ScoreError  Units
MulHiTest.mulHiTest1   (aph) avgt3  1570.587 ± 16.602  ns/op
MulHiTest.mulHiTest2   (adinn)   avgt3  2237.637 ±  4.740  ns/op

In any case, note that with this optimization the unsigned mulHi is in the 
nanosecond range, so Good Enough. IMO.

-

PR: https://git.openjdk.java.net/jdk/pull/4644

Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]

[Alan Bateman]

On 02/07/2021 13:08, Jaikiran Pai wrote:


Thank you for noticing this issue in my change and bringing this up. I 
have a question around this use case. Please consider a small 
variation to your example as below:


try (var os = Files.newOutputStream(ZIPFILE);
 ZipOutputStream zos = new ZipOutputStream(os)) {
    zos.putNextEntry(new ZipEntry("../Hello.txt"));
    zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
    zos.closeEntry();
    zos.putNextEntry(new ZipEntry("/Hello.txt"));
            zos.write("Bye bye".getBytes(StandardCharsets.UTF_8));
            zos.closeEntry();

    }

Notice that I now have a zip/jar which has 2 differently named entries 
"../Hello.txt" and "/Hello.txt". This creates the archive without any 
issues and those 2 entries are noted in its listing. Now assuming 
someone walks over this jar using the ZipFileSystem, starting at root 
("/"), what should be the expected output? The path(s) returned will 
end up being "/" and "/Hello.txt" but which resource is expected to be 
served in this case? The one which has "Hello World" in it or the one 
which has "Bye bye"? This example can be extended a bit more by 
introducing a "./Hello.txt", in the jar, with yet another different 
content in that entry. Is there some specification for this that I can 
check and adapt the test case accordingly?


This is an area where treating a zip file as a virtual file system 
doesn't quite work. We may have to look at zipfs ignoring entries that 
contain "." or ".." name elements or have it throw an exception if they 
are encountered. I think the main thing is that the APIs and access 
behaves consistently.


-Alan

Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller [v2]

[Daniel Fuchs]

On Wed, 30 Jun 2021 15:45:25 GMT, Weijun Wang  wrote:

>> Add a cache to record which sources have called `System::setSecurityManager` 
>> and only print out warning lines once for each.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update cache key from String to Class

Marked as reviewed by dfuchs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/166

Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]

[Jaikiran Pai]

Hello Lance,

On 02/07/21 4:42 pm, Lance Andersen wrote:

Hi Jaikiran,

Consider:


 try (var os = Files.newOutputStream(ZIPFILE);
  ZipOutputStream zos = new ZipOutputStream(os)) {
 zos.putNextEntry(new ZipEntry("../Hello.txt"));
 zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
 }


With your proposed fix, you will only return "/" when you walk the the Zip file and we 
should also return "/Hello.txt"


Thank you for noticing this issue in my change and bringing this up. I 
have a question around this use case. Please consider a small variation 
to your example as below:


try (var os = Files.newOutputStream(ZIPFILE);
 ZipOutputStream zos = new ZipOutputStream(os)) {
    zos.putNextEntry(new ZipEntry("../Hello.txt"));
    zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
    zos.closeEntry();
    zos.putNextEntry(new ZipEntry("/Hello.txt"));
            zos.write("Bye bye".getBytes(StandardCharsets.UTF_8));
            zos.closeEntry();

    }

Notice that I now have a zip/jar which has 2 differently named entries 
"../Hello.txt" and "/Hello.txt". This creates the archive without any 
issues and those 2 entries are noted in its listing. Now assuming 
someone walks over this jar using the ZipFileSystem, starting at root 
("/"), what should be the expected output? The path(s) returned will end 
up being "/" and "/Hello.txt" but which resource is expected to be 
served in this case? The one which has "Hello World" in it or the one 
which has "Bye bye"? This example can be extended a bit more by 
introducing a "./Hello.txt", in the jar, with yet another different 
content in that entry. Is there some specification for this that I can 
check and adapt the test case accordingly?


-Jaikiran

Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller [v2]

[Alan Bateman]

On Wed, 30 Jun 2021 15:45:25 GMT, Weijun Wang  wrote:

>> Add a cache to record which sources have called `System::setSecurityManager` 
>> and only print out warning lines once for each.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update cache key from String to Class

Marked as reviewed by alanb (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/166

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v2]

[Raffaello Giulietti]

... or even as a one liner, like in the test

	return Math.multiplyHigh(x, y) + ((x >> (Long.SIZE - 1)) & y) + ((y >> 
(Long.SIZE - 1)) & x);




On 2021-07-02 13:08, Andrew Dinn wrote:

On Fri, 2 Jul 2021 09:39:46 GMT, Andrew Haley  wrote:


src/java.base/share/classes/java/lang/Math.java line 1211:


1209: long z1 = t >>> 32;
1210:
1211: return x1 * y1 + z1 + (z0 >>> 32);


Suggestion:

 long result = Math.multiplyHigh(x, y);
 if (x < 0) result += y;
 if (y < 0) result += x;
 return result;


This is just subtracting the 2's-complement offset. I guess the idea, longer 
term, is that this be an intrinsic anyway, but if you do `unsignedMultiplyHigh` 
this way you'll utilize the existing `multiplyHigh` intrinsic on all platforms 
that have it.


You can also do that branchlessly which might prove better

  long result = Math.multiplyHigh(x, y);
  result += (y & (x >> 63));
  result += (x & (y >> 63));
  return result;

-

PR: https://git.openjdk.java.net/jdk/pull/4644

Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]

[Lance Andersen]

On Fri, 2 Jul 2021 11:06:40 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this proposed fix for 
>> https://bugs.openjdk.java.net/browse/JDK-8251329?
>> 
>> As noted in that issue, if a zip filesystem created on top of a jar 
>> containing a "./" entry is "walked" using the `Files.walkFileTree`, it leads 
>> to a infinite never ending iteration (which ultimately fails with Java heap 
>> space OOM).
>> 
>> Alan notes in that issue that:
>> 
>>> This is more likely an issue with the zipfs DirectoryStream implementation. 
>>> A DirectoryStream is specified to not include elements that for the special 
>>> links to the current or parent directory. It should be rare. 
>> 
>> This indeed turned out to be an issue in the 
>> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` implementation where it calls 
>> the `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` implementation.  The 
>> implementation, unlike what the javadoc of `java.nio.file.DirectoryStream` 
>> states, was including the "." and ".." paths in its returned iterator:
>> 
>>> The elements returned by the iterator are in no specific order. Some file
>>  systems maintain special links to the directory itself and the directory's
>>   parent directory. Entries representing these links are not returned by the
>>   iterator.
>> 
>> 
>> The proposed fix in this patch checks the paths for "." and "..", similar to 
>> what the `sun.nio.fs.UnixDirectoryStream` does in its `isSelfOrParent` and 
>> skips those paths from being added into the returned iterator. The 
>> `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` (where this change has been 
>> done) is currently only used by 
>> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` and has package-private 
>> visibility, so this change shouldn't impact any other usage/expectations.
>> 
>> A new jtreg test has been added to reproduce this issue and verify the fix. 
>> Local testing of the `test/jdk/jdk/nio/` (including this new test) went fine 
>> without any issues after this change. I will be triggering a `tier1` test 
>> locally in a while.
>
> Jaikiran Pai has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains four additional 
> commits since the last revision:
> 
>  - Merge latest from upstream master branch to bring in fixes that might help 
> fix the unrelated tier1 failures in Github Actions job runs
>  - implement review suggestions:
> - reduce the toString() calls by creating a helper
> - fs.getPath("/") instead of fs.getRootDirectories().iterator().next()
>  - implement review suggestion - move isSelfOrParent to ZipPath class
>  - 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named 
> "." inside

Hi Jaikiran,

Consider:


try (var os = Files.newOutputStream(ZIPFILE);
 ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
}


With your proposed fix, you will only return "/" when you walk the the Zip file 
and we should also return "/Hello.txt"

If. you resolve the path when the Inode entry is created:

   ` name(new ZipPath(null, normalize(name), true).getResolvedPath());`

That should address the issue and also allow you to access the entry.

-

PR: https://git.openjdk.java.net/jdk/pull/4604

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v2]

[Andrew Dinn]

On Fri, 2 Jul 2021 09:39:46 GMT, Andrew Haley  wrote:

>> src/java.base/share/classes/java/lang/Math.java line 1211:
>> 
>>> 1209: long z1 = t >>> 32;
>>> 1210: 
>>> 1211: return x1 * y1 + z1 + (z0 >>> 32);
>> 
>> Suggestion:
>> 
>> long result = Math.multiplyHigh(x, y);
>> if (x < 0) result += y;
>> if (y < 0) result += x;
>> return result;
>
> This is just subtracting the 2's-complement offset. I guess the idea, longer 
> term, is that this be an intrinsic anyway, but if you do 
> `unsignedMultiplyHigh` this way you'll utilize the existing `multiplyHigh` 
> intrinsic on all platforms that have it.

You can also do that branchlessly which might prove better

 long result = Math.multiplyHigh(x, y);
 result += (y & (x >> 63));
 result += (x & (y >> 63));
 return result;

-

PR: https://git.openjdk.java.net/jdk/pull/4644

Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]

[Jaikiran Pai]

> Can I please get a review of this proposed fix for 
> https://bugs.openjdk.java.net/browse/JDK-8251329?
> 
> As noted in that issue, if a zip filesystem created on top of a jar 
> containing a "./" entry is "walked" using the `Files.walkFileTree`, it leads 
> to a infinite never ending iteration (which ultimately fails with Java heap 
> space OOM).
> 
> Alan notes in that issue that:
> 
>> This is more likely an issue with the zipfs DirectoryStream implementation. 
>> A DirectoryStream is specified to not include elements that for the special 
>> links to the current or parent directory. It should be rare. 
> 
> This indeed turned out to be an issue in the 
> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` implementation where it calls 
> the `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` implementation.  The 
> implementation, unlike what the javadoc of `java.nio.file.DirectoryStream` 
> states, was including the "." and ".." paths in its returned iterator:
> 
>> The elements returned by the iterator are in no specific order. Some file
>  systems maintain special links to the directory itself and the directory's
>   parent directory. Entries representing these links are not returned by the
>   iterator.
> 
> 
> The proposed fix in this patch checks the paths for "." and "..", similar to 
> what the `sun.nio.fs.UnixDirectoryStream` does in its `isSelfOrParent` and 
> skips those paths from being added into the returned iterator. The 
> `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` (where this change has been 
> done) is currently only used by `jdk.nio.zipfs.ZipDirectoryStream#iterator()` 
> and has package-private visibility, so this change shouldn't impact any other 
> usage/expectations.
> 
> A new jtreg test has been added to reproduce this issue and verify the fix. 
> Local testing of the `test/jdk/jdk/nio/` (including this new test) went fine 
> without any issues after this change. I will be triggering a `tier1` test 
> locally in a while.

Jaikiran Pai has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains four additional commits since 
the last revision:

 - Merge latest from upstream master branch to bring in fixes that might help 
fix the unrelated tier1 failures in Github Actions job runs
 - implement review suggestions:
- reduce the toString() calls by creating a helper
- fs.getPath("/") instead of fs.getRootDirectories().iterator().next()
 - implement review suggestion - move isSelfOrParent to ZipPath class
 - 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named 
"." inside

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4604/files
  - new: https://git.openjdk.java.net/jdk/pull/4604/files/8e77d289..abae52bb

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4604=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4604=02-03

  Stats: 10322 lines in 336 files changed: 6220 ins; 2802 del; 1300 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4604.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4604/head:pull/4604

PR: https://git.openjdk.java.net/jdk/pull/4604

Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v3]

[Jaikiran Pai]

On Fri, 2 Jul 2021 10:25:29 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this proposed fix for 
>> https://bugs.openjdk.java.net/browse/JDK-8251329?
>> 
>> As noted in that issue, if a zip filesystem created on top of a jar 
>> containing a "./" entry is "walked" using the `Files.walkFileTree`, it leads 
>> to a infinite never ending iteration (which ultimately fails with Java heap 
>> space OOM).
>> 
>> Alan notes in that issue that:
>> 
>>> This is more likely an issue with the zipfs DirectoryStream implementation. 
>>> A DirectoryStream is specified to not include elements that for the special 
>>> links to the current or parent directory. It should be rare. 
>> 
>> This indeed turned out to be an issue in the 
>> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` implementation where it calls 
>> the `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` implementation.  The 
>> implementation, unlike what the javadoc of `java.nio.file.DirectoryStream` 
>> states, was including the "." and ".." paths in its returned iterator:
>> 
>>> The elements returned by the iterator are in no specific order. Some file
>>  systems maintain special links to the directory itself and the directory's
>>   parent directory. Entries representing these links are not returned by the
>>   iterator.
>> 
>> 
>> The proposed fix in this patch checks the paths for "." and "..", similar to 
>> what the `sun.nio.fs.UnixDirectoryStream` does in its `isSelfOrParent` and 
>> skips those paths from being added into the returned iterator. The 
>> `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` (where this change has been 
>> done) is currently only used by 
>> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` and has package-private 
>> visibility, so this change shouldn't impact any other usage/expectations.
>> 
>> A new jtreg test has been added to reproduce this issue and verify the fix. 
>> Local testing of the `test/jdk/jdk/nio/` (including this new test) went fine 
>> without any issues after this change. I will be triggering a `tier1` test 
>> locally in a while.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   implement review suggestions:
>- reduce the toString() calls by creating a helper
>- fs.getPath("/") instead of fs.getRootDirectories().iterator().next()

>I skimmed the test. A helper method that maps a Set to a modifiable Set would 
>avoid the 10 usages of toString() in the setup, just a suggestion.

Done. I introduced a helper in the updated version of the PR.

> Also I was puzzled by fs.getRootDirectories().iterator().next() and why this 
> isn't fs.getPath("/").

When I started with a reproducer and then this test, I used the code that the 
reporter had attached in the JBS issue. That one had used 
"fs.getRootDirectories().iterator().next()" to show this issue. I just happened 
to carry that over into the test. I've updated the PR to now use 
fs.getPath("/") and also verified that this version of the test continues to 
fail without the fix proposed in this PR and passes with the changes.

-

PR: https://git.openjdk.java.net/jdk/pull/4604

Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v3]

[Jaikiran Pai]

> Can I please get a review of this proposed fix for 
> https://bugs.openjdk.java.net/browse/JDK-8251329?
> 
> As noted in that issue, if a zip filesystem created on top of a jar 
> containing a "./" entry is "walked" using the `Files.walkFileTree`, it leads 
> to a infinite never ending iteration (which ultimately fails with Java heap 
> space OOM).
> 
> Alan notes in that issue that:
> 
>> This is more likely an issue with the zipfs DirectoryStream implementation. 
>> A DirectoryStream is specified to not include elements that for the special 
>> links to the current or parent directory. It should be rare. 
> 
> This indeed turned out to be an issue in the 
> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` implementation where it calls 
> the `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` implementation.  The 
> implementation, unlike what the javadoc of `java.nio.file.DirectoryStream` 
> states, was including the "." and ".." paths in its returned iterator:
> 
>> The elements returned by the iterator are in no specific order. Some file
>  systems maintain special links to the directory itself and the directory's
>   parent directory. Entries representing these links are not returned by the
>   iterator.
> 
> 
> The proposed fix in this patch checks the paths for "." and "..", similar to 
> what the `sun.nio.fs.UnixDirectoryStream` does in its `isSelfOrParent` and 
> skips those paths from being added into the returned iterator. The 
> `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` (where this change has been 
> done) is currently only used by `jdk.nio.zipfs.ZipDirectoryStream#iterator()` 
> and has package-private visibility, so this change shouldn't impact any other 
> usage/expectations.
> 
> A new jtreg test has been added to reproduce this issue and verify the fix. 
> Local testing of the `test/jdk/jdk/nio/` (including this new test) went fine 
> without any issues after this change. I will be triggering a `tier1` test 
> locally in a while.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  implement review suggestions:
   - reduce the toString() calls by creating a helper
   - fs.getPath("/") instead of fs.getRootDirectories().iterator().next()

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4604/files
  - new: https://git.openjdk.java.net/jdk/pull/4604/files/3971ad6f..8e77d289

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4604=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4604=01-02

  Stats: 18 lines in 1 file changed: 6 ins; 0 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4604.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4604/head:pull/4604

PR: https://git.openjdk.java.net/jdk/pull/4604

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v2]

[Andrew Haley]

On Fri, 2 Jul 2021 09:37:47 GMT, Andrew Haley  wrote:

>> Brian Burkhalter has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8188044: Add @see links between multiplyHigh() and unsignedMultiplyHigh().
>
> src/java.base/share/classes/java/lang/Math.java line 1211:
> 
>> 1209: long z1 = t >>> 32;
>> 1210: 
>> 1211: return x1 * y1 + z1 + (z0 >>> 32);
> 
> Suggestion:
> 
> long result = Math.multiplyHigh(x, y);
> if (x < 0) result += y;
> if (y < 0) result += x;
> return result;

This is just subtracting the 2's-complement offset. I guess the idea, longer 
term, is that this be an intrinsic anyway, but if you do `unsignedMultiplyHigh` 
this way you'll utilize the existing `multiplyHigh` intrinsic on all platforms 
that have it.

-

PR: https://git.openjdk.java.net/jdk/pull/4644

Re: RFR: 8188044: We need Math.unsignedMultiplyHigh [v2]

[Andrew Haley]

On Thu, 1 Jul 2021 18:08:22 GMT, Brian Burkhalter  wrote:

>> Please consider this proposal to add a method 
>> `unsignedMultiplyHigh(long,long)` to each of `java.lang.Math` and 
>> `java.lang.StrictMath`.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8188044: Add @see links between multiplyHigh() and unsignedMultiplyHigh().

src/java.base/share/classes/java/lang/Math.java line 1211:

> 1209: long z1 = t >>> 32;
> 1210: 
> 1211: return x1 * y1 + z1 + (z0 >>> 32);

Suggestion:

long result = Math.multiplyHigh(x, y);
if (x < 0) result += y;
if (y < 0) result += x;
return result;

-

PR: https://git.openjdk.java.net/jdk/pull/4644

Re: [jdk17] RFR: JDK-8225667: Clarify the behavior of System::gc w.r.t. reference processing [v2]

[Peter Levart]

On Fri, 2 Jul 2021 08:52:28 GMT, Peter Levart  wrote:

>> src/java.base/share/classes/java/lang/Runtime.java line 662:
>> 
>>> 660:  * or that any particular number of {@link java.lang.ref.Reference 
>>> Reference}
>>> 661:  * objects will be cleared and enqueued.
>>> 662:  * 
>> 
>> Hi Mandy,
>> I'm not a native speaker so this might be wrong thinking: The word 
>> "determine" feels to me like saying "cause". But running System.gc never 
>> actually causes the change of reachability (well it does, when the Reference 
>> object is cleared, the reachability of referent changes from 
>> Weakly/Softly/Phantom-reachable to unreachable, but I don't think you had 
>> that in mind. Or did you?). What would you say about using word "detect" 
>> instead? Please others, do say if my thinking is wrong...
>
> Well, "determine" seems to have both meanings:
> https://www.google.com/search?q=determine
> So by the 2nd meaning, it is good.

... and considering the "Schrödinger's cat", even the 1st interpretation is 
correct. You can't know whether the cat is dead or not until you "determine" 
it's death. This does not mean that you killed it though.

-

PR: https://git.openjdk.java.net/jdk17/pull/183

Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v2]

[Alan Bateman]

On Thu, 1 Jul 2021 13:25:32 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this proposed fix for 
>> https://bugs.openjdk.java.net/browse/JDK-8251329?
>> 
>> As noted in that issue, if a zip filesystem created on top of a jar 
>> containing a "./" entry is "walked" using the `Files.walkFileTree`, it leads 
>> to a infinite never ending iteration (which ultimately fails with Java heap 
>> space OOM).
>> 
>> Alan notes in that issue that:
>> 
>>> This is more likely an issue with the zipfs DirectoryStream implementation. 
>>> A DirectoryStream is specified to not include elements that for the special 
>>> links to the current or parent directory. It should be rare. 
>> 
>> This indeed turned out to be an issue in the 
>> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` implementation where it calls 
>> the `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` implementation.  The 
>> implementation, unlike what the javadoc of `java.nio.file.DirectoryStream` 
>> states, was including the "." and ".." paths in its returned iterator:
>> 
>>> The elements returned by the iterator are in no specific order. Some file
>>  systems maintain special links to the directory itself and the directory's
>>   parent directory. Entries representing these links are not returned by the
>>   iterator.
>> 
>> 
>> The proposed fix in this patch checks the paths for "." and "..", similar to 
>> what the `sun.nio.fs.UnixDirectoryStream` does in its `isSelfOrParent` and 
>> skips those paths from being added into the returned iterator. The 
>> `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` (where this change has been 
>> done) is currently only used by 
>> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` and has package-private 
>> visibility, so this change shouldn't impact any other usage/expectations.
>> 
>> A new jtreg test has been added to reproduce this issue and verify the fix. 
>> Local testing of the `test/jdk/jdk/nio/` (including this new test) went fine 
>> without any issues after this change. I will be triggering a `tier1` test 
>> locally in a while.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   implement review suggestion - move isSelfOrParent to ZipPath class

The implementation change look good.

I skimmed the test. A helper method that maps a Set to a modifiable 
Set would avoid the 10 usages of toString() in the setup, just a 
suggestion. Also I was puzzled by fs.getRootDirectories().iterator().next() and 
why this isn't fs.getPath("/").

-

PR: https://git.openjdk.java.net/jdk/pull/4604

Re: RFR: 8266972: Use String.concat() in j.l.Class where invokedynamic-based String concatenation is not available [v4]

[Сергей Цыпанов]

> Hello, from discussion in https://github.com/openjdk/jdk/pull/3464 I've found 
> out, that in a few of JDK core classes, e.g. in `j.l.Class` expressions like 
> `baseName.replace('.', '/') + '/' + name` are not compiled into 
> `invokedynamic`-based code, but into one using `StringBuilder`. This happens 
> due to some bootstraping issues.
> 
> However the code like
> 
> private String getSimpleName0() {
> if (isArray()) {
> return getComponentType().getSimpleName() + "[]";
> }
> //...
> }
> 
> can be improved via replacement of `+` with `String.concat()` call.
> 
> I've used this benchmark to measure impact:
> 
> @State(Scope.Thread)
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class ClassMethodsBenchmark {
>   private final Class arrayClass = Object[].class;
>   private Method descriptorString;
> 
>   @Setup
>   public void setUp() throws NoSuchMethodException {
> //fore some reason compiler doesn't allow me to call 
> Class.descriptorString() directly
> descriptorString = Class.class.getDeclaredMethod("descriptorString");
>   }
> 
>   @Benchmark
>   public Object descriptorString() throws Exception {
> return descriptorString.invoke(arrayClass);
>   }
> 
>   @Benchmark
>   public Object typeName() {
> return arrayClass.getTypeName();
>   }
> }
> 
> and got those results
> 
>Mode  Cnt Score 
> Error   Units
> descriptorString   avgt   6091.480 ±   
> 1.839   ns/op
> descriptorString:·gc.alloc.rate.norm   avgt   60   404.008 ±   
> 4.033B/op
> descriptorString:·gc.churn.G1_Eden_Space   avgt   60  2791.866 ± 
> 181.589  MB/sec
> descriptorString:·gc.churn.G1_Eden_Space.norm  avgt   60   401.702 ±  
> 26.047B/op
> descriptorString:·gc.churn.G1_Survivor_Space   avgt   60 0.003 ±   
> 0.001  MB/sec
> descriptorString:·gc.churn.G1_Survivor_Space.norm  avgt   60≈ 10⁻³
>   B/op
> descriptorString:·gc.count avgt   60   205.000
> counts
> descriptorString:·gc.time  avgt   60   216.000
> ms
> 
> patched
>Mode  Cnt Score 
> Error   Units
> descriptorString   avgt   6065.016 ±   
> 3.446   ns/op
> descriptorString:·gc.alloc.rateavgt   60  2844.240 ± 
> 115.719  MB/sec
> descriptorString:·gc.alloc.rate.norm   avgt   60   288.006 ±   
> 0.001B/op
> descriptorString:·gc.churn.G1_Eden_Space   avgt   60  2832.996 ± 
> 206.939  MB/sec
> descriptorString:·gc.churn.G1_Eden_Space.norm  avgt   60   286.955 ±  
> 17.853B/op
> descriptorString:·gc.churn.G1_Survivor_Space   avgt   60 0.003 ±   
> 0.001  MB/sec
> descriptorString:·gc.churn.G1_Survivor_Space.norm  avgt   60≈ 10⁻³
>   B/op
> descriptorString:·gc.count avgt   60   208.000
> counts
> descriptorString:·gc.time  avgt   60   228.000
> ms
> -
> typeName   avgt   6034.273 ±   
> 0.480   ns/op
> typeName:·gc.alloc.rateavgt   60  3265.356 ±  
> 45.113  MB/sec
> typeName:·gc.alloc.rate.norm   avgt   60   176.004 ±   
> 0.001B/op
> typeName:·gc.churn.G1_Eden_Space   avgt   60  3268.454 ± 
> 134.458  MB/sec
> typeName:·gc.churn.G1_Eden_Space.norm  avgt   60   176.109 ±   
> 6.595B/op
> typeName:·gc.churn.G1_Survivor_Space   avgt   60 0.003 ±   
> 0.001  MB/sec
> typeName:·gc.churn.G1_Survivor_Space.norm  avgt   60≈ 10⁻⁴
>   B/op
> typeName:·gc.count avgt   60   240.000
> counts
> typeName:·gc.time  avgt   60   255.000
> ms
> 
> patched
> 
> typeName   avgt   6015.787 ±   
> 0.214   ns/op
> typeName:·gc.alloc.rateavgt   60  2577.554 ±  
> 32.339  MB/sec
> typeName:·gc.alloc.rate.norm   avgt   6064.001 ±   
> 0.001B/op
> typeName:·gc.churn.G1_Eden_Space   avgt   60  2574.039 ± 
> 147.774  MB/sec
> typeName:·gc.churn.G1_Eden_Space.norm  avgt   6063.895 ±   
> 3.511B/op
> typeName:·gc.churn.G1_Survivor_Space   avgt   60 0.003 ±   
> 0.001  MB/sec
> typeName:·gc.churn.G1_Survivor_Space.norm  avgt   60≈ 10⁻⁴
>   B/op
> typeName:·gc.count avgt   60   189.000
> counts
> typeName:·gc.time  avgt   60   199.000
> ms
> 
> I think this patch is likely to improve 

Re: [jdk17] RFR: JDK-8225667: Clarify the behavior of System::gc w.r.t. reference processing [v2]

[Peter Levart]

On Fri, 2 Jul 2021 08:40:42 GMT, Peter Levart  wrote:

>> Mandy Chung has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Kim's suggestion on the wording
>
> src/java.base/share/classes/java/lang/Runtime.java line 662:
> 
>> 660:  * or that any particular number of {@link java.lang.ref.Reference 
>> Reference}
>> 661:  * objects will be cleared and enqueued.
>> 662:  * 
> 
> Hi Mandy,
> I'm not a native speaker so this might be wrong thinking: The word 
> "determine" feels to me like saying "cause". But running System.gc never 
> actually causes the change of reachability (well it does, when the Reference 
> object is cleared, the reachability of referent changes from 
> Weakly/Softly/Phantom-reachable to unreachable, but I don't think you had 
> that in mind. Or did you?). What would you say about using word "detect" 
> instead? Please others, do say if my thinking is wrong...

Well, "determine" seems to have both meanings:
https://www.google.com/search?q=determine
So by the 2nd meaning, it is good.

-

PR: https://git.openjdk.java.net/jdk17/pull/183

Re: [jdk17] RFR: JDK-8225667: Clarify the behavior of System::gc w.r.t. reference processing [v2]

[Peter Levart]

On Wed, 30 Jun 2021 22:52:36 GMT, Mandy Chung  wrote:

>> This spec clarification is a follow-up to 
>> [JDK-8224760](https://bugs.openjdk.java.net/browse/JDK-8224760?focusedCommentId=14268320=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14268320)
>>  w.r.t. reference processing.  Since there is no guarantee for any memory 
>> reclamation by the invocation of `System::gc`, the spec should also clarify 
>> that there is no guarantee in determining the change of reachability of any 
>> objects or any particular number of `Reference` objects be enqueued and 
>> cleared.
>> 
>> CSR:
>>https://bugs.openjdk.java.net/browse/JDK-8269690
>
> Mandy Chung has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Kim's suggestion on the wording

src/java.base/share/classes/java/lang/Runtime.java line 662:

> 660:  * or that any particular number of {@link java.lang.ref.Reference 
> Reference}
> 661:  * objects will be cleared and enqueued.
> 662:  * 

Hi Mandy,
I'm not a native speaker so this might be wrong thinking: The word "determine" 
feels to me like saying "cause". But running System.gc never actually causes 
the change of reachability (well it does, when the Reference object is cleared, 
the reachability of referent changes from Weakly/Softly/Phantom-reachable to 
unreachable, but I don't think you had that in mind. Or did you?). What would 
you say about using word "detect" instead? Please others, do say if my thinking 
is wrong...

-

PR: https://git.openjdk.java.net/jdk17/pull/183

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v2]

[Markus KARG]

On Thu, 1 Jul 2021 21:59:33 GMT, Markus KARG 
 wrote:

>> This PR-*draft* is **work in progress** and an invitation to discuss a 
>> possible solution for issue 
>> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
>> yet* intended for a final review.
>> 
>> As proposed in JDK-8265891, this PR provides an implementation for 
>> `Channels.newInputStream().transferTo()` which provide superior performance 
>> compared to the current implementation. The changes are:
>> * Prevents transfers through the JVM heap as much as possibly by offloading 
>> to deeper levels via NIO, hence allowing the operating system to optimize 
>> the transfer.
>> * Using more JRE heap in the fallback case when no NIO is possible (still 
>> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
>> hardware / fast I/O devides.
>> 
>> Using JMH I have benchmarked both, the original implementation and this 
>> implementation, and (depending on the used hardware and use case) 
>> performance change was approx. doubled performance. So this PoC proofs that 
>> it makes sense to finalize this work and turn it into an actual OpenJDK 
>> contribution. 
>> 
>> I encourage everybody to discuss this draft:
>> * Are there valid arguments for *not* doing this change?
>> * Is there a *better* way to improve performance of 
>> `Channels.newInputStream().transferTo()`?
>> * How to go on from here: What is missing to get this ready for an actual 
>> review?
>
> Markus KARG has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Draft: Renaming i and separating code into several methods
>   
>   Signed-off-by: Markus Karg 

I have renamed `i` (and other ambiguous variables) and separated the 
implementation of `transferTo` into multiple methods. I will look into the test 
examples next to learn what I can reuse when writing tests for all those 
implementations. Thank you for all the kind input and the good ideas.

-

PR: https://git.openjdk.java.net/jdk/pull/4263

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]

[Markus KARG]

> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  Draft: Renaming i and separating code into several methods
  
  Signed-off-by: Markus Karg 

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/7d18ca62..47ee00a2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4263=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4263=01-02

  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263