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

[naoto . sato]

Hi Joe,

On 7/20/20 7:14 PM, Joe Wang wrote:

Hi Naoto,

"Unless it showed 100% slower", wow, your tolerance is quite high :-). 
On the other hand, I do agree it's unlikely to show in specJVM (that's a 
speculation though).


I am not saying 100% slowing is permissible :-) That's an example of 
definite no.




The short-cut worked well. There's maybe a further optimization we could 
do to rid us of the performance concern (or the need to run additional 
performance tests). Consider the cases where strings in comparison don't 
contain any sup characters, if we make the toLower/UpperCase() block a 
method and call it before and after the surrogate-check block, the 
routine would be effectively the same as prior to the introduction of 
the surrogate-check block, and regular comparisons would suffer the 
surrogate-check only once (the last check). For strings that do contain 
sup characters then, the toLower/UpperCase() method would have been 
called twice, but then we don't care about the performance in that 
situation. You may call the existing codePointAt method too when an 
extra getChar and performance is not a concern (but that's your call.


Can you please elaborate this more? What's "the last check" here?

Naoto



Regards,
Joe

On 7/20/20 3:20 PM, naoto.s...@oracle.com wrote:

Small correction in the updated part:

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

Naoto

On 7/20/20 2:39 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Thank you for your comments.

On 7/20/20 11:20 AM, Joe Wang wrote:

Hi Naoto,

StringUTF16: line 384 - 388 seem unnecessary since you'd only get 
there if 389:isHighSurrogate is not true. 


Good point.

But more importantly, StringUTF16
has existing method "codePointAt" you may want to consider instead 
of adding a new method.


If we call codePointAt/Before, it would call an extra getChar(). 
Since we know one codepoint as an input, I would avoid the extra calls.




Comparing to the base benchmark:
StringCompareToIgnoreCase.lower  8.5%
StringCompareToIgnoreCase.supLower  139%
StringCompareToIgnoreCase.supUpperLower  -21.8%
StringCompareToIgnoreCase.upperLower avgt   -5.9%


"lower" was 8.5% slower, if such test exists in the specJVM, it 
would be considered a regression. I would suggest you run the 
specJVM. I agree with you on surrogate check being a requirement, 
thus supLower being 139% slower is ok since it won't otherwise be 
correct anyways.


Yes, it would be a regression if SPECjvm produces 8+% degradation, 
but the test suite is aimed at the entire application performance. 
But for this one, it is a micro benchmark for relatively rarely 
issued methods (I would think normal cases fall into Latin1 
equivalents), I would consider it is acceptable.


But after introducing additional operations supUpperLower and 
upperLower ran faster? That may indicate irregularity in the tests. 
Maybe we should consider running tests with short, long and very 
long sample strings to see if we can reduce the noise level and also 
see how it fares for a longer string. I assume the machine you're 
running the test on was isolated.


This result pretty much depends on the data it is testing for. As I 
wrote in the previous email, (sup)UpperLower tests use the data that 
are almost identical, but one last character is case insensitively 
equal. So in these cases, the new short cut works really well and not 
call toLower/UpperCase() at all for most of the characters. Thus the 
new results are faster. Again the test result is very dependent on 
the input data, Unless the result showed 100% slower or something 
(except supLower case), I think it is OK.


Anyways, here is the updated webrev addressing your first suggestion:

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

Naoto



Regards,
Joe

On 7/19/2020 11:05 AM, naoto.s...@oracle.com wrote:

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 

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

[Joe Wang]

Hi Naoto,

"Unless it showed 100% slower", wow, your tolerance is quite high :-). 
On the other hand, I do agree it's unlikely to show in specJVM (that's a 
speculation though).


The short-cut worked well. There's maybe a further optimization we could 
do to rid us of the performance concern (or the need to run additional 
performance tests). Consider the cases where strings in comparison don't 
contain any sup characters, if we make the toLower/UpperCase() block a 
method and call it before and after the surrogate-check block, the 
routine would be effectively the same as prior to the introduction of 
the surrogate-check block, and regular comparisons would suffer the 
surrogate-check only once (the last check). For strings that do contain 
sup characters then, the toLower/UpperCase() method would have been 
called twice, but then we don't care about the performance in that 
situation. You may call the existing codePointAt method too when an 
extra getChar and performance is not a concern (but that's your call.


Regards,
Joe

On 7/20/20 3:20 PM, naoto.s...@oracle.com wrote:

Small correction in the updated part:

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

Naoto

On 7/20/20 2:39 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Thank you for your comments.

On 7/20/20 11:20 AM, Joe Wang wrote:

Hi Naoto,

StringUTF16: line 384 - 388 seem unnecessary since you'd only get 
there if 389:isHighSurrogate is not true. 


Good point.

But more importantly, StringUTF16
has existing method "codePointAt" you may want to consider instead 
of adding a new method.


If we call codePointAt/Before, it would call an extra getChar(). 
Since we know one codepoint as an input, I would avoid the extra calls.




Comparing to the base benchmark:
StringCompareToIgnoreCase.lower  8.5%
StringCompareToIgnoreCase.supLower  139%
StringCompareToIgnoreCase.supUpperLower  -21.8%
StringCompareToIgnoreCase.upperLower avgt   -5.9%


"lower" was 8.5% slower, if such test exists in the specJVM, it 
would be considered a regression. I would suggest you run the 
specJVM. I agree with you on surrogate check being a requirement, 
thus supLower being 139% slower is ok since it won't otherwise be 
correct anyways.


Yes, it would be a regression if SPECjvm produces 8+% degradation, 
but the test suite is aimed at the entire application performance. 
But for this one, it is a micro benchmark for relatively rarely 
issued methods (I would think normal cases fall into Latin1 
equivalents), I would consider it is acceptable.


But after introducing additional operations supUpperLower and 
upperLower ran faster? That may indicate irregularity in the tests. 
Maybe we should consider running tests with short, long and very 
long sample strings to see if we can reduce the noise level and also 
see how it fares for a longer string. I assume the machine you're 
running the test on was isolated.


This result pretty much depends on the data it is testing for. As I 
wrote in the previous email, (sup)UpperLower tests use the data that 
are almost identical, but one last character is case insensitively 
equal. So in these cases, the new short cut works really well and not 
call toLower/UpperCase() at all for most of the characters. Thus the 
new results are faster. Again the test result is very dependent on 
the input data, Unless the result showed 100% slower or something 
(except supLower case), I think it is OK.


Anyways, here is the updated webrev addressing your first suggestion:

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

Naoto



Regards,
Joe

On 7/19/2020 11:05 AM, naoto.s...@oracle.com wrote:

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 

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

[naoto . sato]

Small correction in the updated part:

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

Naoto

On 7/20/20 2:39 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Thank you for your comments.

On 7/20/20 11:20 AM, Joe Wang wrote:

Hi Naoto,

StringUTF16: line 384 - 388 seem unnecessary since you'd only get 
there if 389:isHighSurrogate is not true. 


Good point.

But more importantly, StringUTF16
has existing method "codePointAt" you may want to consider instead of 
adding a new method.


If we call codePointAt/Before, it would call an extra getChar(). Since 
we know one codepoint as an input, I would avoid the extra calls.




Comparing to the base benchmark:
StringCompareToIgnoreCase.lower  8.5%
StringCompareToIgnoreCase.supLower  139%
StringCompareToIgnoreCase.supUpperLower  -21.8%
StringCompareToIgnoreCase.upperLower avgt   -5.9%


"lower" was 8.5% slower, if such test exists in the specJVM, it would 
be considered a regression. I would suggest you run the specJVM. I 
agree with you on surrogate check being a requirement, thus supLower 
being 139% slower is ok since it won't otherwise be correct anyways.


Yes, it would be a regression if SPECjvm produces 8+% degradation, but 
the test suite is aimed at the entire application performance. But for 
this one, it is a micro benchmark for relatively rarely issued methods 
(I would think normal cases fall into Latin1 equivalents), I would 
consider it is acceptable.


But after introducing additional operations supUpperLower and 
upperLower ran faster? That may indicate irregularity in the tests. 
Maybe we should consider running tests with short, long and very long 
sample strings to see if we can reduce the noise level and also see 
how it fares for a longer string. I assume the machine you're running 
the test on was isolated.


This result pretty much depends on the data it is testing for. As I 
wrote in the previous email, (sup)UpperLower tests use the data that are 
almost identical, but one last character is case insensitively equal. So 
in these cases, the new short cut works really well and not call 
toLower/UpperCase() at all for most of the characters. Thus the new 
results are faster. Again the test result is very dependent on the input 
data, Unless the result showed 100% slower or something (except supLower 
case), I think it is OK.


Anyways, here is the updated webrev addressing your first suggestion:

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

Naoto



Regards,
Joe

On 7/19/2020 11:05 AM, naoto.s...@oracle.com wrote:

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 

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

[naoto . sato]

Hi Joe,

Thank you for your comments.

On 7/20/20 11:20 AM, Joe Wang wrote:

Hi Naoto,

StringUTF16: line 384 - 388 seem unnecessary since you'd only get there 
if 389:isHighSurrogate is not true. 


Good point.

But more importantly, StringUTF16
has existing method "codePointAt" you may want to consider instead of 
adding a new method.


If we call codePointAt/Before, it would call an extra getChar(). Since 
we know one codepoint as an input, I would avoid the extra calls.




Comparing to the base benchmark:
StringCompareToIgnoreCase.lower  8.5%
StringCompareToIgnoreCase.supLower  139%
StringCompareToIgnoreCase.supUpperLower  -21.8%
StringCompareToIgnoreCase.upperLower avgt   -5.9%


"lower" was 8.5% slower, if such test exists in the specJVM, it would be 
considered a regression. I would suggest you run the specJVM. I agree 
with you on surrogate check being a requirement, thus supLower being 
139% slower is ok since it won't otherwise be correct anyways.


Yes, it would be a regression if SPECjvm produces 8+% degradation, but 
the test suite is aimed at the entire application performance. But for 
this one, it is a micro benchmark for relatively rarely issued methods 
(I would think normal cases fall into Latin1 equivalents), I would 
consider it is acceptable.


But after 
introducing additional operations supUpperLower and upperLower ran 
faster? That may indicate irregularity in the tests. Maybe we should 
consider running tests with short, long and very long sample strings to 
see if we can reduce the noise level and also see how it fares for a 
longer string. I assume the machine you're running the test on was 
isolated.


This result pretty much depends on the data it is testing for. As I 
wrote in the previous email, (sup)UpperLower tests use the data that are 
almost identical, but one last character is case insensitively equal. So 
in these cases, the new short cut works really well and not call 
toLower/UpperCase() at all for most of the characters. Thus the new 
results are faster. Again the test result is very dependent on the input 
data, Unless the result showed 100% slower or something (except supLower 
case), I think it is OK.


Anyways, here is the updated webrev addressing your first suggestion:

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

Naoto



Regards,
Joe

On 7/19/2020 11:05 AM, naoto.s...@oracle.com wrote:

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 

Re: 8248248: [macos] EmptyFolderPackageTest.java fails EmptyFolderPackageTest-dmg-setup.scpt exited with 134 code

[Alexey Semenyuk]

Looks good.

Minor suggestion: introduce a constant for infinite timeout and use it 
instead of "-1".


- Alexey

On 7/20/2020 4:43 PM, alexander.matv...@oracle.com wrote:

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

It is not clear why script was hanging for more than 7 minutes which 
caused test to timeout. Fixed by limiting script execution time to 3 
minutes. Also, EmptyFolderPackageTest was removed from ProblemList.


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

Thanks,
Alexander

8248248: [macos] EmptyFolderPackageTest.java fails EmptyFolderPackageTest-dmg-setup.scpt exited with 134 code

[alexander . matveev]

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

It is not clear why script was hanging for more than 7 minutes which 
caused test to timeout. Fixed by limiting script execution time to 3 
minutes. Also, EmptyFolderPackageTest was removed from ProblemList.


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

Thanks,
Alexander

Re: [15] RFR(T) : 8249697 : java/lang/invoke/RicochetTest.java should use @requires instead of @ignore

[Mandy Chung]

webrev.03 looks good.

Mandy

On 7/20/20 12:22 PM, Igor Ignatyev wrote:

Hi Mandy,

you are right, it's better to have just one @run, and as I don't think 
that 7197210 changes '-XX:-VerifyDependencies' nor '/timeout=3600' are 
needed anymore, I suggest to restore the test to its original version 
w/  `@run junit/othervm -DRicochetTest.MAX_ARITY=255 
test.java.lang.invoke.RicochetTest`, so the patch 
(http://cr.openjdk.java.net/~iignatyev//8249697/webrev.03) would be just:



-/* @test
+/*
+ * @test
  * @summary unit tests for recursive method handles
- * @run junit/othervm/timeout=3600 -XX:+IgnoreUnrecognizedVMOptions 
-XX:-VerifyDependencies -DRicochetTest.MAX_ARITY=10 
test.java.lang.invoke.RicochetTest

- */
-/*
- * @ignore The following test creates an unreasonable number of 
adapters in -Xcomp mode (7049122)
  * @run junit/othervm -DRicochetTest.MAX_ARITY=255 
test.java.lang.invoke.RicochetTest

  */


and then the bug's summary would be smth like 'remove temporary fixes 
from java/lang/invoke/RicochetTest.java' .


sure there is no reason for it to be pushed into 15, I've retargeted 
to 16.


-- Igor

On Jul 20, 2020, at 11:57 AM, Mandy Chung > wrote:


Hi Igor,

OK.  Should this revert the change by 7049122 then? i.e. simply 
change -DRicochetTest.MAX_ARITY=10 to 255


Your proposed patch adds a new @run instead of modifying the existing 
@run command:


  * @run junit/othervm/timeout=3600 -XX:+IgnoreUnrecognizedVMOptions 
-XX:-VerifyDependencies -DRicochetTest.MAX_ARITY=10 
test.java.lang.invoke.RicochetTest


I looked at the history and this @run was modified by JDK-7197210 
that adds -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies 
options and reduce MAX_ARITY from 50 to 10.


This issue is not critical to target for 15.  It may worth 
considering target this test fix for 16.  Just a suggestion.


Mandy

On 7/20/20 10:13 AM, Igor Ignatyev wrote:

Hi Mandy,

that's actually the opposite, the 2nd subtest is run only in modes 
other than Xcomp, as w/ Xcomp the test creates lots of adapters and 
used to lead to JVM failure as described in 7049122. I tried to 
reproduce this failure, but in vain,..  after a bit more historical 
digging, I realized that the underlying problem was 7009641, which 
has been fixed in hs25/jdk8. so I've changed the fix for 8249697 to 
simply return run w/ '-DRicochetTest.MAX_ARITY=255': 
http://cr.openjdk.java.net/~iignatyev//8249697/webrev.02


I've verified that the test passes w/ Xcomp and
 - -XX:+TieredCompilation (c1 + c2);
 - -XX:-TieredCompilation (c2-only);
 - -XX:+NeverActAsServerClassMachine (emulated-client, c1-only)

the test was run 100 times on {linux,windows,macos}-x64 w/ 0 failures.
Thanks,
-- Igor

On Jul 18, 2020, at 9:32 PM, Mandy Chung > wrote:




On 7/17/20 8:54 PM, Igor Ignatyev wrote:

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



I suggest to change this:
  32  * @comment The following test creates an unreasonable number 
of adapters in -Xcomp mode (7049122)


To:

   @bug 8249697
   @summary verify very high number of adapters in -Xcomp mode

Otherwise, looks fine.

Mandy

Hi all,

could you please review this small and trivial patch for 
java/lang/invoke/RicochetTest.java test?
from JBS:

a run of java/lang/invoke/RicochetTest.java w/ MAX_ARITY=255 was removed from 
all configurations by JDK-7049122, yet the problem manifests itself only w/ 
Xcomp. as now we have @requires to filter out tests from certain 
configurations, the test can be updated to run MAX_ARITY=255 in all configs but 
Xcomp.

the patch splits the test into two subtests, each one w/ one @run, and use 
@requires to exclude one w/ MAX_ARITY=255 from execution if Xcomp flag is used.

JBS:https://bugs.openjdk.java.net/browse/JDK-8249697
webrev:http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/
testing: java/lang/invoke/RicochetTest.java on {linux,windows,macos}-x64 w/ and 
w/o -Xcomp; Xcomp runs, as expected, had only 1 test run

Thanks,
-- Igor

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

Re: 8249774: Add java/foreign/TestMismatch.java to ProblemList.txt

[Lance Andersen]

+1

> On Jul 20, 2020, at 3:25 PM, Daniel Fuchs  wrote:
> 
> Hi,
> 
> Please find below a fix for:
> 
> 8249774: Add java/foreign/TestMismatch.java to ProblemList.txt
> https://bugs.openjdk.java.net/browse/JDK-8249774
> 
> The test has failed twice in timeout on macOS in the tier1 CI.
> Requesting to put it into a problem list until a fix is proposed.
> 
> 
> best regards,
> 
> -- daniel
> 
> patch:
> 
> 
> 
> diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
> --- a/test/jdk/ProblemList.txt
> +++ b/test/jdk/ProblemList.txt
> @@ -559,6 +559,12 @@
> 
> 
> 
> +# jdk_foreign
> +
> +java/foreign/TestMismatch.java 8249684 macosx-all
> +
> +
> +
> # jdk_lang
> 
> java/lang/StringCoding/CheckEncodings.sh 7008363 generic-all


Best
Lance
--




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

8249774: Add java/foreign/TestMismatch.java to ProblemList.txt

[Daniel Fuchs]

Hi,

Please find below a fix for:

8249774: Add java/foreign/TestMismatch.java to ProblemList.txt
https://bugs.openjdk.java.net/browse/JDK-8249774

The test has failed twice in timeout on macOS in the tier1 CI.
Requesting to put it into a problem list until a fix is proposed.


best regards,

-- daniel

patch:



diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt
+++ b/test/jdk/ProblemList.txt
@@ -559,6 +559,12 @@




+# jdk_foreign
+
+java/foreign/TestMismatch.java 8249684 macosx-all
+
+
+
 # jdk_lang

 java/lang/StringCoding/CheckEncodings.sh 
7008363 generic-all

Re: [15] RFR(T) : 8249697 : java/lang/invoke/RicochetTest.java should use @requires instead of @ignore

[Igor Ignatyev]

Hi Mandy,

you are right, it's better to have just one @run, and as I don't think that 
7197210 changes '-XX:-VerifyDependencies' nor '/timeout=3600' are needed 
anymore, I suggest to restore the test to its original version w/  `@run 
junit/othervm -DRicochetTest.MAX_ARITY=255 test.java.lang.invoke.RicochetTest`, 
so the patch (http://cr.openjdk.java.net/~iignatyev//8249697/webrev.03) would 
be just:

> -/* @test
> +/*
> + * @test
>   * @summary unit tests for recursive method handles
> - * @run junit/othervm/timeout=3600 -XX:+IgnoreUnrecognizedVMOptions 
> -XX:-VerifyDependencies -DRicochetTest.MAX_ARITY=10 
> test.java.lang.invoke.RicochetTest
> - */
> -/*
> - * @ignore The following test creates an unreasonable number of adapters in 
> -Xcomp mode (7049122)
>   * @run junit/othervm -DRicochetTest.MAX_ARITY=255 
> test.java.lang.invoke.RicochetTest
>   */


and then the bug's summary would be smth like 'remove temporary fixes from 
java/lang/invoke/RicochetTest.java' .

sure there is no reason for it to be pushed into 15, I've retargeted to 16.

-- Igor

> On Jul 20, 2020, at 11:57 AM, Mandy Chung  wrote:
> 
> Hi Igor,
> 
> OK.  Should this revert the change by 7049122 then? i.e. simply change 
> -DRicochetTest.MAX_ARITY=10 to 255
> 
> Your proposed patch adds a new @run instead of modifying the existing @run 
> command:
> 
>   * @run junit/othervm/timeout=3600 -XX:+IgnoreUnrecognizedVMOptions 
> -XX:-VerifyDependencies -DRicochetTest.MAX_ARITY=10 
> test.java.lang.invoke.RicochetTest
> 
> I looked at the history and this @run was modified by JDK-7197210 that adds 
> -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies options and reduce 
> MAX_ARITY from 50 to 10.
> 
> This issue is not critical to target for 15.  It may worth considering target 
> this test fix for 16.  Just a suggestion.
> 
> Mandy
> 
> On 7/20/20 10:13 AM, Igor Ignatyev wrote:
>> Hi Mandy,
>> 
>> that's actually the opposite, the 2nd subtest is run only in modes other 
>> than Xcomp, as w/ Xcomp the test creates lots of adapters and used to lead 
>> to JVM failure as described in 7049122. I tried to reproduce this failure, 
>> but in vain,..  after a bit more historical digging, I realized that the 
>> underlying problem was 7009641, which has been fixed in hs25/jdk8. so I've 
>> changed the fix for 8249697 to simply return run w/ 
>> '-DRicochetTest.MAX_ARITY=255': 
>> http://cr.openjdk.java.net/~iignatyev//8249697/webrev.02 
>> 
>> 
>> I've verified that the test passes w/ Xcomp and 
>>  - -XX:+TieredCompilation (c1 + c2);
>>  - -XX:-TieredCompilation (c2-only);
>>  - -XX:+NeverActAsServerClassMachine (emulated-client, c1-only)
>> 
>> the test was run 100 times on {linux,windows,macos}-x64 w/ 0 failures.
>>  
>> Thanks,
>> -- Igor
>> 
>>> On Jul 18, 2020, at 9:32 PM, Mandy Chung >> > wrote:
>>> 
>>> 
>>> 
>>> On 7/17/20 8:54 PM, Igor Ignatyev wrote:
 http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/ 
 
 
>>> 
>>> I suggest to change this:
>>>   32  * @comment The following test creates an unreasonable number of 
>>> adapters in -Xcomp mode (7049122)
>>> 
>>> To:
>>> 
>>>@bug 8249697
>>>@summary verify very high number of adapters in -Xcomp mode
>>> 
>>> Otherwise, looks fine.
>>> 
>>> Mandy
 Hi all,
 
 could you please review this small and trivial patch for 
 java/lang/invoke/RicochetTest.java test?
 from JBS:
> a run of java/lang/invoke/RicochetTest.java w/ MAX_ARITY=255 was removed 
> from all configurations by JDK-7049122, yet the problem manifests itself 
> only w/ Xcomp. as now we have @requires to filter out tests from certain 
> configurations, the test can be updated to run MAX_ARITY=255 in all 
> configs but Xcomp.
 the patch splits the test into two subtests, each one w/ one @run, and use 
 @requires to exclude one w/ MAX_ARITY=255 from execution if Xcomp flag is 
 used.
 
 JBS: https://bugs.openjdk.java.net/browse/JDK-8249697 
 
 webrev: http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/ 
 
 testing: java/lang/invoke/RicochetTest.java on {linux,windows,macos}-x64 
 w/ and w/o -Xcomp; Xcomp runs, as expected, had only 1 test run
 
 Thanks,
 -- Igor
 
 JDK-7049122 : https://bugs.openjdk.java.net/browse/JDK-7049122 
 
>> 
> 

Re: [15] RFR(T) : 8249697 : java/lang/invoke/RicochetTest.java should use @requires instead of @ignore

[Mandy Chung]

Hi Igor,

OK.  Should this revert the change by 7049122 then? i.e. simply change 
-DRicochetTest.MAX_ARITY=10 to 255


Your proposed patch adds a new @run instead of modifying the existing 
@run command:


  * @run junit/othervm/timeout=3600 -XX:+IgnoreUnrecognizedVMOptions 
-XX:-VerifyDependencies -DRicochetTest.MAX_ARITY=10 
test.java.lang.invoke.RicochetTest


I looked at the history and this @run was modified by JDK-7197210 that 
adds -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies options 
and reduce MAX_ARITY from 50 to 10.


This issue is not critical to target for 15.  It may worth considering 
target this test fix for 16.  Just a suggestion.


Mandy

On 7/20/20 10:13 AM, Igor Ignatyev wrote:

Hi Mandy,

that's actually the opposite, the 2nd subtest is run only in modes 
other than Xcomp, as w/ Xcomp the test creates lots of adapters and 
used to lead to JVM failure as described in 7049122. I tried to 
reproduce this failure, but in vain,..  after a bit more historical 
digging, I realized that the underlying problem was 7009641, which has 
been fixed in hs25/jdk8. so I've changed the fix for 8249697 to simply 
return run w/ '-DRicochetTest.MAX_ARITY=255': 
http://cr.openjdk.java.net/~iignatyev//8249697/webrev.02


I've verified that the test passes w/ Xcomp and
 - -XX:+TieredCompilation (c1 + c2);
 - -XX:-TieredCompilation (c2-only);
 - -XX:+NeverActAsServerClassMachine (emulated-client, c1-only)

the test was run 100 times on {linux,windows,macos}-x64 w/ 0 failures.
Thanks,
-- Igor

On Jul 18, 2020, at 9:32 PM, Mandy Chung > wrote:




On 7/17/20 8:54 PM, Igor Ignatyev wrote:

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



I suggest to change this:
  32  * @comment The following test creates an unreasonable number of 
adapters in -Xcomp mode (7049122)


To:

   @bug 8249697
   @summary verify very high number of adapters in -Xcomp mode

Otherwise, looks fine.

Mandy

Hi all,

could you please review this small and trivial patch for 
java/lang/invoke/RicochetTest.java test?
from JBS:

a run of java/lang/invoke/RicochetTest.java w/ MAX_ARITY=255 was removed from 
all configurations by JDK-7049122, yet the problem manifests itself only w/ 
Xcomp. as now we have @requires to filter out tests from certain 
configurations, the test can be updated to run MAX_ARITY=255 in all configs but 
Xcomp.

the patch splits the test into two subtests, each one w/ one @run, and use 
@requires to exclude one w/ MAX_ARITY=255 from execution if Xcomp flag is used.

JBS:https://bugs.openjdk.java.net/browse/JDK-8249697
webrev:http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/
testing: java/lang/invoke/RicochetTest.java on {linux,windows,macos}-x64 w/ and 
w/o -Xcomp; Xcomp runs, as expected, had only 1 test run

Thanks,
-- Igor

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

[15] RFR(S) : 6501010 : test/java/io/File/GetXSpace.java fails on Windows

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//6501010/webrev.00
> 98 lines changed: 18 ins; 31 del; 49 mod;

Hi all,

could you please review this small fix to make java/io/File/GetXSpace.java work 
again on windows?

the test has been updated to work under cygwin, which includes the following 
changes:
 - GetXSpace.sh accepts CYGWIN_* as known OS and sets windows path to TMP var
 - uses the same regexp to parse df on windows as on other platforms
 - but uses 'FileSystem' field as Space's name, as opposed to 'MountedOn' on 
*nix, as on cygwin, MountedOn is cygwin path, while FileSystem is corresponding 
windows path, and the test expect Space's name to be appropriate path

the patch also includes some small faceliftings:
 - printing out a bit more information to facilitate failure analyses, e.g. df 
output, exception stack traces
 - usage of try-w/-resources
 - remove of code duplication
 - usage of generics

webrev: http://cr.openjdk.java.net/~iignatyev//6501010/webrev.00
testing: java/io/File/GetXSpace.java 100 times on {linux,windows}-x64
JBS: https://bugs.openjdk.java.net/browse/JDK-6501010

Thanks,
-- Igor

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

[Joe Wang]

Hi Naoto,

StringUTF16: line 384 - 388 seem unnecessary since you'd only get there 
if 389:isHighSurrogate is not true. But more importantly, StringUTF16 
has existing method "codePointAt" you may want to consider instead of 
adding a new method.


Comparing to the base benchmark:
StringCompareToIgnoreCase.lower  8.5%
StringCompareToIgnoreCase.supLower  139%
StringCompareToIgnoreCase.supUpperLower  -21.8%
StringCompareToIgnoreCase.upperLower avgt   -5.9%


"lower" was 8.5% slower, if such test exists in the specJVM, it would be 
considered a regression. I would suggest you run the specJVM. I agree 
with you on surrogate check being a requirement, thus supLower being 
139% slower is ok since it won't otherwise be correct anyways. But after 
introducing additional operations supUpperLower and upperLower ran 
faster? That may indicate irregularity in the tests. Maybe we should 
consider running tests with short, long and very long sample strings to 
see if we can reduce the noise level and also see how it fares for a 
longer string. I assume the machine you're running the test on was isolated.


Regards,
Joe

On 7/19/2020 11:05 AM, naoto.s...@oracle.com wrote:

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, 

Standardizing JEP 343 with draft of new JEP - JDK-8247768: Packaging Tool (Standard)

[Andy Herrick]

The jpackage tool described in JEP 343 
(https://bugs.openjdk.java.net/browse/JDK-8200758) 
.


This was delivered as an incubating tool in JDK 14, and again in JDK 15 
to allow more time for feedback.


At this time we have received and incorporated such feedback, would like 
to standardize this tool.


To that end we intend to submit: JDK-8247768: Packaging Tool (Standard) 
(https://bugs.openjdk.java.net/browse/JDK-8247768).


Please review and send feedback to core-libs-dev@openjdk.java.net.

/Andy

Re: [15] RFR(T) : 8249697 : java/lang/invoke/RicochetTest.java should use @requires instead of @ignore

[Igor Ignatyev]

Hi Mandy,

that's actually the opposite, the 2nd subtest is run only in modes other than 
Xcomp, as w/ Xcomp the test creates lots of adapters and used to lead to JVM 
failure as described in 7049122. I tried to reproduce this failure, but in 
vain,..  after a bit more historical digging, I realized that the underlying 
problem was 7009641, which has been fixed in hs25/jdk8. so I've changed the fix 
for 8249697 to simply return run w/ '-DRicochetTest.MAX_ARITY=255': 
http://cr.openjdk.java.net/~iignatyev//8249697/webrev.02

I've verified that the test passes w/ Xcomp and 
 - -XX:+TieredCompilation (c1 + c2);
 - -XX:-TieredCompilation (c2-only);
 - -XX:+NeverActAsServerClassMachine (emulated-client, c1-only)

the test was run 100 times on {linux,windows,macos}-x64 w/ 0 failures.
 
Thanks,
-- Igor

> On Jul 18, 2020, at 9:32 PM, Mandy Chung  wrote:
> 
> 
> 
> On 7/17/20 8:54 PM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/ 
>> 
>> 
> 
> I suggest to change this:
>   32  * @comment The following test creates an unreasonable number of 
> adapters in -Xcomp mode (7049122)
> 
> To:
> 
>@bug 8249697
>@summary verify very high number of adapters in -Xcomp mode
> 
> Otherwise, looks fine.
> 
> Mandy
>> Hi all,
>> 
>> could you please review this small and trivial patch for 
>> java/lang/invoke/RicochetTest.java test?
>> from JBS:
>>> a run of java/lang/invoke/RicochetTest.java w/ MAX_ARITY=255 was removed 
>>> from all configurations by JDK-7049122, yet the problem manifests itself 
>>> only w/ Xcomp. as now we have @requires to filter out tests from certain 
>>> configurations, the test can be updated to run MAX_ARITY=255 in all configs 
>>> but Xcomp.
>> the patch splits the test into two subtests, each one w/ one @run, and use 
>> @requires to exclude one w/ MAX_ARITY=255 from execution if Xcomp flag is 
>> used.
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8249697 
>> 
>> webrev: http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/ 
>> 
>> testing: java/lang/invoke/RicochetTest.java on {linux,windows,macos}-x64 w/ 
>> and w/o -Xcomp; Xcomp runs, as expected, had only 1 test run
>> 
>> Thanks,
>> -- Igor
>> 
>> JDK-7049122 : https://bugs.openjdk.java.net/browse/JDK-7049122 
>> 

Re: [15] 8235792: LineNumberReader.getLineNumber() behavior is inconsistent with respect to EOF

[Brian Burkhalter]

Push to JDK 16 that is.

> On Jul 20, 2020, at 10:04 AM, Brian Burkhalter  
> wrote:
> 
> The CSR [2] has been approved so unless there are further comments I’ll push 
> this change [1] this week.
> 
> Thanks,
> 
> Brian
> 
>> On Mar 19, 2020, at 7:43 AM, Brian Burkhalter  
>> wrote:
>> 
>> Another webrev [1] which is adjusted from the previous one per the comments 
>> on the CSR [2] is available for review. The only change is to the 
>> class-level specification:
>> 
>> --- a/src/java.base/share/classes/java/io/LineNumberReader.java
>> +++ b/src/java.base/share/classes/java/io/LineNumberReader.java
>> @@ -41,7 +41,8 @@
>>  *
>>  *  A line is considered to be terminated by any one of a
>>  * line feed ('\n'), a carriage return ('\r'), or a carriage return followed
>> - * immediately by a linefeed.
>> + * immediately by a linefeed, or any of the previous terminators followed by
>> + * end of stream, or end of stream not preceded by another terminator.
>>  *
>> 
>> Thanks,
>> 
>> Brian
>> 
>>> On Mar 13, 2020, at 10:28 AM, Brian Burkhalter 
>>>  wrote:
>>> 
>>> An updated webrev is at [1] and a CSR has been filed [2].
> 
> [1] http://cr.openjdk.java.net/~bpb/8235792/webrev.02/
> [2] https://bugs.openjdk.java.net/browse/JDK-8241020
> 

Re: [15] 8235792: LineNumberReader.getLineNumber() behavior is inconsistent with respect to EOF

[Brian Burkhalter]

The CSR [2] has been approved so unless there are further comments I’ll push 
this change [1] this week.

Thanks,

Brian

> On Mar 19, 2020, at 7:43 AM, Brian Burkhalter  
> wrote:
> 
> Another webrev [1] which is adjusted from the previous one per the comments 
> on the CSR [2] is available for review. The only change is to the class-level 
> specification:
> 
> --- a/src/java.base/share/classes/java/io/LineNumberReader.java
> +++ b/src/java.base/share/classes/java/io/LineNumberReader.java
> @@ -41,7 +41,8 @@
>   *
>   *  A line is considered to be terminated by any one of a
>   * line feed ('\n'), a carriage return ('\r'), or a carriage return followed
> - * immediately by a linefeed.
> + * immediately by a linefeed, or any of the previous terminators followed by
> + * end of stream, or end of stream not preceded by another terminator.
>   *
> 
> Thanks,
> 
> Brian
> 
>> On Mar 13, 2020, at 10:28 AM, Brian Burkhalter  
>> wrote:
>> 
>> An updated webrev is at [1] and a CSR has been filed [2].

[1] http://cr.openjdk.java.net/~bpb/8235792/webrev.02/
[2] https://bugs.openjdk.java.net/browse/JDK-8241020

Re: [15] RFR(S) : 8249700 : java/io/File/GetXSpace.java should be added to exclude list, and not @ignore-d

[Igor Ignatyev]

Hi Alan,

thanks for the review, pushed to jdk15.

-- Igor

> On Jul 19, 2020, at 8:18 AM, Alan Bateman  wrote:
> 
> 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: [15] RFR(T) : 8249698 : java/lang/invoke/LFCaching/LFGarbageCollectedTest.java should be ProblemList-ed and not @ignored

[Igor Ignatyev]

Mandy, Vladimir,

thanks for your reviews, pushed to jdk15.

-- Igor

> On Jul 18, 2020, at 9:33 PM, Mandy Chung  wrote:
> 
> +1
> 
> Mandy
> 
> On 7/17/20 8:57 PM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8249698/webrev.00 
>> 
>>> 3 lines changed: 1 ins; 1 del; 1 mod;
>> 
>> Hi all,
>> 
>> could you please review this trivial patch which removes @ignore from 
>> LFGarbageCollectedTest and adds it into problem-list instead?
>> 
>> from  8249698:
>>> java/lang/invoke/LFCaching/LFGarbageCollectedTest.java is excluded from 
>>> execution due to JDK-8078602. although the test might indeed fail due to 
>>> JDK-8078602, it still can be useful and isn't harmful to run, therefore 
>>> this test should be put in ProblemList.txt and @ignore is to be removed.
>> from main issue(8249618):
>>> although ProblemList and @ignore achieve the same end result (test 
>>> exclusion), their server different goals and have slightly different 
>>> meanings, simplified @ignore should be used to exclude useless or harmful 
>>> tests, and ProblemList in all other cases (see yet-not-integrated 
>>> `ProblemListing or `@ignore`-ing a Test` section of dev guide, PR -- 
>>> https://github.com/openjdk/guide/pull/21 
>>>  for more details). 
>>> 
>>> due to different reasons, this hasn't been always followed and some 
>>> currently @ignore-d tests should rather be ProblemList-ed, and some of 
>>> ProblemList-ed should be @ignore-d, this issue is to clean up the current 
>>> state in a hope that this will reduce further confusion. 
>> 
>> webrev: http://cr.openjdk.java.net/~iignatyev//8249698/webrev.00 
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8249698 
>> 
>> 
>> Thanks,
>> -- Igor
>> 
>> 8078602: https://bugs.openjdk.java.net/browse/JDK-8078602 
>> 
>> 8249618: https://bugs.openjdk.java.net/browse/JDK-8249618 
>> 

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

[Lisa Li]

Hi Julia, Brent, Jason and Rob,

Thanks a lot for all your help guys! It's been a wonderful experience for
me. First patch for OpenJDK, checked!

Cheers,
Yu L.

On Mon, Jul 20, 2020 at 8:18 PM Julia Boes  wrote:

> Hi Lisa,
>
> I added a newline at the end of the test and ran the relevant test set,
> which came back all clear.
>
> The change is pushed now.
>
> Thanks,
>
> Julia
>
> On 19/07/2020 11:48, Lisa Li wrote:
> > 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 mailto:liyu.it...@gmail.com
> >>
> >
> > 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() == 

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

[Julia Boes]

Hi Lisa,

I added a newline at the end of the test and ran the relevant test set, 
which came back all clear.


The change is pushed now.

Thanks,

Julia

On 19/07/2020 11:48, Lisa Li wrote:

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 mailto:liyu.it...@gmail.com>>

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
+ * @bug        8245694
+ * @summary    tests 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");
+        

Re: [PATCH] 8247402: Rewrite the implementation requirements for Map::compute()

[林自均]

Hi Martin,

It's been over a month since your last email. Do you have any update?

Best,
John Lin

林自均  於 2020年6月20日 週六 上午9:00寫道：
>
> Hi Martin,
>
> Any update on this? Thanks!
>
> Best,
> John Lin
>
> 林自均  於 2020年6月13日 週六 上午11:08寫道：
> >
> > Hi Martin,
> >
> > I see your point. Thank you for demostrating this for me.
> >
> > Here's my updated patch:
> >
> > # HG changeset patch
> > # User John Lin 
> > # Date 1591923561 -28800
> > #  Fri Jun 12 08:59:21 2020 +0800
> > # Node ID e01d9d020506a88d3d585bd3264594a26450c659
> > # Parent  49a68abdb0ba68351db0f140ddac793b1c391bd5
> > 8247402: Rewrite the implementation requirements for Map::compute()
> >
> > diff --git a/src/java.base/share/classes/java/util/Map.java
> > b/src/java.base/share/classes/java/util/Map.java
> > --- a/src/java.base/share/classes/java/util/Map.java
> > +++ b/src/java.base/share/classes/java/util/Map.java
> > @@ -1113,17 +1113,12 @@ public interface Map {
> >   *  {@code
> >   * V oldValue = map.get(key);
> >   * V newValue = remappingFunction.apply(key, oldValue);
> > - * if (oldValue != null) {
> > - *if (newValue != null)
> > - *   map.put(key, newValue);
> > - *else
> > - *   map.remove(key);
> > + * if (newValue != null) {
> > + * map.put(key, newValue);
> >   * } else {
> > - *if (newValue != null)
> > - *   map.put(key, newValue);
> > - *else
> > - *   return null;
> > + * map.remove(key);
> >   * }
> > + * return newValue;
> >   * }
> >   *
> >   * The default implementation makes no guarantees about detecting 
> > if the
> >
> > By the way, I saw everyone in this mailing list is using webrev.
> > However, the tutorial https://openjdk.java.net/guide/codeReview.html
> > says that only the users with push access to the OpenJDK Mercurial
> > server can use webrev. Can I apply for push access to the OpenJDK
> > Mercurial server too?
> >
> > Best,
> > John Lin
> >
> > Martin Buchholz  於 2020年6月13日 週六 上午12:36寫道：
> > >
> > > If I rub that program, I get:
> > >
> > > HashMap false
> > > HashMap1 false
> > > HashMap2 true
> > >
> > > which suggests that HashMap2's implementation is wrong.

[PATCH] trivial improvement of String.replace(CharSequence cs1, CharSequence cs2) for the case cs1 is empty

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

Hello,

I'd like to contribute this simple patch that improves performance of
 String.replace(CharSequence cs1, CharSequence cs2) for the case cs1 is empty.

The idea is to get rid of StringBuilder in favour of byte[] / char[].

To measure improvement I've used benchmark [1] that demonstrates improvement as 
of both memory and time consumption
for the case when I have String s = "" and I need to surround each char 'a' 
with 'b'. For this I call

String str = s.replace("", "b");

See the results for original JDK and patched one:

before
(latin)  (length)   
Mode  Cnt  Score Error   Units
insertBetweenEachChar  true 5  
thrpt   50  16992.105 ± 528.388  ops/ms
insertBetweenEachChar  true10  
thrpt   50   9119.878 ± 421.776  ops/ms
insertBetweenEachChar  true50  
thrpt   50   2270.251 ±  45.815  ops/ms
insertBetweenEachChar false 5  
thrpt   50  11468.241 ± 623.440  ops/ms
insertBetweenEachChar false10  
thrpt   50   7150.812 ± 299.363  ops/ms
insertBetweenEachChar false50  
thrpt   50   1613.470 ±  66.941  ops/ms

insertBetweenEachChar:·gc.alloc.rate.norm  true 5  
thrpt   50 88.010 ±   0.001B/op
insertBetweenEachChar:·gc.alloc.rate.norm  true10  
thrpt   50104.015 ±   0.001B/op
insertBetweenEachChar:·gc.alloc.rate.norm  true50  
thrpt   50264.046 ±   0.002B/op
insertBetweenEachChar:·gc.alloc.rate.norm false 5  
thrpt   50177.618 ±   5.880B/op
insertBetweenEachChar:·gc.alloc.rate.norm false10  
thrpt   50251.229 ±   4.801B/op
insertBetweenEachChar:·gc.alloc.rate.norm false50  
thrpt   50736.091 ±   0.005B/op

after

insertBetweenEachChar  true 5  
thrpt   50  19249.086 ± 325.786  ops/ms
insertBetweenEachChar  true10  
thrpt   50  12962.483 ±  61.992  ops/ms
insertBetweenEachChar  true50  
thrpt   50   3232.475 ±  16.864  ops/ms
insertBetweenEachChar false 5  
thrpt   50  26083.880 ± 218.046  ops/ms
insertBetweenEachChar false10  
thrpt   50  18305.650 ± 180.684  ops/ms
insertBetweenEachChar false50  
thrpt   50   5712.175 ±  37.051  ops/ms

insertBetweenEachChar:·gc.alloc.rate.norm  true 5  
thrpt   50 88.010 ±   0.001B/op
insertBetweenEachChar:·gc.alloc.rate.norm  true10  
thrpt   50104.012 ±   0.001B/op
insertBetweenEachChar:·gc.alloc.rate.norm  true50  
thrpt   50264.038 ±   0.002B/op
insertBetweenEachChar:·gc.alloc.rate.norm false 5  
thrpt   50136.012 ±   0.001B/op
insertBetweenEachChar:·gc.alloc.rate.norm false10  
thrpt   50192.017 ±   0.001B/op
insertBetweenEachChar:·gc.alloc.rate.norm false50  
thrpt   50592.053 ±   0.001B/op

Full output can be found in [2].

Patch is attached.

With best regards,
Sergey Tsypanov

[1] 
https://github.com/stsypanov/strings/blob/master/src/jmh/java/tsypanov/strings/string/InsertCharBenchmark.java
[2] 
https://github.com/stsypanov/strings/blob/master/results/InsertCharBenchmark.txtdiff --git a/src/java.base/share/classes/java/lang/String.java b/src/java.base/share/classes/java/lang/String.java
--- a/src/java.base/share/classes/java/lang/String.java
+++ b/src/java.base/share/classes/java/lang/String.java
@@ -2190,12 +2190,31 @@
 throw new OutOfMemoryError("Required length exceeds implementation limit");
 }
 
-StringBuilder sb = new StringBuilder(resultLen);
-sb.append(replStr);
+if (isLatin1() && trgtStr.isLatin1() && replStr.isLatin1()) {
+byte[] bytes = new byte[resultLen];
+
+byte[] replacementBytes = replStr.value;
+System.arraycopy(replacementBytes, 0, bytes, 0, replLen);
+
+int index = replLen;
+byte[] thisValue = this.value;
+for (int i = 0; i < thisLen; ++i) {
+bytes[index++] = thisValue[i];
+System.arraycopy(replacementBytes, 0, bytes, index, replLen);
+index += replLen;
+}
+return StringLatin1.newString(bytes, 0, index);
+}
+
+char[] chars = new char[resultLen];
+replStr.getChars(0, replLen, chars, 0);
+