RFR: 8245311: [macos] misc package tests failed due to "execution error: Finder got an error: AppleEvent timed out."

[alexander . matveev]

Please review the jpackage fix for bug [1] at [2].

- "hdiutil detach" resource busy error is fixed in same way as 
JDK-8242786 by repeating detach several times.

- pkgbuild timeout will be covered with JDK-8249395.

[1] https://bugs.openjdk.java.net/browse/JDK-8245311
[2] http://cr.openjdk.java.net/~almatvee/8245311/webrev.00/

Thanks,
Alexander

Re: RFR: JDK-8249289: Exception thrown when --temp points to non-existant directory

[alexander . matveev]

Hi Andy,

Can you double check two other places where File.list() was introduced 
with JDK-8223955 for similar issues?

One in MacPkgBundler.java and second in DeployParams.java.

Thanks,
Alexander

On 7/15/20 3:09 PM, Alexey Semenyuk wrote:

Andy,

Stream.close() call is missing on the result of Files.list() call.
I'd also replace Files.list(root) with Files.walk(root, 1) call to 
eliminate diving in directory tree.


- Alexey

On 7/15/2020 2:37 PM, Andy Herrick wrote:

Please review the jpackage fix for issue [1] at [2].

This fixes the regression in JDK16 which caused exception when an 
empty directory was used int the --temp jpackage option, and added to 
the existing test.


[1] - https://bugs.openjdk.java.net/browse/JDK-8249289

[2[ - http://cr.openjdk.java.net/~herrick/8249289/webrev.02/

/Andy

Re: RFR: JDK-8249289: Exception thrown when --temp points to non-existant directory

[Alexey Semenyuk]

Andy,

Stream.close() call is missing on the result of Files.list() call.
I'd also replace Files.list(root) with Files.walk(root, 1) call to 
eliminate diving in directory tree.


- Alexey

On 7/15/2020 2:37 PM, Andy Herrick wrote:

Please review the jpackage fix for issue [1] at [2].

This fixes the regression in JDK16 which caused exception when an 
empty directory was used int the --temp jpackage option, and added to 
the existing test.


[1] - https://bugs.openjdk.java.net/browse/JDK-8249289

[2[ - http://cr.openjdk.java.net/~herrick/8249289/webrev.02/

/Andy

Re: RFR: JDK-8222187: java.util.Base64.Decoder stream adds unexpected null bytes at the end

[Raffaello Giulietti]

Hi Roger,

On 2020-07-15 22:21, Roger Riggs wrote:

Hi Raffaello,

Base64.java:
2: Please update 2nd copyright year to 2020



I was unsure what to do here, thanks for guidance.
I also removed the seemingly useless import at former L33.




leftovers():
996: "&" the shortcutting && is more typical in a condition.
997: the -= is buried in an expression and a reader might miss it.



Corrected, as well as the analogous -= usage for wpos now at L1106-7




1053:  "can be reallocated to other vars":  not a useful comment, 
reflecting on only one possible implementation that is out of the 
control of the developer.
I'd almost rather see 'len' than 'limit - off'; it might be informative 
to the reader if 'limit' was declared final. (1056:)




Done, as well as declaring i and v final in the loop.




TestBase54.java:

2: Update 2nd copyright year to 2020

27:  Please add the bug number to the @bug line.

Style-wise, I would remove the blank lines at the beginning of blocks. 
(1095, 1115)




Done.



Thanks, Roger


On 7/14/20 11:47 AM, Raffaello Giulietti wrote:

Hi Roger,

here's the latest version of the patch.

I did:
* Withdraw the simplification in encodedOutLength(), as it is indeed 
out of scope for this bug fix.
* Restore field names in DecInputStream except for nextin (now wpos) 
and nextout (now rpos) because they have slightly different semantics. 
That's just for clarity. I would have nothing against reusing the old 
names for the proposed purpose.

* Switch back to the original no-arg read() implementation.
* Adjust comment continuation lines.
* Preserve the proposed logic of read(byte[], int, int) and the 
supporting methods.


Others needed changes are:
* Correct the copyright years: that's something better left to Oracle.
* Remove the apparently useless import at L33. I could build and run 
without it.



Greetings
Raffaello






# HG changeset patch
# User lello
# Date 1594848775 -7200
#  Wed Jul 15 23:32:55 2020 +0200
# Node ID f7407f35e83ab508f0e6deab4f776bb1a1c85e68
# Parent  a5649ebf94193770ca763391dd63807059d333b4
8222187: java.util.Base64.Decoder stream adds unexpected null bytes at 
the end

Reviewed-by: TBD
Contributed-by: Raffaello Giulietti 

diff --git a/src/java.base/share/classes/java/util/Base64.java 
b/src/java.base/share/classes/java/util/Base64.java

--- a/src/java.base/share/classes/java/util/Base64.java
+++ b/src/java.base/share/classes/java/util/Base64.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2019, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -30,7 +30,6 @@
 import java.io.IOException;
 import java.io.OutputStream;
 import java.nio.ByteBuffer;
-import java.nio.charset.StandardCharsets;

 import sun.nio.cs.ISO_8859_1;

@@ -961,12 +960,15 @@

 private final InputStream is;
 private final boolean isMIME;
-private final int[] base64;  // base64 -> byte mapping
-private int bits = 0;// 24-bit buffer for decoding
-private int nextin = 18; // next available "off" in 
"bits" for input;

- // -> 18, 12, 6, 0
-private int nextout = -8;// next available "off" in 
"bits" for output;
- // -> 8, 0, -8 (no byte for 
output)

+private final int[] base64; // base64 -> byte mapping
+private int bits = 0;   // 24-bit buffer for decoding
+
+/* writing bit pos inside bits; one of 24 (left, msb), 18, 12, 
6, 0 */

+private int wpos = 0;
+
+/* reading bit pos inside bits: one of 24 (left, msb), 16, 8, 0 */
+private int rpos = 0;
+
 private boolean eof = false;
 private boolean closed = false;

@@ -983,107 +985,153 @@
 return read(sbBuf, 0, 1) == -1 ? -1 : sbBuf[0] & 0xff;
 }

-private int eof(byte[] b, int off, int len, int oldOff)
-throws IOException
-{
+private int leftovers(byte[] b, int off, int pos, int limit) {
 eof = true;
-if (nextin != 18) {
-if (nextin == 12)
-throw new IOException("Base64 stream has one 
un-decoded dangling byte.");

-// treat ending xx/xxx without padding character legal.
-// same logic as v == '=' below
-b[off++] = (byte)(bits >> (16));
-if (nextin == 0) {   // only one padding byte
-if (len == 1) {  // no enough output space
-bits >>= 8;  // shift to lowest byte
-nextout = 0;
-} else {
-b[off++] = (byte) (bits >>  8);
-}
-}
+
+

Re: RFR: JDK-8222187: java.util.Base64.Decoder stream adds unexpected null bytes at the end

[Roger Riggs]

Hi Raffaello,

Base64.java:
2: Please update 2nd copyright year to 2020

leftovers():
996: "&" the shortcutting && is more typical in a condition.
997: the -= is buried in an expression and a reader might miss it.


1053:  "can be reallocated to other vars":  not a useful comment, 
reflecting on only one possible implementation that is out of the 
control of the developer.
I'd almost rather see 'len' than 'limit - off'; it might be informative 
to the reader if 'limit' was declared final. (1056:)


TestBase54.java:

2: Update 2nd copyright year to 2020

27:  Please add the bug number to the @bug line.

Style-wise, I would remove the blank lines at the beginning of blocks.  
(1095, 1115)


Thanks, Roger


On 7/14/20 11:47 AM, Raffaello Giulietti wrote:

Hi Roger,

here's the latest version of the patch.

I did:
* Withdraw the simplification in encodedOutLength(), as it is indeed 
out of scope for this bug fix.
* Restore field names in DecInputStream except for nextin (now wpos) 
and nextout (now rpos) because they have slightly different semantics. 
That's just for clarity. I would have nothing against reusing the old 
names for the proposed purpose.

* Switch back to the original no-arg read() implementation.
* Adjust comment continuation lines.
* Preserve the proposed logic of read(byte[], int, int) and the 
supporting methods.


Others needed changes are:
* Correct the copyright years: that's something better left to Oracle.
* Remove the apparently useless import at L33. I could build and run 
without it.



Greetings
Raffaello



# HG changeset patch
# User lello
# Date 1594741356 -7200
#  Tue Jul 14 17:42:36 2020 +0200
# Node ID 2bebe2aced4c3fa3b42b3c6ee445f9b7b0d20b5d
# Parent  a5649ebf94193770ca763391dd63807059d333b4
8222187: java.util.Base64.Decoder stream adds unexpected null bytes at 
the end

Reviewed-by: TBD
Contributed-by: Raffaello Giulietti 

diff --git a/src/java.base/share/classes/java/util/Base64.java 
b/src/java.base/share/classes/java/util/Base64.java

--- a/src/java.base/share/classes/java/util/Base64.java
+++ b/src/java.base/share/classes/java/util/Base64.java
@@ -961,12 +961,15 @@

 private final InputStream is;
 private final boolean isMIME;
-    private final int[] base64;  // base64 -> byte mapping
-    private int bits = 0;    // 24-bit buffer for decoding
-    private int nextin = 18; // next available "off" in 
"bits" for input;

- // -> 18, 12, 6, 0
-    private int nextout = -8;    // next available "off" in 
"bits" for output;
- // -> 8, 0, -8 (no byte for 
output)

+    private final int[] base64; // base64 -> byte mapping
+    private int bits = 0;   // 24-bit buffer for decoding
+
+    /* writing bit pos inside bits; one of 24 (left, msb), 18, 
12, 6, 0 */

+    private int wpos = 0;
+
+    /* reading bit pos inside bits: one of 24 (left, msb), 16, 8, 
0 */

+    private int rpos = 0;
+
 private boolean eof = false;
 private boolean closed = false;

@@ -983,107 +986,156 @@
 return read(sbBuf, 0, 1) == -1 ? -1 : sbBuf[0] & 0xff;
 }

-    private int eof(byte[] b, int off, int len, int oldOff)
-    throws IOException
-    {
+    private int leftovers(byte[] b, int off, int pos, int limit) {
 eof = true;
-    if (nextin != 18) {
-    if (nextin == 12)
-    throw new IOException("Base64 stream has one 
un-decoded dangling byte.");

-    // treat ending xx/xxx without padding character legal.
-    // same logic as v == '=' below
-    b[off++] = (byte)(bits >> (16));
-    if (nextin == 0) {   // only one padding byte
-    if (len == 1) {  // no enough output space
-    bits >>= 8;  // shift to lowest byte
-    nextout = 0;
-    } else {
-    b[off++] = (byte) (bits >>  8);
-    }
-    }
+
+    /*
+ * We use a loop here, as this method is executed only a 
few times.
+ * Unrolling the loop would probably not contribute much 
here.

+ */
+    while (rpos - 8 >= wpos & pos != limit) {
+    b[pos++] = (byte) (bits >> (rpos -= 8));
 }
-    return off == oldOff ? -1 : off - oldOff;
+    return pos - off != 0 || rpos - 8 >= wpos ? pos - off : -1;
 }

-    private int padding(byte[] b, int off, int len, int oldOff)
-    throws IOException
-    {
-    // = shiftto==18 unnecessary padding
-    // x=    shiftto==12 dangling x, invalid unit
-    // xx=   shiftto==6 && missing last '='
-    // xx=y  or last is not '='
-    if (nextin == 18 || nextin == 12 ||
-

Re: RFR: 8249543: Force DirectBufferAllocTest to run with -ExplicitGCInvokesConcurrent

[Roman Kennke]

Hi Alan,

> On 15/07/2020 18:20, rken...@redhat.com wrote:
> > DirectBufferAllocTest is unreliable when running with
> > +ExplicitGCInvokesConcurrent, because allocating DBB spreads
> > System.gc() calls all over concurrent GC cycles. It becomes more
> > reliable when running with -ExplicitGCInvokesConcurrent.
> > (Shenandoah
> > defaults to +ExplicitGCInvokesConcurrent, other GCs don't as far as
> > I
> > know.)
> > 
> > Bug:
> > https://bugs.openjdk.java.net/browse/JDK-8249543
> > Webrev:
> > http://cr.openjdk.java.net/~rkennke/JDK-8249543/webrev.00/
> > 
> > Ok?
> > 
> I guess this is okay but if -ExplicitGCInvokesConcurrent is the
> default 
> then doesn't it break RMI DGC?

Why would it? Can you explain? (-ExplicitGCInvokesConcurrent is the
default for all GCs but Shenandoah, and has been that way forever. Do
you mean +ExplicitGCInvokesConcurrent?)

Here's some context from our perspective:

Normally, when System.gc() is called, it invokes a STW garbage
collection. For most GCs that has been that way forever. This is what
-ExplicitGCInvokesConcurrent implies. In Shenandoah, we opted to do
+ExplicitGCInvokesConcurrent instead. This means that when System.gc()
is called, a *concurrent* collection cycle is started, and the calling
thread will wait for that to complete (and other threads will keep on
running - unless they also call System.gc() ).

It breaks this test because all test threads are hammering the GC with
System.gc(), the first one will trigger the start of a concurrent GC,
and the other ones will line up while concurrent GC is running. This is
normally ok. However, the test (or even DirectByteBuffer allocation
routine in Bits.java) is also over-assuming that when System.gc()
returns (and Cleaner thread did its thing), it could now allocate
native memory. However, when lots of test threads are competing for
this, the last one could already been outrun by the first ones that are
rescheduled already. The additional concurrency introduced by
concurrent GC, plus a bunch of wrinkles in our implementation (e.g. the
cleaner can run concurrently with ongoing GC, and not after the GC as
it would do with STW GC) makes this test spuriously fail with
Shenandoah. Forcing it to -ExplicitGCInvokesConcurrent makes it more
reliable. But as far as I can tell, the test is intrinsically
unreliable, but I'm also not sure how it could be made better (or the
DBB allocator even).


>   Are you sure this is the only test that 
> fails?

So far, yes. Can you point me to specific tests that you would expect
to fail?

Roman

Re: RFR: 8248655: Support supplementary characters in String case insensitive operations

[James Laskey]





> On Jul 15, 2020, at 4:32 PM, Joe Wang  wrote:
> 
> Jim: I was referring to the rest of the process (the calls to 
> toCodePoint/toUpperCase/toLowerCase), not isHighSurrogate itself. But Roger 
> has a more comprehensive review on performance, and Naoto is planning on 
> preparing a JMH test.
> 
> -Joe
> 
>> On 7/15/2020 11:46 AM, Jim Laskey wrote:
>> Joe: This is a defensive approach that I believe has minimal cost.
>> 
>> public static boolean isHighSurrogate(char ch) {
>> // Help VM constant-fold; MAX_HIGH_SURROGATE + 1 == MIN_LOW_SURROGATE
>> return ch >= MIN_HIGH_SURROGATE && ch < (MAX_HIGH_SURROGATE + 1);
>> }
>> 
>> 
 On Jul 15, 2020, at 3:32 PM, naoto.s...@oracle.com wrote:
>>> 
>>> Hi Joe,
>>> 
>>> Thank you for your review.
>>> 
>>> On 7/15/20 10:57 AM, Joe Wang wrote:
 Hi Naoto,
 In StringUTF16.java, if one is isHighSurrogate and the other not, you may 
 quickly return without going through the rest of the process, probably not 
 significant as cp1 and cp2 and/or u1 and u2 won't be equal anyways. But it 
 could skip a couple of toCodePoint/toUpperCase/toLowerCase calls.
>>> Yes, that is correct as of now, which is based on the assumption that case 
>>> mappings do not cross BMP and supplementary planes boundary. I could not 
>>> find any description where that's given or not. So I just took it to be 
>>> safe.
>>> 
>>> Naoto
>>> 
 -Joe
 On 7/15/20 9:00 AM, naoto.s...@oracle.com wrote:
> Hello,
> 
> Please review the fix to the following issues:
> 
> https://bugs.openjdk.java.net/browse/JDK-8248655
> https://bugs.openjdk.java.net/browse/JDK-8248434
> 
> The proposed changeset and its CSR are located at:
> 
> https://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8248664
> 
> A bug was filed against SimpleDateFormat (8248434) where case-insensitive 
> date format/parse failed in some of the new locales in JDK15. The root 
> cause was that case-insensitive String.regionMatches() method did not 
> work with supplementary characters. The problem is that the method's spec 
> does not expect case mappings of supplementary characters, possibly 
> because it was overlooked in the first place, JSR 204 - "Unicode 
> Supplementary Character support". Similar behavior is observed in other 
> two case-insensitive methods, i.e., compareToIgnoreCase() and 
> equalsIgnoreCase().
> 
> The fix is straightforward to compare strings by code point basis, 
> instead of code unit (16bit "char") basis. Technically this change will 
> introduce a backward incompatibility, but I believe it is an 
> incompatibility to wrong behavior, not true to the meaning of those 
> methods' expectations.
> 
> Naoto
> 

Re: RFR: 8248655: Support supplementary characters in String case insensitive operations

[Joe Wang]

Jim: I was referring to the rest of the process (the calls to 
toCodePoint/toUpperCase/toLowerCase), not isHighSurrogate itself. But 
Roger has a more comprehensive review on performance, and Naoto is 
planning on preparing a JMH test.


-Joe

On 7/15/2020 11:46 AM, Jim Laskey wrote:

Joe: This is a defensive approach that I believe has minimal cost.

 public static boolean isHighSurrogate(char ch) {
 // Help VM constant-fold; MAX_HIGH_SURROGATE + 1 == MIN_LOW_SURROGATE
 return ch >= MIN_HIGH_SURROGATE && ch < (MAX_HIGH_SURROGATE + 1);
 }



On Jul 15, 2020, at 3:32 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Thank you for your review.

On 7/15/20 10:57 AM, Joe Wang wrote:

Hi Naoto,
In StringUTF16.java, if one is isHighSurrogate and the other not, you may 
quickly return without going through the rest of the process, probably not 
significant as cp1 and cp2 and/or u1 and u2 won't be equal anyways. But it 
could skip a couple of toCodePoint/toUpperCase/toLowerCase calls.

Yes, that is correct as of now, which is based on the assumption that case 
mappings do not cross BMP and supplementary planes boundary. I could not find 
any description where that's given or not. So I just took it to be safe.

Naoto


-Joe
On 7/15/20 9:00 AM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issues:

https://bugs.openjdk.java.net/browse/JDK-8248655
https://bugs.openjdk.java.net/browse/JDK-8248434

The proposed changeset and its CSR are located at:

https://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8248664

A bug was filed against SimpleDateFormat (8248434) where case-insensitive date 
format/parse failed in some of the new locales in JDK15. The root cause was that 
case-insensitive String.regionMatches() method did not work with supplementary 
characters. The problem is that the method's spec does not expect case mappings of 
supplementary characters, possibly because it was overlooked in the first place, JSR 204 
- "Unicode Supplementary Character support". Similar behavior is observed in 
other two case-insensitive methods, i.e., compareToIgnoreCase() and equalsIgnoreCase().

The fix is straightforward to compare strings by code point basis, instead of code unit 
(16bit "char") basis. Technically this change will introduce a backward 
incompatibility, but I believe it is an incompatibility to wrong behavior, not true to 
the meaning of those methods' expectations.

Naoto

Re: RFR: 8248655: Support supplementary characters in String case insensitive operations

[Jim Laskey]

Joe: This is a defensive approach that I believe has minimal cost.

public static boolean isHighSurrogate(char ch) {
// Help VM constant-fold; MAX_HIGH_SURROGATE + 1 == MIN_LOW_SURROGATE
return ch >= MIN_HIGH_SURROGATE && ch < (MAX_HIGH_SURROGATE + 1);
}


> On Jul 15, 2020, at 3:32 PM, naoto.s...@oracle.com wrote:
> 
> Hi Joe,
> 
> Thank you for your review.
> 
> On 7/15/20 10:57 AM, Joe Wang wrote:
>> Hi Naoto,
>> In StringUTF16.java, if one is isHighSurrogate and the other not, you may 
>> quickly return without going through the rest of the process, probably not 
>> significant as cp1 and cp2 and/or u1 and u2 won't be equal anyways. But it 
>> could skip a couple of toCodePoint/toUpperCase/toLowerCase calls.
> 
> Yes, that is correct as of now, which is based on the assumption that case 
> mappings do not cross BMP and supplementary planes boundary. I could not find 
> any description where that's given or not. So I just took it to be safe.
> 
> Naoto
> 
>> -Joe
>> On 7/15/20 9:00 AM, naoto.s...@oracle.com wrote:
>>> Hello,
>>> 
>>> Please review the fix to the following issues:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8248655
>>> https://bugs.openjdk.java.net/browse/JDK-8248434
>>> 
>>> The proposed changeset and its CSR are located at:
>>> 
>>> https://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8248664
>>> 
>>> A bug was filed against SimpleDateFormat (8248434) where case-insensitive 
>>> date format/parse failed in some of the new locales in JDK15. The root 
>>> cause was that case-insensitive String.regionMatches() method did not work 
>>> with supplementary characters. The problem is that the method's spec does 
>>> not expect case mappings of supplementary characters, possibly because it 
>>> was overlooked in the first place, JSR 204 - "Unicode Supplementary 
>>> Character support". Similar behavior is observed in other two 
>>> case-insensitive methods, i.e., compareToIgnoreCase() and 
>>> equalsIgnoreCase().
>>> 
>>> The fix is straightforward to compare strings by code point basis, instead 
>>> of code unit (16bit "char") basis. Technically this change will introduce a 
>>> backward incompatibility, but I believe it is an incompatibility to wrong 
>>> behavior, not true to the meaning of those methods' expectations.
>>> 
>>> Naoto

Re: RFR: 8249543: Force DirectBufferAllocTest to run with -ExplicitGCInvokesConcurrent

[Alan Bateman]

On 15/07/2020 18:20, rken...@redhat.com wrote:

DirectBufferAllocTest is unreliable when running with
+ExplicitGCInvokesConcurrent, because allocating DBB spreads
System.gc() calls all over concurrent GC cycles. It becomes more
reliable when running with -ExplicitGCInvokesConcurrent. (Shenandoah
defaults to +ExplicitGCInvokesConcurrent, other GCs don't as far as I
know.)

Bug:
https://bugs.openjdk.java.net/browse/JDK-8249543
Webrev:
http://cr.openjdk.java.net/~rkennke/JDK-8249543/webrev.00/

Ok?

I guess this is okay but if -ExplicitGCInvokesConcurrent is the default 
then doesn't it break RMI DGC?  Are you sure this is the only test that 
fails?


-Alan

RFR: JDK-8249289: Exception thrown when --temp points to non-existant directory

[Andy Herrick]

Please review the jpackage fix for issue [1] at [2].

This fixes the regression in JDK16 which caused exception when an empty 
directory was used int the --temp jpackage option, and added to the 
existing test.


[1] - https://bugs.openjdk.java.net/browse/JDK-8249289

[2[ - http://cr.openjdk.java.net/~herrick/8249289/webrev.02/

/Andy

Re: RFR: 8248655: Support supplementary characters in String case insensitive operations

[naoto . sato]

Hi Joe,

Thank you for your review.

On 7/15/20 10:57 AM, Joe Wang wrote:

Hi Naoto,

In StringUTF16.java, if one is isHighSurrogate and the other not, you 
may quickly return without going through the rest of the process, 
probably not significant as cp1 and cp2 and/or u1 and u2 won't be equal 
anyways. But it could skip a couple of 
toCodePoint/toUpperCase/toLowerCase calls.


Yes, that is correct as of now, which is based on the assumption that 
case mappings do not cross BMP and supplementary planes boundary. I 
could not find any description where that's given or not. So I just took 
it to be safe.


Naoto



-Joe

On 7/15/20 9:00 AM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issues:

https://bugs.openjdk.java.net/browse/JDK-8248655
https://bugs.openjdk.java.net/browse/JDK-8248434

The proposed changeset and its CSR are located at:

https://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8248664

A bug was filed against SimpleDateFormat (8248434) where 
case-insensitive date format/parse failed in some of the new locales 
in JDK15. The root cause was that case-insensitive 
String.regionMatches() method did not work with supplementary 
characters. The problem is that the method's spec does not expect case 
mappings of supplementary characters, possibly because it was 
overlooked in the first place, JSR 204 - "Unicode Supplementary 
Character support". Similar behavior is observed in other two 
case-insensitive methods, i.e., compareToIgnoreCase() and 
equalsIgnoreCase().


The fix is straightforward to compare strings by code point basis, 
instead of code unit (16bit "char") basis. Technically this change 
will introduce a backward incompatibility, but I believe it is an 
incompatibility to wrong behavior, not true to the meaning of those 
methods' expectations.


Naoto

Re: RFR: 8248655: Support supplementary characters in String case insensitive operations

[naoto . sato]

Hi Roger,

Thank you for your review and suggestions.

On 7/15/20 10:56 AM, Roger Riggs wrote:

Hi Naoto,

Given the extra tests in the body of the loop, I think its worth finding 
or creating

a JMH test for this and checking the performance.


Good point.



With performance in mind, I would try to fall back to the UC/LC 
conversions only

when the bytes don't match.  See java.util.Arrays.mismatch(byte[], byte[]).

It might even be worth finding the mismatch in the byte arrays before even
starting to look at the characters.


Agree.



There's also an option to assemble 4 bytes at a time and compare the int's.
If they are equal you are ahead of the game.  If not, back off to comparing
the characters and checking for surrogates.  The backoff code will be a bit
messier though.


Sure int comparison would be faster, I am not quite sure that would be 
worth compared to more complicated recovery code though.




Also, compareToCI and regionMatchesCI could share the implementation of 
the inner loop.


Yes.



If k1 and k2 ever get out of sync, isn't that failed assertion, so why 
have two indexes.


This is for preventing possible case maps where one in BMP and the other 
in supplementary, thus the indices could be different.




The loop will have fewer checks against the length of it processes len-1 
chars

and then have a check if there is a final char to be checked.
it can always know there is another char and can blindly get it.


Maybe, but could be complicated if the last char is a low surrogate.

Anyway, I will come up with a JMH test case and revise the webrev.

Naoto



Regards, Roger


On 7/15/20 12:00 PM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issues:

https://bugs.openjdk.java.net/browse/JDK-8248655
https://bugs.openjdk.java.net/browse/JDK-8248434

The proposed changeset and its CSR are located at:

https://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8248664

A bug was filed against SimpleDateFormat (8248434) where 
case-insensitive date format/parse failed in some of the new locales 
in JDK15. The root cause was that case-insensitive 
String.regionMatches() method did not work with supplementary 
characters. The problem is that the method's spec does not expect case 
mappings of supplementary characters, possibly because it was 
overlooked in the first place, JSR 204 - "Unicode Supplementary 
Character support". Similar behavior is observed in other two 
case-insensitive methods, i.e., compareToIgnoreCase() and 
equalsIgnoreCase().


The fix is straightforward to compare strings by code point basis, 
instead of code unit (16bit "char") basis. Technically this change 
will introduce a backward incompatibility, but I believe it is an 
incompatibility to wrong behavior, not true to the meaning of those 
methods' expectations.


Naoto

Re: RFR: 8248655: Support supplementary characters in String case insensitive operations

[Joe Wang]

Hi Naoto,

In StringUTF16.java, if one is isHighSurrogate and the other not, you 
may quickly return without going through the rest of the process, 
probably not significant as cp1 and cp2 and/or u1 and u2 won't be equal 
anyways. But it could skip a couple of 
toCodePoint/toUpperCase/toLowerCase calls.


-Joe

On 7/15/20 9:00 AM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issues:

https://bugs.openjdk.java.net/browse/JDK-8248655
https://bugs.openjdk.java.net/browse/JDK-8248434

The proposed changeset and its CSR are located at:

https://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8248664

A bug was filed against SimpleDateFormat (8248434) where 
case-insensitive date format/parse failed in some of the new locales 
in JDK15. The root cause was that case-insensitive 
String.regionMatches() method did not work with supplementary 
characters. The problem is that the method's spec does not expect case 
mappings of supplementary characters, possibly because it was 
overlooked in the first place, JSR 204 - "Unicode Supplementary 
Character support". Similar behavior is observed in other two 
case-insensitive methods, i.e., compareToIgnoreCase() and 
equalsIgnoreCase().


The fix is straightforward to compare strings by code point basis, 
instead of code unit (16bit "char") basis. Technically this change 
will introduce a backward incompatibility, but I believe it is an 
incompatibility to wrong behavior, not true to the meaning of those 
methods' expectations.


Naoto

Re: RFR: 8248655: Support supplementary characters in String case insensitive operations

[Roger Riggs]

Hi Naoto,

Given the extra tests in the body of the loop, I think its worth finding 
or creating

a JMH test for this and checking the performance.

With performance in mind, I would try to fall back to the UC/LC 
conversions only

when the bytes don't match.  See java.util.Arrays.mismatch(byte[], byte[]).

It might even be worth finding the mismatch in the byte arrays before even
starting to look at the characters.

There's also an option to assemble 4 bytes at a time and compare the int's.
If they are equal you are ahead of the game.  If not, back off to comparing
the characters and checking for surrogates.  The backoff code will be a bit
messier though.

Also, compareToCI and regionMatchesCI could share the implementation of 
the inner loop.


If k1 and k2 ever get out of sync, isn't that failed assertion, so why 
have two indexes.


The loop will have fewer checks against the length of it processes len-1 
chars

and then have a check if there is a final char to be checked.
it can always know there is another char and can blindly get it.

Regards, Roger


On 7/15/20 12:00 PM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issues:

https://bugs.openjdk.java.net/browse/JDK-8248655
https://bugs.openjdk.java.net/browse/JDK-8248434

The proposed changeset and its CSR are located at:

https://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8248664

A bug was filed against SimpleDateFormat (8248434) where 
case-insensitive date format/parse failed in some of the new locales 
in JDK15. The root cause was that case-insensitive 
String.regionMatches() method did not work with supplementary 
characters. The problem is that the method's spec does not expect case 
mappings of supplementary characters, possibly because it was 
overlooked in the first place, JSR 204 - "Unicode Supplementary 
Character support". Similar behavior is observed in other two 
case-insensitive methods, i.e., compareToIgnoreCase() and 
equalsIgnoreCase().


The fix is straightforward to compare strings by code point basis, 
instead of code unit (16bit "char") basis. Technically this change 
will introduce a backward incompatibility, but I believe it is an 
incompatibility to wrong behavior, not true to the meaning of those 
methods' expectations.


Naoto

Re: RFR: JDK-8249258 java/util/StringJoiner/StringJoinerTest.java failed due to OOM (JDK 15)

[Jim Laskey]

In StringJoiner::toString

final int addLen = prefix.length() + suffix.length();

Looks suspicious. There is no check for overflow here. Looking at the 
constructor it should have raised a OOM much earlier. Will investigate and file 
a bug.

Thanks.

-- Jim


> On Jul 15, 2020, at 1:21 PM, Thomas Schatzl  wrote:
> 
> Hi,
> 
>  I looked a bit at the allocations themselves, but first answering questions.
> 
> On 15.07.20 15:25, David Holmes wrote:
> > On 15/07/2020 10:18 pm, Jim Laskey wrote:
> >> Thomas explained: That large objects are never moved (outstanding
> >> issue) So, it's possible to fragment the -Xmx4g such that a 2G object
> >> can't be allocated,
> >
> > Naively one would expect that whatever memory was consumed by
> >
> > String maxString = "*".repeat(MAX_ARRAY_LENGTH);
> >
> > in OOM2, would be fully freed and available for use by the same
> > statememt in OOM3. But without knowing the exact allocation patterns
> 
> This is true.
> 
> Augmenting OOM3 code with allocations/gcs:
> 
> Heap has 2.05g (1030 regions)
> Allocation 1 for 1025 regions, 2g
> 
>  - concurrent mark start pause because of humongous allocation attempt; heap 
> has 2.05g
>  - not enough free space, so do a young collection, elevate to full 
> collection -> heap shrunk to 2M
>  - allocation goes through
> 
> 1)  String maxString = "*".repeat(MAX_ARRAY_LENGTH);
>try {
> 
> Allocation 2 for 2048 regions(!), 4g
>  - concurrent start pause because of humongous allocation attempt
>  - not enough free space, so do a young collection, elevate to full 
> collection -> heap stays at 2.05g -> OOME
> 
> 2)  new StringJoiner(maxString, "", maxString).toString();
>fail("Should have thrown OutOfMemoryError");
> 
> Two observations:
> - I ask myselves how that test could have ever failed (i.e. not throw an 
> OOME). A 4g allocation on a 4g heap can in practice never succeed. This is 
> very suspicious.
> 
> - It is also interesting why Allocation 2 internally has been truncated to a 
> 2048 region allocation. It should be 2049 (4g + 16 bytes header?). Checking 
> at lower layers, memory management get a request for 4294967296 bytes which 
> is exactly 2^32... this is too small for that object. Something is truncating 
> that string. Printing it gives a length of 2147483639 chars (i.e. 2^32-1-9). 
> I assume that is correct to silently truncate the result string?
> 
> Thanks,
>  Thomas

RFR: 8249543: Force DirectBufferAllocTest to run with -ExplicitGCInvokesConcurrent

[rkennke]

DirectBufferAllocTest is unreliable when running with
+ExplicitGCInvokesConcurrent, because allocating DBB spreads
System.gc() calls all over concurrent GC cycles. It becomes more
reliable when running with -ExplicitGCInvokesConcurrent. (Shenandoah
defaults to +ExplicitGCInvokesConcurrent, other GCs don't as far as I
know.)

Bug:
https://bugs.openjdk.java.net/browse/JDK-8249543
Webrev:
http://cr.openjdk.java.net/~rkennke/JDK-8249543/webrev.00/

Ok?

Thanks,
Roman

Re: RFR: 8248655: Support supplementary characters in String case insensitive operations

[naoto . sato]

Thank you, Jim, for the quick review!

On 7/15/20 9:26 AM, Jim Laskey wrote:

I think I'm good with this. +1

Asides:

  325 int cp1 = (int)getChar(value, k1);
  326 int cp2 = (int)getChar(other, k2);

I would be tempted to short cut by exiting when not equal, but I think we 
agreed we need to allow for upper/lowers on different planes.
  
In the UTF-16 code I was trying to think of how your could exhaust the first string and not the second, and still have their lengths the same. I think I have convinced myself that it's not possible as long as surrogates always map upper/lowers to surrogates (two chars each.)


Right. All code points as of JDK15/6 is in the same plane, thus the 
lengths won't change. I was trying to create a test case for that 
hypothetical situation, but gave up because each character case map is 
embedded in Unicode Character Database, which cannot be modified.


Naoto



Cheers,

-- Jim






On Jul 15, 2020, at 1:00 PM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issues:

https://bugs.openjdk.java.net/browse/JDK-8248655
https://bugs.openjdk.java.net/browse/JDK-8248434

The proposed changeset and its CSR are located at:

https://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8248664

A bug was filed against SimpleDateFormat (8248434) where case-insensitive date 
format/parse failed in some of the new locales in JDK15. The root cause was that 
case-insensitive String.regionMatches() method did not work with supplementary 
characters. The problem is that the method's spec does not expect case mappings of 
supplementary characters, possibly because it was overlooked in the first place, JSR 204 
- "Unicode Supplementary Character support". Similar behavior is observed in 
other two case-insensitive methods, i.e., compareToIgnoreCase() and equalsIgnoreCase().

The fix is straightforward to compare strings by code point basis, instead of code unit 
(16bit "char") basis. Technically this change will introduce a backward 
incompatibility, but I believe it is an incompatibility to wrong behavior, not true to 
the meaning of those methods' expectations.

Naoto

Re: RFR: 8248655: Support supplementary characters in String case insensitive operations

[Jim Laskey]

I think I'm good with this. +1

Asides:

 325 int cp1 = (int)getChar(value, k1);
 326 int cp2 = (int)getChar(other, k2);

I would be tempted to short cut by exiting when not equal, but I think we 
agreed we need to allow for upper/lowers on different planes.
 
In the UTF-16 code I was trying to think of how your could exhaust the first 
string and not the second, and still have their lengths the same. I think I 
have convinced myself that it's not possible as long as surrogates always map 
upper/lowers to surrogates (two chars each.)

Cheers,

-- Jim





> On Jul 15, 2020, at 1:00 PM, naoto.s...@oracle.com wrote:
> 
> Hello,
> 
> Please review the fix to the following issues:
> 
> https://bugs.openjdk.java.net/browse/JDK-8248655
> https://bugs.openjdk.java.net/browse/JDK-8248434
> 
> The proposed changeset and its CSR are located at:
> 
> https://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8248664
> 
> A bug was filed against SimpleDateFormat (8248434) where case-insensitive 
> date format/parse failed in some of the new locales in JDK15. The root cause 
> was that case-insensitive String.regionMatches() method did not work with 
> supplementary characters. The problem is that the method's spec does not 
> expect case mappings of supplementary characters, possibly because it was 
> overlooked in the first place, JSR 204 - "Unicode Supplementary Character 
> support". Similar behavior is observed in other two case-insensitive methods, 
> i.e., compareToIgnoreCase() and equalsIgnoreCase().
> 
> The fix is straightforward to compare strings by code point basis, instead of 
> code unit (16bit "char") basis. Technically this change will introduce a 
> backward incompatibility, but I believe it is an incompatibility to wrong 
> behavior, not true to the meaning of those methods' expectations.
> 
> Naoto

Re: RFR: JDK-8249258 java/util/StringJoiner/StringJoinerTest.java failed due to OOM (JDK 15)

[Thomas Schatzl]

Hi,

  I looked a bit at the allocations themselves, but first answering 
questions.


On 15.07.20 15:25, David Holmes wrote:
> On 15/07/2020 10:18 pm, Jim Laskey wrote:
>> Thomas explained: That large objects are never moved (outstanding
>> issue) So, it's possible to fragment the -Xmx4g such that a 2G object
>> can't be allocated,
>
> Naively one would expect that whatever memory was consumed by
>
> String maxString = "*".repeat(MAX_ARRAY_LENGTH);
>
> in OOM2, would be fully freed and available for use by the same
> statememt in OOM3. But without knowing the exact allocation patterns

This is true.

Augmenting OOM3 code with allocations/gcs:

Heap has 2.05g (1030 regions)
Allocation 1 for 1025 regions, 2g

  - concurrent mark start pause because of humongous allocation 
attempt; heap has 2.05g
  - not enough free space, so do a young collection, elevate to full 
collection -> heap shrunk to 2M

  - allocation goes through

1)  String maxString = "*".repeat(MAX_ARRAY_LENGTH);
try {

Allocation 2 for 2048 regions(!), 4g
  - concurrent start pause because of humongous allocation attempt
  - not enough free space, so do a young collection, elevate to full 
collection -> heap stays at 2.05g -> OOME


2)  new StringJoiner(maxString, "", maxString).toString();
fail("Should have thrown OutOfMemoryError");

Two observations:
- I ask myselves how that test could have ever failed (i.e. not throw an 
OOME). A 4g allocation on a 4g heap can in practice never succeed. This 
is very suspicious.


- It is also interesting why Allocation 2 internally has been truncated 
to a 2048 region allocation. It should be 2049 (4g + 16 bytes header?). 
Checking at lower layers, memory management get a request for 4294967296 
bytes which is exactly 2^32... this is too small for that object. 
Something is truncating that string. Printing it gives a length of 
2147483639 chars (i.e. 2^32-1-9). I assume that is correct to silently 
truncate the result string?


Thanks,
  Thomas

RFR: 8248655: Support supplementary characters in String case insensitive operations

[naoto . sato]

Hello,

Please review the fix to the following issues:

https://bugs.openjdk.java.net/browse/JDK-8248655
https://bugs.openjdk.java.net/browse/JDK-8248434

The proposed changeset and its CSR are located at:

https://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8248664

A bug was filed against SimpleDateFormat (8248434) where 
case-insensitive date format/parse failed in some of the new locales in 
JDK15. The root cause was that case-insensitive String.regionMatches() 
method did not work with supplementary characters. The problem is that 
the method's spec does not expect case mappings of supplementary 
characters, possibly because it was overlooked in the first place, JSR 
204 - "Unicode Supplementary Character support". Similar behavior is 
observed in other two case-insensitive methods, i.e., 
compareToIgnoreCase() and equalsIgnoreCase().


The fix is straightforward to compare strings by code point basis, 
instead of code unit (16bit "char") basis. Technically this change will 
introduce a backward incompatibility, but I believe it is an 
incompatibility to wrong behavior, not true to the meaning of those 
methods' expectations.


Naoto

Re: RFR: JDK-8249258 java/util/StringJoiner/StringJoinerTest.java failed due to OOM (JDK 15)

[Thomas Schatzl]

Hi,

On 15.07.20 15:35, Jim Laskey wrote:

Try this:

- You have a 4G heap.
- You allocate some stuff, say 1 byte.
- You allocate a 2G object - so there is only 2G - 1, left. Not enough space 
for another 2G object.
- But you do allocate 1 byte.
- You have 1 byte, 2G and 1 byte.
- You free the original 2G object.
- But something allocates 1 byte in it's old space.
- Now there is no range that that can accommodate a 2G object. OOM.


  just for clarification: it's a bit more elaborate than that to get 
into this situation as G1 will try a full collection that will compact 
these 1 byte "stuff" into a contiguous range of memory.


G1 at this time will never move "large" (>= heap region size / 2; at 4g 
heap region size is 2M iirc) objects though. I.e. with a suitably placed 
single "large" object in that 4g heap (like covering the middle center 
regions of the heap, i.e. regions #1023 and #1024) you can make 
allocation of that 2g object impossible.


Thanks,
  Thomas

RE: [PING?] RFR(s): 8247863: Unreachable code in OperatingSystemImpl.getTotalSwapSpaceSize()

[Baesken, Matthias]

Hello Severin ,  

> the new cgroups implementation
> supporting v1 and v2 Metrics.getMemoryAndSwapLimit() will never return   0

Wouldn’t it be possible that the  coding  of  getMemoryAndSwapLimit   returns a 
negative value  (might not happen on a "healthy" system but you never know) :

jdk/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1Subsystem.java

444public long getMemoryAndSwapLimit() {
445long retval = getLongValue(memory, "memory.memsw.limit_in_bytes");
446if (retval > CgroupV1SubsystemController.UNLIMITED_MIN) {
447if (memory.isHierarchical()) {
448// memory.memsw.limit_in_bytes returned unlimited, attempt
449// hierarchical memory limit
450String match = "hierarchical_memsw_limit";
451retval = 
CgroupV1SubsystemController.getLongValueMatchingLine(memory,
452"memory.stat",
453match);
454}
455}
456return CgroupV1SubsystemController.longValOrUnlimited(retval);
457}


jdk/src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java

278public long getMemoryAndSwapLimit() {
279String strVal = CgroupSubsystemController.getStringValue(unified, 
"memory.swap.max");
280return limitFromString(strVal);
281}

So  the check you want to clean up does no harm, and might handle "strange" 
cases - why not keep it?



Best regards, Matthias


-Original Message-
From: Severin Gehwolf  
Sent: Mittwoch, 15. Juli 2020 11:47
To: core-libs-dev ; serviceability-dev 

Cc: Baesken, Matthias ; Bob Vandette 

Subject: Re: [PING?] RFR(s): 8247863: Unreachable code in 
OperatingSystemImpl.getTotalSwapSpaceSize()

Anyone?

On Mon, 2020-06-29 at 17:53 +0200, Severin Gehwolf wrote:
> Hi,
> 
> Could I please get a review of this dead-code removal? During review of
> JDK-8244500 it was discovered that with the new cgroups implementation
> supporting v1 and v2 Metrics.getMemoryAndSwapLimit() will never return
> 0 when relevant cgroup files are missing. E.g. on a system where the
> kernel doesn't support swap limit capabilities. Therefore this code
> introduced with JDK-8236617 can no longer be reached and should get
> removed.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8247863
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8247863/01/webrev/
> 
> Testing: Matthias tested this on the affected system and it did pass
> for him. Docker tests on cgroup v1 and cgroup v2.
> 
> Thanks,
> Severin

Re: RFR: JDK-8249258 java/util/StringJoiner/StringJoinerTest.java failed due to OOM (JDK 15)

[Jim Laskey]

Try this:

- You have a 4G heap.
- You allocate some stuff, say 1 byte.
- You allocate a 2G object - so there is only 2G - 1, left. Not enough space 
for another 2G object.
- But you do allocate 1 byte.
- You have 1 byte, 2G and 1 byte.
- You free the original 2G object.
- But something allocates 1 byte in it's old space.
- Now there is no range that that can accommodate a 2G object. OOM.


> On Jul 15, 2020, at 10:25 AM, David Holmes  wrote:
> 
> On 15/07/2020 10:18 pm, Jim Laskey wrote:
>> Thomas explained: That large objects are never moved (outstanding issue) So, 
>> it's possible to fragment the -Xmx4g such that a 2G object can't be 
>> allocated,
> 
> Naively one would expect that whatever memory was consumed by
> 
> String maxString = "*".repeat(MAX_ARRAY_LENGTH);
> 
> in OOM2, would be fully freed and available for use by the same statememt in 
> OOM3. But without knowing the exact allocation patterns ...
> 
> Cheers,
> David
> -
> 
>>> On Jul 15, 2020, at 8:55 AM, Jim Laskey  wrote:
>>> 
>>> Since the MAX_STRING is 2Gbytes, I wonder if those strings are allocated on 
>>> the side per se and are subject to the platforms VM whims.
>>> 
 On Jul 14, 2020, at 8:36 PM, David Holmes  wrote:
 
 On 15/07/2020 5:35 am, Roger Riggs wrote:
> Looks good.
> Though it does seem like the VM should have been able to reclaim enough 
> memory between tests to not need to throw OOME.
 
 I'd have to agree with that - something seems strange here.
 
 The first OOME in OOM1 is not actually Java heap exhaustion, but 
 "Requested array size exceeds VM limit", so it could in theory consume 
 sufficient Java heap that the "repeat" in OOM2 actually exhausts the heap. 
 But that isn't what we see - the unexpected OOME is in OOM3. Before OOM2 
 runs out of memory the GC should have reclaimed everything from OOM1. 
 Similarly before OOM3 runs out of memory the GC should have reclaimed 
 everything from OOM2 - which should make it impossible for the same 
 "repeat" call to hit an OOME in OOM3. Unless the test is being run 
 concurrrently in the same VM as other tests?
 
 Cheers,
 David
 
> Thanks, Roger
> On 7/14/20 2:23 PM, Daniel D. Daugherty wrote:
>> On 7/14/20 2:09 PM, Jim Laskey wrote:
>>> Adding Daniel
>>> 
 Begin forwarded message:
 
 *From: *Jim Laskey >>> >
 *Subject: **RFR: JDK-8249258 
 java/util/StringJoiner/StringJoinerTest.java failed due to OOM (JDK 
 15)*
 *Date: *July 14, 2020 at 2:01:09 PM ADT
 *To: *core-libs-dev >>> >
 
 The test was failing on a newly added mac mini. I've reduced the 
 memory stress by introducing a shared maximum sized string. I don't 
 recommend reducing the 4G memory requirement -- multiple new large 
 objects are constructed in the tests hoping to create an OOM.
 
 JBS: https://bugs.openjdk.java.net/browse/JDK-8249258
 webrev: http://cr.openjdk.java.net/~jlaskey/8249258/webrev-01
>> 
>> test/jdk/java/util/StringJoiner/StringJoinerTest.java
>>No comments on the changes.
>> 
>> This should make it less likely to cause an OOM in an unexpected place.
>> Thanks for fixing this so quickly.
>> 
>> Thumbs up. Don't know if your team updates copyrights as you
>> touch files, but if you do, then this one needs it...
>> 
>> Dan
>> 
>> 
 
 
 
 
 
>>> 
>> 
>>> 

Re: RFR: JDK-8249258 java/util/StringJoiner/StringJoinerTest.java failed due to OOM (JDK 15)

[David Holmes]

On 15/07/2020 10:18 pm, Jim Laskey wrote:

Thomas explained: That large objects are never moved (outstanding issue) So, 
it's possible to fragment the -Xmx4g such that a 2G object can't be allocated,


Naively one would expect that whatever memory was consumed by

String maxString = "*".repeat(MAX_ARRAY_LENGTH);

in OOM2, would be fully freed and available for use by the same 
statememt in OOM3. But without knowing the exact allocation patterns ...


Cheers,
David
-


On Jul 15, 2020, at 8:55 AM, Jim Laskey  wrote:

Since the MAX_STRING is 2Gbytes, I wonder if those strings are allocated on the 
side per se and are subject to the platforms VM whims.


On Jul 14, 2020, at 8:36 PM, David Holmes  wrote:

On 15/07/2020 5:35 am, Roger Riggs wrote:

Looks good.
Though it does seem like the VM should have been able to reclaim enough memory 
between tests to not need to throw OOME.


I'd have to agree with that - something seems strange here.

The first OOME in OOM1 is not actually Java heap exhaustion, but "Requested array size exceeds VM 
limit", so it could in theory consume sufficient Java heap that the "repeat" in OOM2 actually 
exhausts the heap. But that isn't what we see - the unexpected OOME is in OOM3. Before OOM2 runs out of 
memory the GC should have reclaimed everything from OOM1. Similarly before OOM3 runs out of memory the GC 
should have reclaimed everything from OOM2 - which should make it impossible for the same "repeat" 
call to hit an OOME in OOM3. Unless the test is being run concurrrently in the same VM as other tests?

Cheers,
David


Thanks, Roger
On 7/14/20 2:23 PM, Daniel D. Daugherty wrote:

On 7/14/20 2:09 PM, Jim Laskey wrote:

Adding Daniel


Begin forwarded message:

*From: *Jim Laskey mailto:james.las...@oracle.com>>
*Subject: **RFR: JDK-8249258 java/util/StringJoiner/StringJoinerTest.java 
failed due to OOM (JDK 15)*
*Date: *July 14, 2020 at 2:01:09 PM ADT
*To: *core-libs-dev mailto:core-libs-dev@openjdk.java.net>>

The test was failing on a newly added mac mini. I've reduced the memory stress 
by introducing a shared maximum sized string. I don't recommend reducing the 4G 
memory requirement -- multiple new large objects are constructed in the tests 
hoping to create an OOM.

JBS: https://bugs.openjdk.java.net/browse/JDK-8249258
webrev: http://cr.openjdk.java.net/~jlaskey/8249258/webrev-01


test/jdk/java/util/StringJoiner/StringJoinerTest.java
No comments on the changes.

This should make it less likely to cause an OOM in an unexpected place.
Thanks for fixing this so quickly.

Thumbs up. Don't know if your team updates copyrights as you
touch files, but if you do, then this one needs it...

Dan

Re: RFR: JDK-8249258 java/util/StringJoiner/StringJoinerTest.java failed due to OOM (JDK 15)

[Jim Laskey]

Thomas explained: That large objects are never moved (outstanding issue) So, 
it's possible to fragment the -Xmx4g such that a 2G object can't be allocated,

> On Jul 15, 2020, at 8:55 AM, Jim Laskey  wrote:
> 
> Since the MAX_STRING is 2Gbytes, I wonder if those strings are allocated on 
> the side per se and are subject to the platforms VM whims.
> 
>> On Jul 14, 2020, at 8:36 PM, David Holmes  wrote:
>> 
>> On 15/07/2020 5:35 am, Roger Riggs wrote:
>>> Looks good.
>>> Though it does seem like the VM should have been able to reclaim enough 
>>> memory between tests to not need to throw OOME.
>> 
>> I'd have to agree with that - something seems strange here.
>> 
>> The first OOME in OOM1 is not actually Java heap exhaustion, but "Requested 
>> array size exceeds VM limit", so it could in theory consume sufficient Java 
>> heap that the "repeat" in OOM2 actually exhausts the heap. But that isn't 
>> what we see - the unexpected OOME is in OOM3. Before OOM2 runs out of memory 
>> the GC should have reclaimed everything from OOM1. Similarly before OOM3 
>> runs out of memory the GC should have reclaimed everything from OOM2 - which 
>> should make it impossible for the same "repeat" call to hit an OOME in OOM3. 
>> Unless the test is being run concurrrently in the same VM as other tests?
>> 
>> Cheers,
>> David
>> 
>>> Thanks, Roger
>>> On 7/14/20 2:23 PM, Daniel D. Daugherty wrote:
 On 7/14/20 2:09 PM, Jim Laskey wrote:
> Adding Daniel
> 
>> Begin forwarded message:
>> 
>> *From: *Jim Laskey > >
>> *Subject: **RFR: JDK-8249258 
>> java/util/StringJoiner/StringJoinerTest.java failed due to OOM (JDK 15)*
>> *Date: *July 14, 2020 at 2:01:09 PM ADT
>> *To: *core-libs-dev > >
>> 
>> The test was failing on a newly added mac mini. I've reduced the memory 
>> stress by introducing a shared maximum sized string. I don't recommend 
>> reducing the 4G memory requirement -- multiple new large objects are 
>> constructed in the tests hoping to create an OOM.
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8249258
>> webrev: http://cr.openjdk.java.net/~jlaskey/8249258/webrev-01
 
 test/jdk/java/util/StringJoiner/StringJoinerTest.java
No comments on the changes.
 
 This should make it less likely to cause an OOM in an unexpected place.
 Thanks for fixing this so quickly.
 
 Thumbs up. Don't know if your team updates copyrights as you
 touch files, but if you do, then this one needs it...
 
 Dan
 
 
>> 
>> 
>> 
>> 
>> 
> 
 
> 

Re: RFR: JDK-8249258 java/util/StringJoiner/StringJoinerTest.java failed due to OOM (JDK 15)

[Jim Laskey]

Since the MAX_STRING is 2Gbytes, I wonder if those strings are allocated on the 
side per se and are subject to the platforms VM whims.

> On Jul 14, 2020, at 8:36 PM, David Holmes  wrote:
> 
> On 15/07/2020 5:35 am, Roger Riggs wrote:
>> Looks good.
>> Though it does seem like the VM should have been able to reclaim enough 
>> memory between tests to not need to throw OOME.
> 
> I'd have to agree with that - something seems strange here.
> 
> The first OOME in OOM1 is not actually Java heap exhaustion, but "Requested 
> array size exceeds VM limit", so it could in theory consume sufficient Java 
> heap that the "repeat" in OOM2 actually exhausts the heap. But that isn't 
> what we see - the unexpected OOME is in OOM3. Before OOM2 runs out of memory 
> the GC should have reclaimed everything from OOM1. Similarly before OOM3 runs 
> out of memory the GC should have reclaimed everything from OOM2 - which 
> should make it impossible for the same "repeat" call to hit an OOME in OOM3. 
> Unless the test is being run concurrrently in the same VM as other tests?
> 
> Cheers,
> David
> 
>> Thanks, Roger
>> On 7/14/20 2:23 PM, Daniel D. Daugherty wrote:
>>> On 7/14/20 2:09 PM, Jim Laskey wrote:
 Adding Daniel
 
> Begin forwarded message:
> 
> *From: *Jim Laskey  >
> *Subject: **RFR: JDK-8249258 java/util/StringJoiner/StringJoinerTest.java 
> failed due to OOM (JDK 15)*
> *Date: *July 14, 2020 at 2:01:09 PM ADT
> *To: *core-libs-dev  >
> 
> The test was failing on a newly added mac mini. I've reduced the memory 
> stress by introducing a shared maximum sized string. I don't recommend 
> reducing the 4G memory requirement -- multiple new large objects are 
> constructed in the tests hoping to create an OOM.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8249258
> webrev: http://cr.openjdk.java.net/~jlaskey/8249258/webrev-01
>>> 
>>> test/jdk/java/util/StringJoiner/StringJoinerTest.java
>>> No comments on the changes.
>>> 
>>> This should make it less likely to cause an OOM in an unexpected place.
>>> Thanks for fixing this so quickly.
>>> 
>>> Thumbs up. Don't know if your team updates copyrights as you
>>> touch files, but if you do, then this one needs it...
>>> 
>>> Dan
>>> 
>>> 
> 
> 
> 
> 
> 
 
>>> 

Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

[Alexey Bakhtin]

Hello Daniel,

I’ve updated CSR as you suggested and added kerberos ldap setup commands for 
the client host in the JDK-8245527

Regards
Alexey

> On 14 Jul 2020, at 18:28, Daniel Fuchs  wrote:
> 
> Hi Alexey,
> 
> On 10/07/2020 21:37, Alexey Bakhtin wrote:
>> Updated webrev:http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v14/
> 
> In what the JNDI part is concerned this looks good to me now.
> 
> nit: java.naming/share/classes/com/sun/jndi/ldap/sasl/TlsChannelBinding.java:
> 138 }catch(NoSuchAlgorithmException | CertificateEncodingException e) 
> {
> 
> missing spaces around `catch`; No need for a new webrev.
> 
> Please make sure to update the CSR, and in particular update
> the specification section with the diffs from
> 
> src/java.naming/share/classes/module-info.java
> 
> Also I am not sure the links that are currently in the
> specification section are at their place. They may be better
> placed in the Solution section (the solution is to implement
> the client part of the channel binding as described by these
> documents in the default JNDI/LDAP/GSS provider).
> 
> Since we don't really have any end-to-end regression test
> (what we have is mostly a smoke test) - it would be good if
> you could describe in more details what you did to test your
> fix against a real server in a JBS comment in JDK-8245527 - so
> that someone (future or current maintainers) could reproduce
> this testing to verify that nothing is broken by future evolutions.
> In particular - if anything specific needs to be
> installed/configured on the test machine (LDAP server? Which?
> Is that all?)
> 
> 
> best regards,
> 
> -- daniel

Re: [PING?] RFR(s): 8247863: Unreachable code in OperatingSystemImpl.getTotalSwapSpaceSize()

[Severin Gehwolf]

Anyone?

On Mon, 2020-06-29 at 17:53 +0200, Severin Gehwolf wrote:
> Hi,
> 
> Could I please get a review of this dead-code removal? During review of
> JDK-8244500 it was discovered that with the new cgroups implementation
> supporting v1 and v2 Metrics.getMemoryAndSwapLimit() will never return
> 0 when relevant cgroup files are missing. E.g. on a system where the
> kernel doesn't support swap limit capabilities. Therefore this code
> introduced with JDK-8236617 can no longer be reached and should get
> removed.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8247863
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8247863/01/webrev/
> 
> Testing: Matthias tested this on the affected system and it did pass
> for him. Docker tests on cgroup v1 and cgroup v2.
> 
> Thanks,
> Severin

Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

[Aleks Efimov]

Hi Alexey,

Thanks for addressing comments and answering questions. The JNDI changes 
in latest webrev looks good to me.

CI is also happy with the latest changes.

Best,
Aleksei

On 10/07/2020 21:37, Alexey Bakhtin wrote:

Hello Aleksei,

Thank you for review.
Please see my comments below.

Updated webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v14/

Regards
Alexey


On 10 Jul 2020, at 19:40, Aleks Efimov  wrote:

Hi Alexey,

Thank you for removing the dependency on the timeout property and adding tests 
for TLS handshake cases.

Please, find the comments about the latest webrev below:

Not quite sure why the CF is completed at two places. Probably that’s OK, but 
it would be good to know the reason :)

HandshakeCompletedListener is called in case of successful handshake only.
In case of async handshake failed we close the connection and complete CF 
exceptionally to release CF.get()


The ExecutionException could be unpacked instead of using it directly - and its 
cause used instead.

Thank you. Fixed in Connection.java and LdapCBPropertiesTest.java

'getTlsServerCertificate' should return null if 'isTlsConnection()' is false - 
rather than waiting forever.

We call isTlsConnection() in the LdapSasl before getTlsServerCertificate().
But your are right, we can double check it to prevent possible deadlock in the 
future, if code changed.
Fixed in Connection.java


java.naming/share/classes/module-info.java: could we try to improve the 
formatting of the possible values for the new system property - maybe format 
them as a list.

Done

Connection.java:995 - you could use diamond operator here

Updated

Formatting: Connection.java:1027 'catch (‘

Updated

Could we use the test/jdk/com/sun/jndi/ldap/lib/BaseLdapServer.java from the 
test library to implement dummy LDAPS server, I believe it could help to 
increase the test verbosity and reduce its code size.

Thank you for suggestion. Updated to use BaseLdapServer in the test


LdapCBPropertiesTest.java:122 - could use no param Hastable constructor

Fixed

I've also run your latest patch through our CI system and it showed no failures 
with your latest changes.

-Aleksei

On 09/07/2020 20:34, Alexey Bakhtin wrote:

Hello Sean, Daniel,

Thank you for review
I’ve added “com.sun.jndi.ldap.tls.cbtype” property into the module-info file
and updated synchronisation using CompletableFuture.
Also, I have added new test cases : successful and unsuccessful TLS handshake,
synchronous and asynchronous TLS handshake.

New webrev at : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v13

Connection property is removed from CSR :
https://bugs.openjdk.java.net/browse/JDK-8247311

Regards
Alexey


On 8 Jul 2020, at 17:41, Daniel Fuchs  wrote:

Thanks Sean, Alexey,

On 08/07/2020 13:25, Sean Mullan wrote:

Updated webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v12/

You will also need to update the CSR to remove the connection timeout property.
Also, you should document the "com.sun.jndi.ldap.tls.cbtype" property in the 
java.naming module-info file as was done for other properties in Daniel's recent RFR, 
once he has pushed it [1].

I have pushed the fix:
https://hg.openjdk.java.net/jdk/jdk/rev/ed375ae6c779

Alexey, you should now be able to document your new 
"com.sun.jndi.ldap.tls.cbtype"
property in that @implNote as well, and update your CSR
consequently.


* src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java
It looks like there is a possibility of deadlock if getTlsServerCertificate() 
is called before handshakeCompleted(). In that case the lock could be obtained 
first and the thread would end up holding the lock and waiting forever.

I also have a concern with the use of wait/notify in that code.
I would suggest prototyping using a CompletableFuture instead
(or can new certificates be exchanged later on the same
connection?)

CompletableFuture would allow you to relay and propagate any
exception raised in the callback handler as well, and would
avoid running into deadlocks.

best regards,

-- daniel