Re: [PATCH] Enhancement proposal for java.util.StringJoiner

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

Oh, I was unaware of it! Thanks!

13.02.2020, 21:49, "Brent Christian" :
> As a point of interest, some investigation of updating StringJoiner for
> CompactStrings was done a while back.
>
> See https://bugs.openjdk.java.net/browse/JDK-8148937
>
> -Brent
>
> On 2/3/20 2:38 PM, Сергей Цыпанов wrote:
>>  Hello,
>>
>>  as of JDK14 java.util.StringJoiner still uses char[] as a storage of glued 
>> Strings.
>>
>>  This applies for the cases when all joined Strings as well as delimiter, 
>> prefix and suffix contain only ASCII symbols.
>>
>>  As a result when StringJoiner.toString() is invoked, byte[] stored in 
>> String is inflated in order to fill in char[] and
>>  finally char[] is compressed when constructor of String is called:
>>
>>  String delimiter = this.delimiter;
>>  char[] chars = new char[this.len + addLen];
>>  int k = getChars(this.prefix, chars, 0);
>>  if (size > 0) {
>>   k += getChars(elts[0], chars, k); // inflate byte[] -> char[]
>>
>>   for(int i = 1; i < size; ++i) {
>>   k += getChars(delimiter, chars, k);
>>   k += getChars(elts[i], chars, k);
>>   }
>>  }
>>
>>  k += getChars(this.suffix, chars, k);
>>  return new String(chars); // compress char[] -> byte[]
>>
>>  This can be improved by detecting cases when String.isLatin1() returns true 
>> for all involved Strings.
>>
>>  I've prepared a patch along with benchmark proving that this change is 
>> correct and brings improvement.
>>  The only concern I have is about String.isLatin1(): as far as String 
>> belongs to java.lang and StringJoiner to java.util
>>  package-private String.isLatin1() cannot be directly accessed, we need to 
>> make it public for successful compilation.
>>
>>  Another solution is to create an intermediate utility class located in 
>> java.lang which delegates the call to String.isLatin1():
>>
>>  package java.lang;
>>
>>  public class StringHelper {
>>   public static boolean isLatin1(String str) {
>>   return str.isLatin1();
>>   }
>>  }
>>
>>  This allows to keep java.lang.String intact and have access to it's 
>> package-private method outside of java.lang package.
>>
>>  Below I've added results of benchmarking for specified case (all Strings 
>> are Latin1). The other case (at least one String is UTF-8) uses existing 
>> code so there will be only a tiny regression due to several if-checks.
>>
>>  With best regards,
>>  Sergey Tsypanov
>>
>> (count) (length) Original 
>> Patched Units
>>  stringJoiner 1 1 26.7 ± 1.3 38.2 ± 1.1 ns/op
>>  stringJoiner 1 5 27.4 ± 0.0 40.5 ± 2.2 ns/op
>>  stringJoiner 1 10 29.6 ± 1.9 38.4 ± 1.9 ns/op
>>  stringJoiner 1 100 61.1 ± 6.9 47.6 ± 0.6 ns/op
>>  stringJoiner 5 1 91.1 ± 6.7 83.6 ± 2.0 ns/op
>>  stringJoiner 5 5 96.1 ± 10.7 85.6 ± 1.1 ns/op
>>  stringJoiner 5 10 105.5 ± 14.3 84.7 ± 1.1 ns/op
>>  stringJoiner 5 100 266.6 ± 30.1 139.6 ± 14.0 ns/op
>>  stringJoiner 10 1 190.7 ± 23.0 162.0 ± 2.9 ns/op
>>  stringJoiner 10 5 200.0 ± 16.9 167.5 ± 11.0 ns/op
>>  stringJoiner 10 10 216.4 ± 12.4 164.8 ± 1.7 ns/op
>>  stringJoiner 10 100 545.3 ± 49.7 282.2 ± 12.0 ns/op
>>  stringJoiner 100 1 1467.0 ± 90.3 1302.0 ± 18.5 ns/op
>>  stringJoiner 100 5 1491.8 ± 166.2 1493.0 ± 135.4 ns/op
>>  stringJoiner 100 10 1768.8 ± 160.6 1760.8 ± 111.4 ns/op
>>  stringJoiner 100 100 3654.3 ± 113.1 3120.9 ± 175.9 ns/op
>>
>>  stringJoiner:·gc.alloc.rate.norm 1 1 120.0 ± 0.0 120.0 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 1 5 128.0 ± 0.0 120.0 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 1 10 144.0 ± 0.0 136.0 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 1 100 416.0 ± 0.0 312.0 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 5 1 144.0 ± 0.0 136.0 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 5 5 200.0 ± 0.0 168.0 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 5 10 272.0 ± 0.0 216.0 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 5 100 1632.0 ± 0.0 1128.0 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 10 1 256.0 ± 0.0 232.0 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 10 5 376.0 ± 0.0 312.0 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 10 10 520.0 ± 0.0 408.0 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 10 100 3224.1 ± 0.0 2216.1 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 100 1 1760.2 ± 14.9 1544.2 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 100 5 2960.3 ± 14.9 2344.2 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 100 10 4440.4 ± 0.0 3336.3 ± 0.0 B/op
>>  stringJoiner:·gc.alloc.rate.norm 100 100 31449.3 ± 12.2 21346.7 ± 14.7 B/op

Re: RFR 8238579: HttpsURLConnection drops the timeout and hangs forever in read(Internet mail)

[杨晓峰]

Hi Vyom,
  The patch looks fine. Can you add a regression test for it?

Thanks,
Felix Yang
在 2020/2/14 下午1:01，“core-libs-dev 代表 Vyom 
Tiwari” 写入:

Hi All,

Please find the below fix  which resolves the issue(
https://bugs.openjdk.java.net/browse/JDK-8238579).

"HttpURLConnection.writeRequests()" retry in case of any write failure,
during retry it creates new HttpsClient  without connectTimeout &
readTimeout. Below fix sets the connect & read timeout.

Thanks,
Vyom

diff -r 7e6165c9c606

src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
---

a/src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
Thu Feb 13 17:14:45 2020 -0800
+++

b/src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
Fri Feb 14 10:11:06 2020 +0530
@@ -87,10 +87,15 @@
  */
 public void setNewClient (URL url, boolean useCache)
 throws IOException {
+int readTimeout = getReadTimeout();
 http = HttpsClient.New (getSSLSocketFactory(),
 url,
 getHostnameVerifier(),
-useCache, this);
+ null,
+ -1,
+useCache,
+ getConnectTimeout(), this);
+ http.setReadTimeout(readTimeout);
 ((HttpsClient)http).afterConnect();
 }

@@ -132,10 +137,14 @@
 boolean useCache) throws IOException {
 if (connected)
 return;
-http = HttpsClient.New (getSSLSocketFactory(),
-url,
-getHostnameVerifier(),
-proxyHost, proxyPort, useCache, this);
+int readTimeout = getReadTimeout();
+http = HttpsClient.New(getSSLSocketFactory(),
+url,
+getHostnameVerifier(),
+proxyHost, proxyPort,
+useCache,
+getConnectTimeout(), this);
+http.setReadTimeout(readTimeout);
 connected = true;
 }

RFR 8238579: HttpsURLConnection drops the timeout and hangs forever in read

[Vyom Tiwari]

Hi All,

Please find the below fix  which resolves the issue(
https://bugs.openjdk.java.net/browse/JDK-8238579).

"HttpURLConnection.writeRequests()" retry in case of any write failure,
during retry it creates new HttpsClient  without connectTimeout &
readTimeout. Below fix sets the connect & read timeout.

Thanks,
Vyom

diff -r 7e6165c9c606
src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
---
a/src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
Thu Feb 13 17:14:45 2020 -0800
+++
b/src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
Fri Feb 14 10:11:06 2020 +0530
@@ -87,10 +87,15 @@
  */
 public void setNewClient (URL url, boolean useCache)
 throws IOException {
+int readTimeout = getReadTimeout();
 http = HttpsClient.New (getSSLSocketFactory(),
 url,
 getHostnameVerifier(),
-useCache, this);
+ null,
+ -1,
+useCache,
+ getConnectTimeout(), this);
+ http.setReadTimeout(readTimeout);
 ((HttpsClient)http).afterConnect();
 }

@@ -132,10 +137,14 @@
 boolean useCache) throws IOException {
 if (connected)
 return;
-http = HttpsClient.New (getSSLSocketFactory(),
-url,
-getHostnameVerifier(),
-proxyHost, proxyPort, useCache, this);
+int readTimeout = getReadTimeout();
+http = HttpsClient.New(getSSLSocketFactory(),
+url,
+getHostnameVerifier(),
+proxyHost, proxyPort,
+useCache,
+getConnectTimeout(), this);
+http.setReadTimeout(readTimeout);
 connected = true;
 }

RE: Should indexOfLatin1Unsafe be private instead of public in java\lang\StringUTF16.java?

[Patrick Zhang OS]

OK, thanks Roger, I am looking for the opportunity of speeding up indexOf, and 
would fix it by the way once I have a patch there.

Regards
Patrick

-Original Message-
From: core-libs-dev  On Behalf Of Roger 
Riggs
Sent: Friday, February 14, 2020 12:12 AM
To: core-libs-dev@openjdk.java.net
Subject: Re: Should indexOfLatin1Unsafe be private instead of public in 
java\lang\StringUTF16.java?

Hi Patrick,

Private would be good, but since they are in a package-private class they are 
not visibile outside the package.
If there is some other change to the class then fix it but otherwise not worth 
the overhead.

Roger

On 2/13/20 10:34 AM, Patrick Zhang OS wrote:
> Hi,
>
> A quick question,
> I read the code snippets of indexOf(String str), found indexOfUnsafe [1] and 
> indexOfLatin1Unsafe [2] have different access control, but it looks both 
> should be private. Did I miss any outer caller or any other restriction? 
> Thanks for your comment.
>
> [1] private static int indexOfUnsafe(byte[] value, int valueCount, 
> byte[] str, int strCount, int fromIndex)
> http://hg.openjdk.java.net/jdk/jdk/file/cf6409153216/src/java.base/sha
> re/classes/java/lang/StringUTF16.java#l392
> [2] public static int indexOfLatin1Unsafe(byte[] src, int srcCount, 
> byte[] tgt, int tgtCount, int fromIndex)
> http://hg.openjdk.java.net/jdk/jdk/file/cf6409153216/src/java.base/sha
> re/classes/java/lang/StringUTF16.java#l440
>
> Regards
> Patrick
>

Re: [PATCH] Enhancement proposal for java.util.StringJoiner

[Brent Christian]

As a point of interest, some investigation of updating StringJoiner for 
CompactStrings was done a while back.


See https://bugs.openjdk.java.net/browse/JDK-8148937

-Brent

On 2/3/20 2:38 PM, Сергей Цыпанов wrote:

Hello,

as of JDK14 java.util.StringJoiner still uses char[] as a storage of glued 
Strings.

This applies for the cases when all joined Strings as well as delimiter, prefix 
and suffix contain only ASCII symbols.

As a result when StringJoiner.toString() is invoked, byte[] stored in String is 
inflated in order to fill in char[] and
finally char[] is compressed when constructor of String is called:

String delimiter = this.delimiter;
char[] chars = new char[this.len + addLen];
int k = getChars(this.prefix, chars, 0);
if (size > 0) {
 k += getChars(elts[0], chars, k);// inflate byte[] -> char[]

 for(int i = 1; i < size; ++i) {
 k += getChars(delimiter, chars, k);
 k += getChars(elts[i], chars, k);
 }
}

k += getChars(this.suffix, chars, k);
return new String(chars);// compress char[] -> byte[]

This can be improved by detecting cases when String.isLatin1() returns true for 
all involved Strings.

I've prepared a patch along with benchmark proving that this change is correct 
and brings improvement.
The only concern I have is about String.isLatin1(): as far as String belongs to 
java.lang and StringJoiner to java.util
package-private String.isLatin1() cannot be directly accessed, we need to make 
it public for successful compilation.

Another solution is to create an intermediate utility class located in 
java.lang which delegates the call to String.isLatin1():

package java.lang;

public class StringHelper {
 public static boolean isLatin1(String str) {
 return str.isLatin1();
 }
}

This allows to keep java.lang.String intact and have access to it's 
package-private method outside of java.lang package.

Below I've added results of benchmarking for specified case (all Strings are 
Latin1). The other case (at least one String is UTF-8) uses existing code so 
there will be only a tiny regression due to several if-checks.

With best regards,
Sergey Tsypanov



   (count)  (length) Original   
  PatchedUnits
stringJoiner1 1 26.7 ±   1.3
38.2 ±   1.1ns/op
stringJoiner1 5 27.4 ±   0.0
40.5 ±   2.2ns/op
stringJoiner110 29.6 ±   1.9
38.4 ±   1.9ns/op
stringJoiner1   100 61.1 ±   6.9
47.6 ±   0.6ns/op
stringJoiner5 1 91.1 ±   6.7
83.6 ±   2.0ns/op
stringJoiner5 5 96.1 ±  10.7
85.6 ±   1.1ns/op
stringJoiner510105.5 ±  14.3
84.7 ±   1.1ns/op
stringJoiner5   100266.6 ±  30.1
   139.6 ±  14.0ns/op
stringJoiner   10 1190.7 ±  23.0
   162.0 ±   2.9ns/op
stringJoiner   10 5200.0 ±  16.9
   167.5 ±  11.0ns/op
stringJoiner   1010216.4 ±  12.4
   164.8 ±   1.7ns/op
stringJoiner   10   100545.3 ±  49.7
   282.2 ±  12.0ns/op
stringJoiner  100 1   1467.0 ±  90.3
  1302.0 ±  18.5ns/op
stringJoiner  100 5   1491.8 ± 166.2
  1493.0 ± 135.4ns/op
stringJoiner  10010   1768.8 ± 160.6
  1760.8 ± 111.4ns/op
stringJoiner  100   100   3654.3 ± 113.1
  3120.9 ± 175.9ns/op

stringJoiner:·gc.alloc.rate.norm1 1120.0 ±   0.0
   120.0 ±   0.0 B/op
stringJoiner:·gc.alloc.rate.norm1 5128.0 ±   0.0
   120.0 ±   0.0 B/op
stringJoiner:·gc.alloc.rate.norm110144.0 ±   0.0
   136.0 ±   0.0 B/op
stringJoiner:·gc.alloc.rate.norm1   100416.0 ±   0.0
   312.0 ±   0.0 B/op
stringJoiner:·gc.alloc.rate.norm5 1144.0 ±   0.0
   136.0 ±   0.0 B/op
stringJoiner:·gc.alloc.rate.norm5 5200.0 ±   0.0
   168.0 ±   0.0 B/op
stringJoiner:·gc.alloc.rate.norm510272.0 ±   0.0
   216.0 ±   0.0 B/op
stringJoiner:·gc.alloc.rate.norm5   10

Re: RFR(S) : 8238943: switch to jtreg 5.0

[Erik Joelsson]

Looks good.

/Erik

On 2020-02-13 10:08, Igor Ignatev wrote:

Oh, I’m sorry I actually changed it to 5.0 when were (re)doing testing, and 
apparently forgot to replace the webrev, the right is 
http://cr.openjdk.java.net/~iignatyev//8238943/webrev.01 ; with version field 
value being the only difference b/w .00 and .01

Thanks,
— Igor


On Feb 13, 2020, at 9:23 AM, Erik Joelsson  wrote:

Looks good, but could you change the "version" field to "5.0", it should work 
now.

/Erik


On 2020-02-13 08:50, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8238943/webrev.00

10 lines changed: 1 ins; 0 del; 9 mod;

Hi all,
could you please review the patch which changes jtreg version used in jdk/jdk 
to the latest and greatest -- jtreg 5.0? and as (recently became) usually, this 
patch also bumps requiredVersion in all the jtreg test suites in order to 
reduce possible discrepancy in results.
JBS: https://bugs.openjdk.java.net/browse/JDK-8238943
webrev: http://cr.openjdk.java.net/~iignatyev/8238943/webrev.00
testing: tier1-4
Thanks,
-- Igor

Re: JDK 15 RFR of JDK-8237805: Use inline @jls @jvms in core libs where appropriate

[Joe Darcy]

Hello,

I changed the one "§" to "section" and re-flowed the paragraphs 
with long lines.


I believe the existing use of

    (Lorem ipsum ... aliqua.)

is correct rather than

    (Lorem ipsum ... aliqua).

Thanks,

-Joe

On 2/13/2020 3:14 AM, Pavel Rappo wrote:

@Joe, I noticed you changed "section" to the § entity in one place. In
other places it is still in prose, i.e. "section" and "sections". I think
whatever we choose we should try to use it consistently. I built the API docs
and checked that everything renders as expected. (Links are 404, obviously,
since they refer to a not-yet-released version.)

   This change looks good to me.

@Daniel, not sure how reputable these sources are. Still, worth a quick look:

   https://www.grammarly.com/blog/period/
   
https://www.chicagomanualofstyle.org/qanda/data/faq/topics/Punctuation/faq0040.html

I was thinking that the issue you were referring to was about differences
between American English and British English. However, after re-reading the
links above I now believe that I misremembered. Those differences apply to full
stops (periods) and quotation marks, not full stops and parentheses.

-Pavel


On 13 Feb 2020, at 10:07, Daniel Fuchs  wrote:

Hi Joe,

Nits:

http://cr.openjdk.java.net/~darcy/8237805.0/src/java.base/share/classes/java/lang/invoke/MethodHandle.java.frames.html
line 226

http://cr.openjdk.java.net/~darcy/8237805.0/src/java.base/share/classes/java/lang/invoke/MethodHandleInfo.java.frames.html
line 46

http://cr.openjdk.java.net/~darcy/8237805.0/src/java.base/share/classes/java/lang/invoke/MethodHandles.java.frames.html
lines 505 and 518

http://cr.openjdk.java.net/~darcy/8237805.0/src/java.base/share/classes/java/lang/invoke/MethodType.java.frames.html
line 95

wouldn't it be more correct to have the full stop
after the closing parenthesis, rather than before?
(no need for new webrev)

best regards,

-- daniel

On 13/02/2020 04:47, Joe Darcy wrote:

Hello,
Inline forms of the @jls and @jvms javadoc tags are now available 
(JDK-8235926). I made a pass over the files in java.base to take advantage of 
the new feature:
 http://cr.openjdk.java.net/~darcy/8237805.0/
Thanks,
-Joe

Re: RFR (XS) 8239007: java/math/BigInteger/largeMemory/ tests should be disabled on 32-bit platforms

[Aleksey Shipilev]

Good. Pushed.

Cheers,
-Aleksey

On 2/13/20 7:14 PM, Brian Burkhalter wrote:
> Works for me.
> 
> Brian
> 
>> On Feb 13, 2020, at 10:13 AM, Aleksey Shipilev  wrote:
>>
>> Thanks. Trivial?
>>
>> On 2/13/20 7:12 PM, Brian Burkhalter wrote:
>>> +1
>>>
>>> Brian
>>>
 On Feb 13, 2020, at 10:10 AM, Aleksey Shipilev >>> > wrote:

 Test bug:
  https://bugs.openjdk.java.net/browse/JDK-8239007

 Fix:
  https://cr.openjdk.java.net/~shade/8239007/webrev.01/

 Follows the precedent of:
  https://hg.openjdk.java.net/jdk/jdk/rev/848859723503

 Testing: affected tests on Linux {x86_64, x86_32}

Re: JDK 15 RFR of JDK-8237805: Use inline @jls @jvms in core libs where appropriate(Internet mail)

[Joe Darcy]

Hi Felix,

Noted; I run a script before pushing to update the copyright year.

Thanks,

-Joe

On 2/12/2020 9:28 PM, felixxfyang(杨晓峰) wrote:

Hi Joe，
   A minor comment on copyright date 2019 -> 2020

-Felix
在 2020/2/13 下午12:49，“core-libs-dev 代表 Joe 
Darcy” 写入:

 Hello,
 
 Inline forms of the @jls and @jvms javadoc tags are now available

 (JDK-8235926). I made a pass over the files in java.base to take
 advantage of the new feature:
 
  http://cr.openjdk.java.net/~darcy/8237805.0/
 
 Thanks,
 
 -Joe
 
 
 

Re: RFR(S) : 8238943: switch to jtreg 5.0

[Joe Wang]

+1 for the change to test/jaxp/TEST.ROOT.

Best,
Joe


On 2/13/20 10:08 AM, Igor Ignatev wrote:

Oh, I’m sorry I actually changed it to 5.0 when were (re)doing testing, and 
apparently forgot to replace the webrev, the right is 
http://cr.openjdk.java.net/~iignatyev//8238943/webrev.01 ; with version field 
value being the only difference b/w .00 and .01

Thanks,
— Igor


On Feb 13, 2020, at 9:23 AM, Erik Joelsson  wrote:

Looks good, but could you change the "version" field to "5.0", it should work 
now.

/Erik


On 2020-02-13 08:50, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8238943/webrev.00

10 lines changed: 1 ins; 0 del; 9 mod;

Hi all,
could you please review the patch which changes jtreg version used in jdk/jdk 
to the latest and greatest -- jtreg 5.0? and as (recently became) usually, this 
patch also bumps requiredVersion in all the jtreg test suites in order to 
reduce possible discrepancy in results.
JBS: https://bugs.openjdk.java.net/browse/JDK-8238943
webrev: http://cr.openjdk.java.net/~iignatyev/8238943/webrev.00
testing: tier1-4
Thanks,
-- Igor

Re: RFR (XS) 8239007: java/math/BigInteger/largeMemory/ tests should be disabled on 32-bit platforms

[Brian Burkhalter]

Works for me.

Brian

> On Feb 13, 2020, at 10:13 AM, Aleksey Shipilev  wrote:
> 
> Thanks. Trivial?
> 
> On 2/13/20 7:12 PM, Brian Burkhalter wrote:
>> +1
>> 
>> Brian
>> 
>>> On Feb 13, 2020, at 10:10 AM, Aleksey Shipilev >> > wrote:
>>> 
>>> Test bug:
>>>  https://bugs.openjdk.java.net/browse/JDK-8239007
>>> 
>>> Fix:
>>>  https://cr.openjdk.java.net/~shade/8239007/webrev.01/
>>> 
>>> Follows the precedent of:
>>>  https://hg.openjdk.java.net/jdk/jdk/rev/848859723503
>>> 
>>> Testing: affected tests on Linux {x86_64, x86_32}
> 

Re: RFR (XS) 8239007: java/math/BigInteger/largeMemory/ tests should be disabled on 32-bit platforms

[Aleksey Shipilev]

Thanks. Trivial?

On 2/13/20 7:12 PM, Brian Burkhalter wrote:
> +1
> 
> Brian
> 
>> On Feb 13, 2020, at 10:10 AM, Aleksey Shipilev > > wrote:
>>
>> Test bug:
>>  https://bugs.openjdk.java.net/browse/JDK-8239007
>>
>> Fix:
>>  https://cr.openjdk.java.net/~shade/8239007/webrev.01/
>>
>> Follows the precedent of:
>>  https://hg.openjdk.java.net/jdk/jdk/rev/848859723503
>>
>> Testing: affected tests on Linux {x86_64, x86_32}

Re: RFR (XS) 8239007: java/math/BigInteger/largeMemory/ tests should be disabled on 32-bit platforms

[Brian Burkhalter]

+1

Brian

> On Feb 13, 2020, at 10:10 AM, Aleksey Shipilev  wrote:
> 
> Test bug:
>  https://bugs.openjdk.java.net/browse/JDK-8239007 
> 
> 
> Fix:
>  https://cr.openjdk.java.net/~shade/8239007/webrev.01/ 
> 
> 
> Follows the precedent of:
>  https://hg.openjdk.java.net/jdk/jdk/rev/848859723503 
> 
> 
> Testing: affected tests on Linux {x86_64, x86_32}

Re: RFR(S) : 8238943: switch to jtreg 5.0

[Igor Ignatev]

Oh, I’m sorry I actually changed it to 5.0 when were (re)doing testing, and 
apparently forgot to replace the webrev, the right is 
http://cr.openjdk.java.net/~iignatyev//8238943/webrev.01 ; with version field 
value being the only difference b/w .00 and .01

Thanks,
— Igor

> On Feb 13, 2020, at 9:23 AM, Erik Joelsson  wrote:
> 
> Looks good, but could you change the "version" field to "5.0", it should 
> work now.
> 
> /Erik
> 
>>> On 2020-02-13 08:50, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8238943/webrev.00
>>> 10 lines changed: 1 ins; 0 del; 9 mod;
>> Hi all,
>> could you please review the patch which changes jtreg version used in 
>> jdk/jdk to the latest and greatest -- jtreg 5.0? and as (recently became) 
>> usually, this patch also bumps requiredVersion in all the jtreg test suites 
>> in order to reduce possible discrepancy in results.
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8238943
>> webrev: http://cr.openjdk.java.net/~iignatyev/8238943/webrev.00
>> testing: tier1-4
>> Thanks,
>> -- Igor

RFR (XS) 8239007: java/math/BigInteger/largeMemory/ tests should be disabled on 32-bit platforms

[Aleksey Shipilev]

Test bug:
  https://bugs.openjdk.java.net/browse/JDK-8239007

Fix:
  https://cr.openjdk.java.net/~shade/8239007/webrev.01/

Follows the precedent of:
  https://hg.openjdk.java.net/jdk/jdk/rev/848859723503

Testing: affected tests on Linux {x86_64, x86_32}

-- 
Thanks,
-Aleksey

Re: RFR(S) : 8238943: switch to jtreg 5.0

[Erik Joelsson]

Looks good, but could you change the "version" field to "5.0", it should 
work now.


/Erik

On 2020-02-13 08:50, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8238943/webrev.00

10 lines changed: 1 ins; 0 del; 9 mod;

Hi all,

could you please review the patch which changes jtreg version used in jdk/jdk 
to the latest and greatest -- jtreg 5.0? and as (recently became) usually, this 
patch also bumps requiredVersion in all the jtreg test suites in order to 
reduce possible discrepancy in results.

JBS: https://bugs.openjdk.java.net/browse/JDK-8238943
webrev: http://cr.openjdk.java.net/~iignatyev/8238943/webrev.00
testing: tier1-4

Thanks,
-- Igor
  

Re: RFR(S) : 8238943: switch to jtreg 5.0

[Jonathan Gibbons]

Igor,

The change to langtools/TEST.ROOT is OK.

That being said, there are some old entries there which could also be 
cleaned up (separately?)  These lines probably date from the development 
of JDK 9 and the evolution of Project Jigsaw.



   20 # Use new module options
   21 useNewOptions=true
   22
   23 # Use --patch-module instead of -Xmodule:
   24 useNewPatchModule=true

-- Jon


On 2/13/20 8:50 AM, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8238943/webrev.00

10 lines changed: 1 ins; 0 del; 9 mod;

Hi all,

could you please review the patch which changes jtreg version used in jdk/jdk 
to the latest and greatest -- jtreg 5.0? and as (recently became) usually, this 
patch also bumps requiredVersion in all the jtreg test suites in order to 
reduce possible discrepancy in results.

JBS: https://bugs.openjdk.java.net/browse/JDK-8238943
webrev: http://cr.openjdk.java.net/~iignatyev/8238943/webrev.00
testing: tier1-4

Thanks,
-- Igor
  

RFR(S) : 8238943: switch to jtreg 5.0

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8238943/webrev.00
> 10 lines changed: 1 ins; 0 del; 9 mod;

Hi all,

could you please review the patch which changes jtreg version used in jdk/jdk 
to the latest and greatest -- jtreg 5.0? and as (recently became) usually, this 
patch also bumps requiredVersion in all the jtreg test suites in order to 
reduce possible discrepancy in results.

JBS: https://bugs.openjdk.java.net/browse/JDK-8238943
webrev: http://cr.openjdk.java.net/~iignatyev/8238943/webrev.00
testing: tier1-4 

Thanks,
-- Igor
 

Re: Should indexOfLatin1Unsafe be private instead of public in java\lang\StringUTF16.java?

[Roger Riggs]

Hi Patrick,

Private would be good, but since they are in a package-private class 
they are not visibile outside the package.
If there is some other change to the class then fix it but otherwise not 
worth the overhead.


Roger

On 2/13/20 10:34 AM, Patrick Zhang OS wrote:

Hi,

A quick question,
I read the code snippets of indexOf(String str), found indexOfUnsafe [1] and 
indexOfLatin1Unsafe [2] have different access control, but it looks both should 
be private. Did I miss any outer caller or any other restriction? Thanks for 
your comment.

[1] private static int indexOfUnsafe(byte[] value, int valueCount, byte[] str, 
int strCount, int fromIndex)
http://hg.openjdk.java.net/jdk/jdk/file/cf6409153216/src/java.base/share/classes/java/lang/StringUTF16.java#l392
[2] public static int indexOfLatin1Unsafe(byte[] src, int srcCount, byte[] tgt, 
int tgtCount, int fromIndex)
http://hg.openjdk.java.net/jdk/jdk/file/cf6409153216/src/java.base/share/classes/java/lang/StringUTF16.java#l440

Regards
Patrick

Re: RFR: 8238953: tools/jpackage tests do not work on Ubuntu Linux

[Alexey Semenyuk]

Hi Matthias,

We don't set "jpackage.test.disabledPackagers" property from the test 
code. It is assumed to be set from jtreg command line that runs tests. 
The value of the property is just checked in tests.

So basically there is no need to change code at all.
However, if you want to disable running rpm tests on Ubuntu, you can 
tweak setting of jdk.jpackage.test.PackageType.Inner.DISABLED_PACKAGERS 
property 
(test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageType.java).

Something like this:

class Inner {
    private static boolean isUbuntu() {
    if (!TKit.isLinux()) {
    return false;
    }
    ...
    }

    private final static Set DISABLED_PACKAGERS;

    static {
    Set disabledPackagers = 
TKit.tokenizeConfigProperty("disabledPackagers");

    if (disabledPackagers != null) {
    DISABLED_PACKAGERS = disabledPackagers;
    } else if (isUbuntu()) {
    DISABLED_PACKAGERS = Set.of("rpm");
    } else {
    DISABLED_PACKAGERS = Collections.emptySet();
    }
    }
}

This way the fix would disable running rpm tests on Ubuntu if 
"jpackage.test.disabledPackagers" property is not set allowing to still 
run rpm tests on Ubuntu in case the property is explicitly set to some 
value.


- Alexey


On 2/12/2020 4:19 AM, Baesken, Matthias wrote:

Hello, please review this small  test related change .

Currently (most of the)  tools/jpackage tests do not work on Ubuntu Linux .
Reason is that the rpm parts of the  jpackage tests  do not pass on this distro 
.
The  rpm tests  can be disabled by this property :



Do you expect  those  tests to work on Ubuntu ?

No, rpm test are not supposed to pass on Ubuntu.
To disable running rpm tests on Ubuntu please set
"jpackage.test.disabledPackagers" system property to "rpm":
-Djpackage.test.disabledPackagers=rpm.

However the tests should set this property   themselves , it is really bad that 
it must be currently configured from outside .




Bug/webrev :

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

http://cr.openjdk.java.net/~mbaesken/webrevs/8238953.0/


Best regards, Matthias

Should indexOfLatin1Unsafe be private instead of public in java\lang\StringUTF16.java?

[Patrick Zhang OS]

Hi,

A quick question,
I read the code snippets of indexOf(String str), found indexOfUnsafe [1] and 
indexOfLatin1Unsafe [2] have different access control, but it looks both should 
be private. Did I miss any outer caller or any other restriction? Thanks for 
your comment.

[1] private static int indexOfUnsafe(byte[] value, int valueCount, byte[] str, 
int strCount, int fromIndex)
http://hg.openjdk.java.net/jdk/jdk/file/cf6409153216/src/java.base/share/classes/java/lang/StringUTF16.java#l392
[2] public static int indexOfLatin1Unsafe(byte[] src, int srcCount, byte[] tgt, 
int tgtCount, int fromIndex)
http://hg.openjdk.java.net/jdk/jdk/file/cf6409153216/src/java.base/share/classes/java/lang/StringUTF16.java#l440

Regards
Patrick

Re: RFR: 8238773: Miscellaneous ODR violations in UNIX native code

[Andrew Haley]

On 2/10/20 4:44 PM, Andrew Haley wrote:
> In a couple of places variables are defined in header files, leading
> to violations of the One Definition Rule. This is not Standard C and
> breaks the build on modern compilers which complain.
> 
> http://cr.openjdk.java.net/~aph/8238773/

I withdraw this: it overlapped with

8238386: (sctp) jdk.sctp/unix/native/libsctp/SctpNet.c "multiple definition" 
link errors with GCC10

and

8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link 
errors with GCC10

which fixed the same bugs in exactly the same way.

-- 
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: JDK 15 RFR of JDK-8237805: Use inline @jls @jvms in core libs where appropriate

[Pavel Rappo]

@Joe, I noticed you changed "section" to the § entity in one place. In
other places it is still in prose, i.e. "section" and "sections". I think
whatever we choose we should try to use it consistently. I built the API docs
and checked that everything renders as expected. (Links are 404, obviously,
since they refer to a not-yet-released version.)

  This change looks good to me.

@Daniel, not sure how reputable these sources are. Still, worth a quick look:

  https://www.grammarly.com/blog/period/
  
https://www.chicagomanualofstyle.org/qanda/data/faq/topics/Punctuation/faq0040.html

I was thinking that the issue you were referring to was about differences
between American English and British English. However, after re-reading the
links above I now believe that I misremembered. Those differences apply to full
stops (periods) and quotation marks, not full stops and parentheses.

-Pavel

> On 13 Feb 2020, at 10:07, Daniel Fuchs  wrote:
> 
> Hi Joe,
> 
> Nits:
> 
> http://cr.openjdk.java.net/~darcy/8237805.0/src/java.base/share/classes/java/lang/invoke/MethodHandle.java.frames.html
> line 226
> 
> http://cr.openjdk.java.net/~darcy/8237805.0/src/java.base/share/classes/java/lang/invoke/MethodHandleInfo.java.frames.html
> line 46
> 
> http://cr.openjdk.java.net/~darcy/8237805.0/src/java.base/share/classes/java/lang/invoke/MethodHandles.java.frames.html
> lines 505 and 518
> 
> http://cr.openjdk.java.net/~darcy/8237805.0/src/java.base/share/classes/java/lang/invoke/MethodType.java.frames.html
> line 95
> 
> wouldn't it be more correct to have the full stop
> after the closing parenthesis, rather than before?
> (no need for new webrev)
> 
> best regards,
> 
> -- daniel
> 
> On 13/02/2020 04:47, Joe Darcy wrote:
>> Hello,
>> Inline forms of the @jls and @jvms javadoc tags are now available 
>> (JDK-8235926). I made a pass over the files in java.base to take advantage 
>> of the new feature:
>> http://cr.openjdk.java.net/~darcy/8237805.0/
>> Thanks,
>> -Joe
> 

Re: JDK 15 RFR of JDK-8237805: Use inline @jls @jvms in core libs where appropriate

[Daniel Fuchs]

Hi Joe,

Nits:

http://cr.openjdk.java.net/~darcy/8237805.0/src/java.base/share/classes/java/lang/invoke/MethodHandle.java.frames.html
line 226

http://cr.openjdk.java.net/~darcy/8237805.0/src/java.base/share/classes/java/lang/invoke/MethodHandleInfo.java.frames.html
line 46

http://cr.openjdk.java.net/~darcy/8237805.0/src/java.base/share/classes/java/lang/invoke/MethodHandles.java.frames.html
lines 505 and 518

http://cr.openjdk.java.net/~darcy/8237805.0/src/java.base/share/classes/java/lang/invoke/MethodType.java.frames.html
line 95

wouldn't it be more correct to have the full stop
after the closing parenthesis, rather than before?
(no need for new webrev)

best regards,

-- daniel

On 13/02/2020 04:47, Joe Darcy wrote:

Hello,

Inline forms of the @jls and @jvms javadoc tags are now available 
(JDK-8235926). I made a pass over the files in java.base to take 
advantage of the new feature:


     http://cr.openjdk.java.net/~darcy/8237805.0/

Thanks,

-Joe

Re: RFR: 8237878: Improve ModuleLoaderMap datastructures

[Claes Redestad]

Hi,

On 2020-02-13 09:12, Peter Levart wrote:
This change is ok as it stands, but I'm afraid it is not in the spirit 
of Valhalla.


I wouldn't really know what the spirit of Valhalla will end up being,
yet. :-)

Unboxing the values have a measurable cost in the interpreter, and
can trigger some very early JIT compilations I'd like to avoid.

/Claes

Re: [PATCH] Enhancement proposal for java.util.StringJoiner

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

Hello, 

I've reworked the patch in order to decide about char[] / byte[] lazily.

This allows to dodge performance impact of reflective calls to 
String.isLatin1() for non-latin Strings
and at the same time keep the benefits of discrimination between char[] / 
byte[].

I've collected resutls for all the versions of my patch into the table below. 
As you can see 
StringBuilder reduces complexity but significantly slows down the case then 
non-latin Strings are joined.

Exclamation marks denote the cases where the last implementation clearly looses 
in performance
agains original StringJoiner.

Regards,
Sergey Tsypanov

P.S.

Also I think the further improvement is possible here: Tagir mentioned decoding 
of byte[] happening in
constructor of String(byte[]). The constructor is not aware about how the bytes 
are encoded and
has to decode them. However in our case when came to `new String(byte[])` we 
are sure that the bytes
are of Latin1. So we can skip decoding by calling StringLatin1.newString(). 
However, it copies incoming
byte[] which is again redundant in our particular case.


Benchmark  (count)  (latin)  (length)  
Original   Patched1 SB  Patched2Units
stringJoiner 1 true 1  26.9 ±   
0.7   48.8 ±   2.2   33.4 ±   0.2  42.6 ±   1.0ns/op   !
stringJoiner 1 true 5  30.5 ±   
1.0   46.1 ±   2.1   33.0 ±   0.1  42.3 ±   1.1ns/op   !
stringJoiner 1 true10  31.2 ±   
0.6   47.3 ±   1.3   34.3 ±   0.3  46.6 ±   1.4ns/op   !
stringJoiner 1 true   100  62.5 ±   
3.3   79.9 ±   4.8   44.4 ±   0.1  63.5 ±   2.1ns/op
stringJoiner 5 true 1  78.2 ±   
1.6  110.3 ±   2.9   87.8 ±   0.8  93.4 ±   1.7ns/op   !
stringJoiner 5 true 5  94.2 ±   
8.7  116.6 ±   0.7   88.2 ±   0.9  91.4 ±   0.9ns/op
stringJoiner 5 true10  95.3 ±   
6.9  100.1 ±   0.4   91.6 ±   0.6  93.7 ±   0.4ns/op
stringJoiner 5 true   100 188.0 ±  
10.2  136.0 ±   0.4  126.1 ±   0.7 135.3 ±   0.9ns/op
stringJoiner10 true 1 160.3 ±   
4.5  172.9 ±   0.8  177.6 ±   0.8 172.1 ±   0.6ns/op   !
stringJoiner10 true 5 169.0 ±   
4.7  180.2 ±   9.1  179.4 ±   1.0 171.7 ±   0.6ns/op
stringJoiner10 true10 205.7 ±  
16.4  182.7 ±   1.1  189.5 ±   1.2 178.3 ±   0.5ns/op
stringJoiner10 true   100 366.5 ±  
17.0  284.5 ±   3.1  290.0 ±   0.8 282.1 ±   0.9ns/op
stringJoiner   100 true 11117.6 ±  
11.1 2123.7 ±  11.1 1563.8 ±   2.81379.5 ±   4.4ns/op   !
stringJoiner   100 true 51270.7 ±  
40.2 2163.6 ±  12.4 1592.4 ±   4.01426.1 ±   4.4ns/op   !
stringJoiner   100 true101364.4 ±  
14.0 2283.8 ±  16.1 1773.7 ±  57.71525.0 ±   3.2ns/op   !
stringJoiner   100 true   1003592.9 ± 
164.8 3535.2 ±  29.9 2899.2 ±  51.03043.9 ±  85.6ns/op

stringJoiner 1false 1  35.6 ±   
1.2   59.1 ±   3.0   52.7 ±   1.2  44.9 ±   0.9ns/op   !
stringJoiner 1false 5  39.3 ±   
1.2   52.6 ±   2.5   54.4 ±   1.6  37.4 ±   0.9ns/op
stringJoiner 1false10  42.2 ±   
1.6   53.6 ±   0.3   52.2 ±   1.0  43.2 ±   1.0ns/op
stringJoiner 1false   100  70.5 ±   
1.8   86.4 ±   0.4   78.6 ±   1.2  75.8 ±   2.2ns/op   !
stringJoiner 5false 1  89.0 ±   
3.5  102.2 ±   1.0  116.3 ±   3.8  85.3 ±   0.1ns/op
stringJoiner 5false 5  87.6 ±   
0.7  106.5 ±   1.2  115.2 ±   2.9  90.7 ±   0.6ns/op   !
stringJoiner 5false10 109.0 ±   
5.6  116.5 ±   1.2  126.5 ±   0.5  97.2 ±   0.3ns/op
stringJoiner 5false   100 324.0 ±  
16.5  221.9 ±   0.5  288.9 ±   0.5 227.3 ±   0.7ns/op
strin

Re: RFR: 8237878: Improve ModuleLoaderMap datastructures

[Peter Levart]

Hi Claes,

I hope I'm not to late to comment on this. This change is ok as it 
stands, but I'm afraid it is not in the spirit of Valhalla. As I 
understand, you rely on the fact that Integer instances in the low range 
of values are cached and you then use identity comparison in the 
following method:


  86 public ClassLoader apply(String name) {
  87 Integer loader = map.get(name);
  88 if (loader == APP_LOADER_INDEX) {
  89 return APP_CLASSLOADER;
  90 } else if (loader == PLATFORM_LOADER_INDEX) {
  91 return PLATFORM_CLASSLOADER;
  92 } else { // BOOT_LOADER_INDEX
  93 return null;
  94 }
  95 }

Wouldn't it be better to rewrite this to something like the following:

  57 /**
  58  * Map from module to: (null: boot loader, FALSE: platform 
loader, TRUE: app loader)

  60  */
  61 private final Map map;

if (loader == null) {
  return null; // BOOT loader
} else if (loader) {
  return APP_CLASSLOADER;
} else {
  return PLATFORM_CLASSLOADER;
}


Regards, Peter


On 2/10/20 12:43 PM, Claes Redestad wrote:



On 2020-02-10 12:34, Alan Bateman wrote:

On 10/02/2020 09:04, Claes Redestad wrote:

:

So how about:
http://cr.openjdk.java.net/~redestad/8237878/open.02/


Thanks for restoring the use of Function. 
Changing Module::defineClass to invoke a method on ModuleLoaderMap is 
okay but the method needs to something like "isBuiltinMapper" because 
it tests if the function is a built-in mapper used for the boot layer 
(or child layers created when we need to dynamically augment the set 
of platform modules).


Minor nit but I think the comment on the Mapper constructor would say 
that it creates a Mapper to map the modules in the given 
Configuration to the built-in class loaders.


The rest looks good to me.


Thanks!

I'll run a few tests and push with this addendum, then:

diff -r 43b98c0e075d src/java.base/share/classes/java/lang/Module.java
--- a/src/java.base/share/classes/java/lang/Module.java    Mon Feb 10 
12:40:49 2020 +0100
+++ b/src/java.base/share/classes/java/lang/Module.java    Mon Feb 10 
12:42:43 2020 +0100

@@ -1094,7 +1094,7 @@

 // map each module to a class loader
 ClassLoader pcl = ClassLoaders.platformClassLoader();
-    boolean isModuleLoaderMapper = 
ModuleLoaderMap.isModuleLoaderMapper(clf);
+    boolean isModuleLoaderMapper = 
ModuleLoaderMap.isBuiltinMapper(clf);


 for (int index = 0; index < numModules; index++) {
 String name = resolvedModules[index].name();
diff -r 43b98c0e075d 
src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java
--- 
a/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java 
Mon Feb 10 12:40:49 2020 +0100
+++ 
b/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java 
Mon Feb 10 12:42:43 2020 +0100

@@ -61,7 +61,8 @@
 private final Map map;

 /**
- * Maps module names to the corresponding built-in classloader.
+ * Creates a Mapper to map module names in the given 
Configuration to

+ * built-in classloaders.
  *
  * As a proxy for the actual classloader, we store an easily 
archiveable
  * index value in the internal map. The index is stored as a 
boxed value

@@ -132,7 +133,7 @@
  * to the boot or platform classloader if the ClassLoader mapping 
function

  * originate from here.
  */
-    public static boolean isModuleLoaderMapper(FunctionClassLoader> clf) {
+    public static boolean isBuiltinMapper(FunctionClassLoader> clf) {

 return clf instanceof Mapper;
 }
 }

/Claes