date:20200721

Re: RFR: JDK-8247592: refactor test/jdk/tools/launcher/Test7029048.java

2020-07-21 Thread Mandy Chung


Hi Aleksei,


Webrev: http://cr.openjdk.java.net/~avoitylov/webrev.8247592.01/


This refactoring seems okay.  I would suggest to change the run method 
to return an int or boolean to indicate the test passed or failed.   The 
caller of the run method (i.e. runTest will add to the failedTests list 
if the return value indicates test failure.   No need to pass the 
failedTest list to the run method as an argument.


Typo in line 90: s/bug got/but got/

Otherwise, looks okay.

Mandy

On 7/21/20 10:37 AM, Aleksei Voitylov wrote:

Hi,

gently reminiding about this simple test refactoring. The patch still
applies cleanly.

-Aleksei

On 24/06/2020 11:44, Aleksei Voitylov wrote:

Hi,

I'd like to refactor test/jdk/tools/launcher/Test7029048.java, make the
logic easier to follow and remove some magic numbers from the test:

JBS: https://bugs.openjdk.java.net/browse/JDK-8247592
Webrev: http://cr.openjdk.java.net/~avoitylov/webrev.8247592.01/

Testing: the test passes on Linux x86, Linux x86_64, Linux ARM, Linux
AArch64, Linux PPC, Windows x86, Windows x86_64, Mac, AIX. Special
thanks to SAP team for helping test on AIX.

Thanks,
-Aleksei

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

2020-07-21 Thread naoto . sato


Hi Brent,

On 7/21/20 2:56 PM, Brent Christian wrote:

Hi, Naoto

I have a few comments:

src/java.base/share/classes/java/lang/StringUTF16.java

379 private static int getSupplementaryCodePoint(byte[] ba, int cp, 
int index, int start, int end)


I think it would be worth a small addition to the comment to reflect 
that non-surrogate chars are returned as-is.


Sure, I will add some more comments to the method.



--

I thought about the scenario of an unpaired low or high surrogate at the 
beginning or end of the string, respectively:


384 if (Character.isLowSurrogate((char)cp)) {
385 if (index > start) {
     ...
391 } else if (index + 1 < end) { // cp == high surrogate
392 char c = getChar(ba, index + 1);
     ...
397 return cp;

It looks like the cp itself would be returned from 
getSupplementaryCodePoint(). And then back in compareToCIImpl(), it's 
converted using Character.to[Upper|Lower]Case(int), which will also 
return the cp itself.  I imagine that's the best we could do, so seems 
fine.


Yes, that is exactly what is intended.



Is there a test case for unmatched surrogates at the beginning and end 
of the string ? Should there be ?


Interestingly, there has been a test case for supplementary characters 
before this change, where it intentionally begins from a low surrogate, 
and ends with a high surrogate, so that it would succeed in the previous 
*exact match* logic. Line 82 in RegionMatches.java tests:
"\uD801\uDC28\uD801\uDC29\uFF41a".regionMatches(true, 1, "\uDC28\uD801", 
0, 2) == true

And the proposed change is compatible with this test case.



--

I see there are no changes to StringLatin1.regionMatchesCI_UTF16().  I 
presume there are no cases in which toUpperCase(toLowerCase()) of a 
supplementary character could yield a Latin-1 character, yes?


Yes, that is correct.

Naoto



Also, thanks for adding the benchmark!

-Brent

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

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

2020-07-21 Thread Brent Christian


Hi, Naoto

I have a few comments:

src/java.base/share/classes/java/lang/StringUTF16.java

379 private static int getSupplementaryCodePoint(byte[] ba, int cp, 
int index, int start, int end)


I think it would be worth a small addition to the comment to reflect 
that non-surrogate chars are returned as-is.


--

I thought about the scenario of an unpaired low or high surrogate at the 
beginning or end of the string, respectively:


384 if (Character.isLowSurrogate((char)cp)) {
385 if (index > start) {
...
391 } else if (index + 1 < end) { // cp == high surrogate
392 char c = getChar(ba, index + 1);
...
397 return cp;

It looks like the cp itself would be returned from 
getSupplementaryCodePoint(). And then back in compareToCIImpl(), it's 
converted using Character.to[Upper|Lower]Case(int), which will also 
return the cp itself.  I imagine that's the best we could do, so seems fine.


Is there a test case for unmatched surrogates at the beginning and end 
of the string ? Should there be ?


--

I see there are no changes to StringLatin1.regionMatchesCI_UTF16().  I 
presume there are no cases in which toUpperCase(toLowerCase()) of a 
supplementary character could yield a Latin-1 character, yes?


Also, thanks for adding the benchmark!

-Brent

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

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

2020-07-21 Thread naoto . sato


Thank you, Joe. I got it now. Will try out and benchmark.

Naoto

On 7/21/20 10:05 AM, Joe Wang wrote:



On 7/20/2020 8:58 PM, naoto.s...@oracle.com wrote:
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?


What I meant was that we could expand the 'short-cut' from case 
sensitive to case insensitive, that is in addition to the line 337, do 
that line 353 - 370 case-insensitive check as well.


I guess it can be explained better with code. I added inline comment:

     for (int k1 = toffset, k2 = ooffset; k1 < tlast && k2 < olast; 
k1++, k2++) {

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

// does a case-insensitive check:

     if (checkEqual(cp1, cp2) == 0) {
     continue;
     }

// this block will be run once for strings that don't contain any 
supplementary characters


  // Check for supplementary characters case
     cp1 = getSupplementaryCodePoint(value, cp1, k1, toffset, 
tlast);

     if ((cp1 & Integer.MIN_VALUE) != 0) {
     k1++;
     cp1 ^= Integer.MIN_VALUE;
     }
     cp2 = getSupplementaryCodePoint(other, cp2, k2, ooffset, 
olast);

     if ((cp2 & Integer.MIN_VALUE) != 0) {
     k2++;
     cp2 ^= Integer.MIN_VALUE;
     }


// thischeck will have been called twice for strings that contain 
supplementary characters

// but only one more for strings that don't

     int diff = checkEqual(cp1, cp2);
     if (diff != 0) {
     return diff;
     }
     }
     return tlen - olen;
     }

// the code block between line 353 - 370 in webrev.04 except the last 
line (return 0):

     private static int checkEqual(int cp1, int cp2) {
     if (cp1 != cp2) {
     // try converting both characters to uppercase.
     // If the results match, then the comparison scan should
     // continue.
     cp1 = Character.toUpperCase(cp1);
     cp2 = Character.toUpperCase(cp2);
     if (cp1 != cp2) {
     // Unfortunately, conversion to uppercase does not work 
properly
     // for the Georgian alphabet, which has strange rules 
about case

     // conversion.  So we need to make one last check before
     // exiting.
     cp1 = Character.toLowerCase(cp1);
     cp2 = Character.toLowerCase(cp2);
     if (cp1 != cp2) {
     return cp1 - cp2;
     }
     }
     }
     return 0;
     }





Naoto

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

2020-07-21 Thread naoto . sato


Thank you, Roger.

On 7/21/20 7:50 AM, Roger Riggs wrote:

Hi Naoto,

StringUTF16.java:

343, 348: The masking and comparisons seem awkward.
I'd suggest just negating the value and testing for < 0 to flag the less 
usual case.
If you continue with the flag bit, define your own static final constant 
for that bit;

It looks odd to be using Integer.MIN_VALUE with bit operators.


Right. I will change it to negating.



Performance wise, I'm a bit cautious about all the if's and branches;

And only calling getSupplementaryCodePoint if cp was a suggorate might 
have some

benefit, but its hard to tell what will be inlined and when.


I think Joe's suggestion will help here. Will update the webrev.

Naoto



Thanks, Roger



On 7/20/20 6: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 
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 ?

Re: RFR: JDK-8248239: jpackage adds some arguments twice in case it is re-executed by JLI

2020-07-21 Thread Alexey Semenyuk


Hi Aleksei,

Looks good!

- Alexey

On 7/21/2020 11:42 AM, Aleksei Voitylov wrote:

Hi,

This is the updated fix which checks if LD_LIBRARY_PATH has been changed
during jpackage executions:

http://cr.openjdk.java.net/~avoitylov/webrev.8248239.03/

The fix stores hash from LD_LIBRARY_PATH in _JPACKAGE_LAUNCHER env
variable. Until C++14 becomes mandatory for OpenJDK, a custom hash
algorithm is used because standard C++ hash requires -std=c++11 or
-std=gnu++11 compiler options.

All test/jdk/tools/jpackage tests but ModulePathTest3.java pass on Linux
x86_64, MacOSX x86_64, and Windows x86_64. The test ModulePathTest3.java
is excluded in test/jdk/ProblemList.txt (8248418 generic-all).

Thanks,

-Aleksei

On 26/06/2020 20:23, Alexey Semenyuk wrote:

Hi Aleksei,

I think the idea of partial reading data from cfg file when the
launcher is restarted has a flaw.
It would be better if app launcher can detect if it was restarted and
if it was, not read cfg file at all, but pass command line arguments
as is in JLI_Launch().
Let my think on ideas how to address this.

- Alexey

On 6/26/2020 7:16 AM, Aleksei Voitylov wrote:

Hi Alexey,

Thank you for looking into it. As far as using parent pid, it does not
seem to work as example [1] demonstrates. The parent process remains the
same after re-execution and does not become the current process.

I checked passing arguments in the "ArgOption" section using several
cases [2, 3, 4] with the proposed fix and app re-execution. The proposed
fix handles this case well, and the results are the same as without the
fix when the app is not re-executed.

Case [3] where only JavaOptions without ArgOptions are passed to app
looks suspicious because default ArgOptions are not used. But it works
the same way without the proposed fix, which seems like a different
issue. According to jpackage documentation:

    --arguments main class arguments
  Command line arguments to pass to the main class if no command
line arguments are given to the launcher.

I filed a separate issue [5] to handle that.

Thanks,
-Aleksei


[1]
cd jdk-dev
make jdk-image
export PATH=build/linux-x86_64-server-release/images/jdk/bin:$PATH
export
LD_LIBRARY_PATH=build/linux-x86_64-server-release/images/jdk/lib/server
jpackage --dest output --name app --type app-image --module-path
input-modules --module com.hello/com.hello.Hello --arguments "A A2 A"
--verbose --java-options -Dparam1=Param1
./output/app/bin/app -Dparam2=Param2 B B2 B
-
pid: 16007, current process: /home/sample/jdk-dev/output/app/bin/app
pid: 15927, parent  process: /bin/bash
JvmLauncher args: 10 [/home/sample/jdk-dev/output/app/bin/app
-Dparam1=Param1 --module-path
/home/sample/jdk-dev/output/app/lib/app/mods -m
com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ]
pid: 16007, current process: /home/sample/jdk-dev/output/app/bin/app
pid: 15927, parent  process: /bin/bash
JvmLauncher args: 15 [/home/sample/jdk-dev/output/app/bin/app
-Dparam1=Param1 --module-path
/home/sample/jdk-dev/output/app/lib/app/mods -m
com.hello/com.hello.Hello -Dparam1=Param1 --module-path
/home/sample/jdk-dev/output/app/lib/app/mods -m
com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ]
-

[2]
jpackage --dest output --name app --type app-image --module-path
input-modules --module com.hello/com.hello.Hello --arguments "A A2 A"
--java-options -Dparam1=Param1
./output/app/bin/app
JvmLauncher args: 9 [output/app/bin/app -Dparam1=Param1 --module-path
output/app/lib/app/mods -m com.hello/com.hello.Hello A A2 A ]
JvmLauncher args: 9 [output/app/bin/app -Dparam1=Param1 --module-path
output/app/lib/app/mods -m com.hello/com.hello.Hello A A2 A ]

[3]
jpackage --dest output --name app --type app-image --module-path
input-modules --module com.hello/com.hello.Hello --arguments "A A2 A"
--java-options -Dparam1=Param1
./output/app/bin/app -Dparam2=Param2
JvmLauncher args: 7 [output/app/bin/app -Dparam1=Param1 --module-path
output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 ]
JvmLauncher args: 7 [output/app/bin/app -Dparam1=Param1 --module-path
output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 ]

[4]
jpackage --dest output --name app --type app-image --module-path
input-modules --module com.hello/com.hello.Hello --arguments "A A2 A"
--java-options -Dparam1=Param1
./output/app/bin/app -Dparam2=Param2 B B2 B
JvmLauncher args: 10 [output/app/bin/app -Dparam1=Param1 --module-path
output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B
B2 B ]
JvmLauncher args: 10 [output/app/bin/app -Dparam1=Param1 --module-path
output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B
B2 B ]

[5] https://bugs.openjdk.java.net/browse/JDK-8248397


On 24/06/2020 19:34, Alexey Semenyuk wrote:

Aleksei,

If I get it right, the fix would not work for the case when there are
`arguments` properties in `ArgOptions` section of a config file.
Why not just check if the parent process is the same executable as the
current one and if

Re: Feature Request

2020-07-21 Thread Justin Dekeyser

Hello,

Wouldn't that be too dangerous, in the sense that it makes us lose control
over how the String are actually constructed and aggregated together?

Aside from that, what kind of interpolation are you looking for, and how
the String.format utility does not meet your needs?

Regards,

D.J.

On Tue, Jul 21, 2020 at 7:08 PM Jessica  wrote:

> Hi,
>
> I would like to strongly request Java have the ability to do string
> interpolation.
> It is such a great convenience and allows for faster, more efficient coding
> that is easier to read.
>
> Is this feature on the roadmap in the foreseeable future?
>
> Thank you!
>
> Jessica Risavi
>
> <
> http://www.avg.com/email-signature?utm_medium=email_source=link_campaign=sig-email_content=webmail
> >
> Virus-free.
> www.avg.com
> <
> http://www.avg.com/email-signature?utm_medium=email_source=link_campaign=sig-email_content=webmail
> >
> <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
>

Re: RFR: JDK-8247592: refactor test/jdk/tools/launcher/Test7029048.java

2020-07-21 Thread Aleksei Voitylov

Hi,

gently reminiding about this simple test refactoring. The patch still
applies cleanly.

-Aleksei

On 24/06/2020 11:44, Aleksei Voitylov wrote:
> Hi,
>
> I'd like to refactor test/jdk/tools/launcher/Test7029048.java, make the
> logic easier to follow and remove some magic numbers from the test:
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8247592
> Webrev: http://cr.openjdk.java.net/~avoitylov/webrev.8247592.01/
>
> Testing: the test passes on Linux x86, Linux x86_64, Linux ARM, Linux
> AArch64, Linux PPC, Windows x86, Windows x86_64, Mac, AIX. Special
> thanks to SAP team for helping test on AIX.
>
> Thanks,
> -Aleksei
>

Re: Feature Request

2020-07-21 Thread Rob Spoor

If you need something right now, you can try 
https://commons.apache.org/proper/commons-text/javadocs/api-release/org/apache/commons/text/StringSubstitutor.html.



On 21/07/2020 09:29, Jessica wrote:

Hi,

I would like to strongly request Java have the ability to do string
interpolation.
It is such a great convenience and allows for faster, more efficient coding
that is easier to read.

Is this feature on the roadmap in the foreseeable future?

Thank you!

Jessica Risavi


Virus-free.
www.avg.com

<#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

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

2020-07-21 Thread Lance Andersen

Hi Roger,

I will plan to push tomorrow morning pending any last minute reviews

Best
Lance

> On Jul 21, 2020, at 9:57 AM, Roger Riggs  wrote:
> 
> Hi Rafaello, Lance,
> 
> That looks good to me.  
> 
> Thanks, Roger
> 
> On 7/19/20 2:31 PM, Lance Andersen wrote:
>> 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 
>>> mailto:raffaello.giulie...@gmail.com>> 
>>> 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
>>> +
>>> +/*

Re: [aarch64-port-dev ] RFR (XXL): 8223347: Integration of Vector API (Incubator): AArch64 backend changes

2020-07-21 Thread Ningsheng Jian


Hi Andrew and all,

Since we are getting ready to propose Vector API target to JDK 16 [1]. I 
have regenerated webrev of aarch64 backend parts from panama repo, which 
has been rebased to jdk/jdk very recently, by:


$ hg update vector-unstable && hg diff -r default > all.patch
$ grep "diff -r" all.patch | grep -e "src/hotspot/cpu/aarch64" | awk 
'{print $4}' > aarch64_list

$ ksh ./webrev.ksh -r default -o aarch64_webrev aarch64_list

The new webrev:
http://cr.openjdk.java.net/~njian/vectorapi/8223347-integration/aarch64-webrev.01/

Could you please help to take a look?

Yang's previous webrevs can still be found at [2], with review comments 
addressed in the latest webrev above.


[1] http://mail.openjdk.java.net/pipermail/hotspot-dev/2020-July/042427.html
[2] 
http://cr.openjdk.java.net/~yzhang/vectorapi/vectorapi.rfr/aarch64_webrev/



Thanks,
Ningsheng

On 7/8/20 3:05 PM, Yang Zhang wrote:

Hi Andrew

I have updated this patch. Could you please help to review it again?
In this patch, the following changes are made:
1. Separate newly added NEON instructions to a new ad file
aarch64_neon.ad
2. Add assembler tests for NEON instructions. Trailing spaces
in the python script are also removed.

http://cr.openjdk.java.net/~yzhang/vectorapi/vectorapi.rfr/aarch64_webrev/webrev.02/

Thanks,
Yang


-Original Message-
From: Andrew Haley 
Sent: Tuesday, June 30, 2020 12:10 AM
To: Yang Zhang ; Viswanathan, Sandhya 
; Paul Sandoz 
Cc: nd ; hotspot-compiler-...@openjdk.java.net; 
hotspot-...@openjdk.java.net; core-libs-dev@openjdk.java.net; 
aarch64-port-...@openjdk.java.net
Subject: Re: [aarch64-port-dev ] RFR (XXL): 8223347: Integration of Vector API 
(Incubator): AArch64 backend changes

On 29/06/2020 08:48, Yang Zhang wrote:

1. Instructions that can be matched with NEON instructions directly.
MulVB, SqrtVF and AbsV have been merged into jdk master already.

2. Instructions that jdk master has middle end support for, but they cannot be 
matched with NEON instructions directly.
Such as AddReductionVL, MulReductionVL, And/Or/XorReductionV These new 
instructions can be moved into jdk master first, but for auto-vectorization, 
the performance might not get improved.

3. Panama/Vector API specific  instructions such as Load/StoreVector ( 16 
bits), VectorReinterpret, VectorMaskCmp, MaxV/MinV, VectorBlend etc.
These instructions cannot be moved into jdk master first because there isn't 
middle-end support.

I will put 2 and 3 in a new ad file aarch64_neon.ad. I will also update 
aarch64_asmtest.py and macroassemler.cpp. When the patch is ready, I will send 
it again.


Thank you *very* much for your hard work. Appreciated!

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

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

2020-07-21 Thread Mark Davis ☕️

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.

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:
> BenchmarkMode  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:
> BenchmarkMode  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
>

Feature Request

2020-07-21 Thread Jessica

Hi,

I would like to strongly request Java have the ability to do string
interpolation.
It is such a great convenience and allows for faster, more efficient coding
that is easier to read.

Is this feature on the roadmap in the foreseeable future?

Thank you!

Jessica Risavi


Virus-free.
www.avg.com

<#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

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

2020-07-21 Thread Joe Wang





On 7/20/2020 8:58 PM, naoto.s...@oracle.com wrote:
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?


What I meant was that we could expand the 'short-cut' from case 
sensitive to case insensitive, that is in addition to the line 337, do 
that line 353 - 370 case-insensitive check as well.


I guess it can be explained better with code. I added inline comment:

    for (int k1 = toffset, k2 = ooffset; k1 < tlast && k2 < olast; 
k1++, k2++) {

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

// does a case-insensitive check:

    if (checkEqual(cp1, cp2) == 0) {
    continue;
    }

// this block will be run once for strings that don't contain any 
supplementary characters


 // Check for supplementary characters case
    cp1 = getSupplementaryCodePoint(value, cp1, k1, toffset, 
tlast);

    if ((cp1 & Integer.MIN_VALUE) != 0) {
    k1++;
    cp1 ^= Integer.MIN_VALUE;
    }
    cp2 = getSupplementaryCodePoint(other, cp2, k2, ooffset, 
olast);

    if ((cp2 & Integer.MIN_VALUE) != 0) {
    k2++;
    cp2 ^= Integer.MIN_VALUE;
    }


// thischeck will have been called twice for strings that contain 
supplementary characters

// but only one more for strings that don't

    int diff = checkEqual(cp1, cp2);
    if (diff != 0) {
    return diff;
    }
    }
    return tlen - olen;
    }

// the code block between line 353 - 370 in webrev.04 except the last 
line (return 0):

    private static int checkEqual(int cp1, int cp2) {
    if (cp1 != cp2) {
    // try converting both characters to uppercase.
    // If the results match, then the comparison scan should
    // continue.
    cp1 = Character.toUpperCase(cp1);
    cp2 = Character.toUpperCase(cp2);
    if (cp1 != cp2) {
    // Unfortunately, conversion to uppercase does not work 
properly
    // for the Georgian alphabet, which has strange rules 
about case

    // conversion.  So we need to make one last check before
    // exiting.
    cp1 = Character.toLowerCase(cp1);
    cp2 = Character.toLowerCase(cp2);
    if (cp1 != cp2) {
    return cp1 - cp2;
    }
    }
    }
    return 0;
    }





Naoto

Re: Build error with GCC 10 in NetworkInterface.c and k_standard.c

2020-07-21 Thread Ioi Lam





On 7/17/20 6:48 AM, Yasumasa Suenaga wrote:

Hi Koichi,

On 2020/07/17 20:26, Koichi Sakata wrote:

Hi Daniel,

 > The changes to NetworkInterface.c look good to me.

Thank you.

 > You'll need to find a reviewer that understands what that
 > method is supposed to do in that case, that's not me ;-)

I understand. This ML is suitable for finding a reviewer, isn't it?
Or, there is another way. We can avoid the error by the accepting 
maybe-uninitialized warning in libfdlibm instead of fixing k_standard.c.


diff --git a/make/modules/java.base/lib/CoreLibraries.gmk 
b/make/modules/java.base/lib/CoreLibraries.gmk

--- a/make/modules/java.base/lib/CoreLibraries.gmk
+++ b/make/modules/java.base/lib/CoreLibraries.gmk
@@ -48,7 +48,7 @@
  CFLAGS := $(CFLAGS_JDKLIB) $(LIBFDLIBM_CFLAGS), \
  CFLAGS_windows_debug := -DLOGGING, \
  CFLAGS_aix := -qfloat=nomaf, \
-    DISABLED_WARNINGS_gcc := sign-compare misleading-indentation 
array-bounds, \
+    DISABLED_WARNINGS_gcc := sign-compare misleading-indentation 
array-bounds maybe-uninitialized, \

  DISABLED_WARNINGS_clang := sign-compare, \
  DISABLED_WARNINGS_microsoft := 4146 4244 4018, \
  ARFLAGS := $(ARFLAGS), \


This change affects all of C sources in core-libs.
So I think it is better affect to k_standard.c only as below:

```
diff --git a/make/modules/java.base/lib/CoreLibraries.gmk 
b/make/modules/java.base/lib/CoreLibraries.gmk

--- a/make/modules/java.base/lib/CoreLibraries.gmk
+++ b/make/modules/java.base/lib/CoreLibraries.gmk
@@ -39,6 +39,10 @@
 LIBFDLIBM_SRC := $(TOPDIR)/src/java.base/share/native/libfdlibm
 LIBFDLIBM_CFLAGS := -I$(LIBFDLIBM_SRC) $(FDLIBM_CFLAGS)

+ifeq ($(call isTargetOs, linux), true)
+  BUILD_LIBFDLIBM_k_standard.c_CFLAGS += -Wno-maybe-uninitialized
+endif
+
 $(eval $(call SetupNativeCompilation, BUILD_LIBFDLIBM, \
 NAME := fdlibm, \
 TYPE := STATIC_LIBRARY, \
```

You can pass compiler option to specified file.





Instead of modifying the makefiles, how about using a pragma in 
k_standard.c?


#ifdef __GNUC__
#if __GNUC__ >= 10
#pragma GCC diagnostic ignored "-Wno-maybe-uninitialized"
#endif
#endif


Thanks
- Ioi



Thanks,

Yasumasa



Thanks,
Koichi

On 2020/07/15 3:36, Daniel Fuchs wrote:

Hi Koichi,

On 13/07/2020 08:03, Koichi Sakata wrote:
 > I understand that. I respect your idea.
 > I fixed the patch as follows.
The changes to NetworkInterface.c look good to me.

 >
 > By the way, k_standard.c still remains. Is there a way to proceed 
with it?


You'll need to find a reviewer that understands what that
method is supposed to do in that case, that's not me ;-)

best regards,

-- daniel


Thanks, > Koichi

= PATCH =
diff --git a/src/java.base/unix/native/libnet/NetworkInterface.c 
b/src/java.base/unix/native/libnet/NetworkInterface.c

--- a/src/java.base/unix/native/libnet/NetworkInterface.c
+++ b/src/java.base/unix/native/libnet/NetworkInterface.c
@@ -1296,7 +1296,8 @@
  static int getIndex(int sock, const char *name) {
  struct ifreq if2;
  memset((char *), 0, sizeof(if2));
-    strncpy(if2.ifr_name, name, sizeof(if2.ifr_name) - 1);
+    strncpy(if2.ifr_name, name, sizeof(if2.ifr_name));
+    if2.ifr_name[sizeof(if2.ifr_name) - 1] = 0;

  if (ioctl(sock, SIOCGIFINDEX, (char *)) < 0) {
  return -1;
@@ -1359,7 +1360,8 @@
  static int getFlags(int sock, const char *ifname, int *flags) {
  struct ifreq if2;
  memset((char *), 0, sizeof(if2));
-    strncpy(if2.ifr_name, ifname, sizeof(if2.ifr_name) - 1);
+    strncpy(if2.ifr_name, ifname, sizeof(if2.ifr_name));
+    if2.ifr_name[sizeof(if2.ifr_name) - 1] = 0;

  if (ioctl(sock, SIOCGIFFLAGS, (char *)) < 0) {
  return -1;
diff --git a/src/java.base/share/native/libfdlibm/k_standard.c 
b/src/java.base/share/native/libfdlibm/k_standard.c

--- a/src/java.base/share/native/libfdlibm/k_standard.c
+++ b/src/java.base/share/native/libfdlibm/k_standard.c
@@ -739,6 +739,10 @@
  errno = EDOM;
  }
  break;
+    default:
+    exc.retval = zero;
+    errno = EINVAL;
+    break;
  }
  return exc.retval;
  }

Re: RFR: JDK-8248239: jpackage adds some arguments twice in case it is re-executed by JLI

2020-07-21 Thread Aleksei Voitylov

Hi,

This is the updated fix which checks if LD_LIBRARY_PATH has been changed
during jpackage executions:

http://cr.openjdk.java.net/~avoitylov/webrev.8248239.03/

The fix stores hash from LD_LIBRARY_PATH in _JPACKAGE_LAUNCHER env
variable. Until C++14 becomes mandatory for OpenJDK, a custom hash
algorithm is used because standard C++ hash requires -std=c++11 or
-std=gnu++11 compiler options.

All test/jdk/tools/jpackage tests but ModulePathTest3.java pass on Linux
x86_64, MacOSX x86_64, and Windows x86_64. The test ModulePathTest3.java
is excluded in test/jdk/ProblemList.txt (8248418 generic-all).

Thanks,

-Aleksei

On 26/06/2020 20:23, Alexey Semenyuk wrote:
> Hi Aleksei,
>
> I think the idea of partial reading data from cfg file when the
> launcher is restarted has a flaw.
> It would be better if app launcher can detect if it was restarted and
> if it was, not read cfg file at all, but pass command line arguments
> as is in JLI_Launch().
> Let my think on ideas how to address this.
>
> - Alexey
>
> On 6/26/2020 7:16 AM, Aleksei Voitylov wrote:
>> Hi Alexey,
>>
>> Thank you for looking into it. As far as using parent pid, it does not
>> seem to work as example [1] demonstrates. The parent process remains the
>> same after re-execution and does not become the current process.
>>
>> I checked passing arguments in the "ArgOption" section using several
>> cases [2, 3, 4] with the proposed fix and app re-execution. The proposed
>> fix handles this case well, and the results are the same as without the
>> fix when the app is not re-executed.
>>
>> Case [3] where only JavaOptions without ArgOptions are passed to app
>> looks suspicious because default ArgOptions are not used. But it works
>> the same way without the proposed fix, which seems like a different
>> issue. According to jpackage documentation:
>>
>>    --arguments main class arguments
>>  Command line arguments to pass to the main class if no command
>> line arguments are given to the launcher.
>>
>> I filed a separate issue [5] to handle that.
>>
>> Thanks,
>> -Aleksei
>>
>>
>> [1]
>> cd jdk-dev
>> make jdk-image
>> export PATH=build/linux-x86_64-server-release/images/jdk/bin:$PATH
>> export
>> LD_LIBRARY_PATH=build/linux-x86_64-server-release/images/jdk/lib/server
>> jpackage --dest output --name app --type app-image --module-path
>> input-modules --module com.hello/com.hello.Hello --arguments "A A2 A"
>> --verbose --java-options -Dparam1=Param1
>> ./output/app/bin/app -Dparam2=Param2 B B2 B
>> -
>> pid: 16007, current process: /home/sample/jdk-dev/output/app/bin/app
>> pid: 15927, parent  process: /bin/bash
>> JvmLauncher args: 10 [/home/sample/jdk-dev/output/app/bin/app
>> -Dparam1=Param1 --module-path
>> /home/sample/jdk-dev/output/app/lib/app/mods -m
>> com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ]
>> pid: 16007, current process: /home/sample/jdk-dev/output/app/bin/app
>> pid: 15927, parent  process: /bin/bash
>> JvmLauncher args: 15 [/home/sample/jdk-dev/output/app/bin/app
>> -Dparam1=Param1 --module-path
>> /home/sample/jdk-dev/output/app/lib/app/mods -m
>> com.hello/com.hello.Hello -Dparam1=Param1 --module-path
>> /home/sample/jdk-dev/output/app/lib/app/mods -m
>> com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ]
>> -
>>
>> [2]
>> jpackage --dest output --name app --type app-image --module-path
>> input-modules --module com.hello/com.hello.Hello --arguments "A A2 A"
>> --java-options -Dparam1=Param1
>> ./output/app/bin/app
>> JvmLauncher args: 9 [output/app/bin/app -Dparam1=Param1 --module-path
>> output/app/lib/app/mods -m com.hello/com.hello.Hello A A2 A ]
>> JvmLauncher args: 9 [output/app/bin/app -Dparam1=Param1 --module-path
>> output/app/lib/app/mods -m com.hello/com.hello.Hello A A2 A ]
>>
>> [3]
>> jpackage --dest output --name app --type app-image --module-path
>> input-modules --module com.hello/com.hello.Hello --arguments "A A2 A"
>> --java-options -Dparam1=Param1
>> ./output/app/bin/app -Dparam2=Param2
>> JvmLauncher args: 7 [output/app/bin/app -Dparam1=Param1 --module-path
>> output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 ]
>> JvmLauncher args: 7 [output/app/bin/app -Dparam1=Param1 --module-path
>> output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 ]
>>
>> [4]
>> jpackage --dest output --name app --type app-image --module-path
>> input-modules --module com.hello/com.hello.Hello --arguments "A A2 A"
>> --java-options -Dparam1=Param1
>> ./output/app/bin/app -Dparam2=Param2 B B2 B
>> JvmLauncher args: 10 [output/app/bin/app -Dparam1=Param1 --module-path
>> output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B
>> B2 B ]
>> JvmLauncher args: 10 [output/app/bin/app -Dparam1=Param1 --module-path
>> output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B
>> B2 B ]
>>
>> [5] https://bugs.openjdk.java.net/browse/JDK-8248397
>>
>>
>> On 24/06/2020 19:34, Alexey Semenyuk wrote:
>>> Aleksei,
>>>
>>> If I get it right, the fix

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

2020-07-21 Thread Roger Riggs


Hi Naoto,

StringUTF16.java:

343, 348: The masking and comparisons seem awkward.
I'd suggest just negating the value and testing for < 0 to flag the less 
usual case.
If you continue with the flag bit, define your own static final constant 
for that bit;

It looks odd to be using Integer.MIN_VALUE with bit operators.

Performance wise, I'm a bit cautious about all the if's and branches;

And only calling getSupplementaryCodePoint if cp was a suggorate might 
have some

benefit, but its hard to tell what will be inlined and when.

Thanks, Roger



On 7/20/20 6: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 
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

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

2020-07-21 Thread Roger Riggs


Hi Rafaello, Lance,

That looks good to me.

Thanks, Roger

On 7/19/20 2:31 PM, Lance Andersen wrote:

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) {
eof = true;
-    if (nextin != 18) {
-    if (nextin == 12)
-    throw new IOException("Base64 stream has one 
un-decoded dangling byte.");

-    // treat ending xx/xxx

Re: RFR: JDK-8247592: refactor test/jdk/tools/launcher/Test7029048.java

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

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

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

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

Re: RFR: JDK-8248239: jpackage adds some arguments twice in case it is re-executed by JLI

Re: Feature Request

Re: RFR: JDK-8247592: refactor test/jdk/tools/launcher/Test7029048.java

Re: Feature Request

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

Re: [aarch64-port-dev ] RFR (XXL): 8223347: Integration of Vector API (Incubator): AArch64 backend changes

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

Feature Request

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

Re: Build error with GCC 10 in NetworkInterface.c and k_standard.c

Re: RFR: JDK-8248239: jpackage adds some arguments twice in case it is re-executed by JLI

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

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

18 matches

Site Navigation

Mail list logo

Footer information