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

[Lance Andersen]

Hi Raffaello,

I think the changes look reasonable.

I have created a webrev, http://cr.openjdk.java.net/~lancea/8222187/webrev.00/ 
, for others to review 
as it is a bit easier than the patch.

I have also run the JCK tests in this area as well as mach5 tiers1-3 (which I 
believe Roger has also)

Best
Lance

> On Jul 15, 2020, at 5:44 PM, Raffaello Giulietti 
>  wrote:
> 
> 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) {
>  

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

[naoto . sato]

Hi Mark,

Thank you for your comments.

On 7/17/20 8:03 PM, Mark Davis ☕ wrote:
One option is to have a fast path that uses char functions, up to the 
point where you hit a high surrogate, then drop into the slower 
codepoint path. That saves time for the high-runner cases.


On the other hand, if the times are good enough, you might not need the 
complication.


The implementation is dealing with bare byte arrays. Only methods that 
it uses from Character class are toLowerCase(int) and toUpperCase(int) 
(sans surrogate check, which is needed at each iteration anyways), and 
their "char" equivalents are merely casting (char) to the int result. So 
it might not be so beneficial to differentiate char and int paths.


Having said that, I found that there was an unnecessary surrogate check 
(always checks high *and* low surrogate on each iteration), so I revised 
the fix (added line 380-383 in StringUTF16.java):


http://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.02/

Naoto



Mark
//


On Fri, Jul 17, 2020 at 4:39 PM > wrote:


Hi,

Based on the suggestions, I modified the fix as follows:

https://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.01/

Changes from the initial revision are:

- Shared the implementation between compareToCI() and regionMatchesCI()
- Enabled immediate short cut if two code points match.
- Created a simple JMH benchmark. Here is the scores before and after
the change:

before:
Benchmark                                Mode  Cnt   Score   Error 
Units
StringCompareToIgnoreCase.lower          avgt   25  53.764 ? 2.811 
ns/op
StringCompareToIgnoreCase.supLower       avgt   25  24.211 ? 1.135 
ns/op
StringCompareToIgnoreCase.supUpperLower  avgt   25  30.595 ? 1.344 
ns/op
StringCompareToIgnoreCase.upperLower     avgt   25  18.859 ? 1.499 
ns/op


after:
Benchmark                                Mode  Cnt   Score   Error 
Units
StringCompareToIgnoreCase.lower          avgt   25  58.354 ? 4.603 
ns/op
StringCompareToIgnoreCase.supLower       avgt   25  57.975 ? 5.672 
ns/op
StringCompareToIgnoreCase.supUpperLower  avgt   25  23.912 ? 0.965 
ns/op
StringCompareToIgnoreCase.upperLower     avgt   25  17.744 ? 0.272 
ns/op


Here, "sup" means all supplementary characters, BMP otherwise. "lower"
means each character requires upper->lower casemap. "upperLower" means
all characters are the same, except the last character which requires
casemap.

I think the result is reasonable, considering surrogates check are now
mandatory. I have tried Roger's suggestion to use Arrays.mismatch() but
it did not seem to benefit here. In fact, the performance degraded
partly because I implemented the short cut, and possibly for the
overhead of extra checks.

Naoto

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: [15] RFR(S) : 8249700 : java/io/File/GetXSpace.java should be added to exclude list, and not @ignore-d

[Alan Bateman]

On 19/07/2020 05:28, Igor Ignatyev wrote:

:
the patch also includes minimal changes to make the test runnable on macosx, 
which reveled that the test might fail on macos. 8249703 has been filed for 
that issue and the appropriate changes are done in ProblemList.txt.

webrev: http://cr.openjdk.java.net/~iignatyev//8249700/webrev.01/
JBS: https://bugs.openjdk.java.net/browse/JDK-8249700
testing:  java/io/File/GetXSpace.java on linux-x64 100 times, 100% passed; 
macos-x64 100 times, 71% passed

PS the test is still failing on windows albeit due to the reason other than in 
6501010; it seems that the test should be just updated to recognize cygwin's 
`df` output. I'm currently working on the fix and will send it out for review 
shortly.


This looks okay to me.

-Alan

Re: [PATCH] 8245694 : java.util.Properties.entrySet() does not override java.lang.Object methods

[Lisa Li]

Hi Julia，

Apologies for the delay. Please review the updated fix for JDK-8245694
.  Also, my name is on
the contributor list as: Yu Li - OpenJDK - yuli.


*BUG DESCRIPTION*:

JDK-8245694  :
java.util.Properties.entrySet()
does not override java.lang.Object methods since java 9.


*PATCH*:

# HG changeset patch
# User yuli
# Date 1595152648 -28800
#  Sun Jul 19 17:57:28 2020 +0800
# Node ID 4f359fdb3df3407cf8b31cb9ad153a99c52165c8
# Parent  72bf0aca309e326f35cdc85cbdeb3076e4d4e74d
8245694: java.util.Properties.entrySet() does not override Object methods
Summary: Add missing override methods
Contributed-by: Yu Li 

diff -r 72bf0aca309e -r 4f359fdb3df3
src/java.base/share/classes/java/util/Properties.java
--- a/src/java.base/share/classes/java/util/Properties.java Fri Jul 17
17:27:31 2020 -0700
+++ b/src/java.base/share/classes/java/util/Properties.java Sun Jul 19
17:57:28 2020 +0800
@@ -1405,6 +1405,21 @@
 }

 @Override
+public boolean equals(Object o) {
+return o == this || entrySet.equals(o);
+}
+
+@Override
+public int hashCode() {
+return entrySet.hashCode();
+}
+
+@Override
+public String toString() {
+return entrySet.toString();
+}
+
+@Override
 public boolean removeAll(Collection c) {
 return entrySet.removeAll(c);
 }
diff -r 72bf0aca309e -r 4f359fdb3df3
test/jdk/java/util/Properties/PropertiesEntrySetTest.java
--- /dev/null Thu Jan 01 00:00:00 1970 +
+++ b/test/jdk/java/util/Properties/PropertiesEntrySetTest.java Sun Jul 19
17:57:28 2020 +0800
@@ -0,0 +1,201 @@
+/*
+ * Copyright (c) 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License
version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug8245694
+ * @summarytests the entrySet() method of Properties class
+ * @author Yu Li
+ * @run testng PropertiesEntrySetTest
+ */
+
+import org.testng.annotations.Test;
+
+import java.util.Properties;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertThrows;
+import static org.testng.Assert.assertTrue;
+
+public class PropertiesEntrySetTest {
+
+@Test
+public void testEquals() {
+Properties a = new Properties();
+var aEntrySet = a.entrySet();
+assertFalse(aEntrySet.equals(null));
+assertTrue(aEntrySet.equals(aEntrySet));
+
+Properties b = new Properties();
+var bEntrySet = b.entrySet();
+assertTrue(bEntrySet.equals(aEntrySet));
+assertTrue(bEntrySet.hashCode() == aEntrySet.hashCode());
+
+a.setProperty("p1", "1");
+assertFalse(bEntrySet.equals(aEntrySet));
+assertFalse(bEntrySet.hashCode() == aEntrySet.hashCode());
+
+b.setProperty("p1", "1");
+assertTrue(aEntrySet.equals(bEntrySet));
+assertTrue(bEntrySet.hashCode() == aEntrySet.hashCode());
+
+Properties c = new Properties();
+c.setProperty("p1", "2");
+var cEntrySet = c.entrySet();
+assertFalse(cEntrySet.equals(bEntrySet));
+assertFalse(bEntrySet.hashCode() == cEntrySet.hashCode());
+assertFalse(cEntrySet.equals(aEntrySet));
+assertFalse(aEntrySet.hashCode() == cEntrySet.hashCode());
+
+a.setProperty("p2", "2");
+Properties d = new Properties();
+d.setProperty("p2", "2");
+d.setProperty("p1", "1");
+var dEntrySet = d.entrySet();
+assertTrue(dEntrySet.equals(aEntrySet));
+assertTrue(aEntrySet.hashCode() == dEntrySet.hashCode());
+
+a.remove("p1");
+assertFalse(aEntrySet.equals(dEntrySet));
+assertFalse(aEntrySet.hashCode() == dEntrySet.hashCode());
+
+d.remove("p1", "1");
+assertTrue(dEntrySet.equals(aEntrySet));
+assertTrue(aEntrySet.hashCode() == dEntrySet.hashCode());
+
+