date:20180424

Re: RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.

2018-04-24 Thread Simon Nash


On 24/04/2018 15:08, Lindenmaier, Goetz wrote:
Hi, 

I implemented what we figured out in 
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-April/027555.html


Some examples:
"Index 12 out-of-bounds for length 10."
"arraycopy source index -17 out of bounds for object array[10]."
"arraycopy destination index -18 out of bounds for double[5]."
"arraycopy length -19 is negative."
"arraycopy: last source index 13 out of bounds for double[10]."
"arraycopy: last destination index 7 out of bounds for long[5]."


Is there a reason why the first message says "out-of-bounds" but all others
say "out of bounds"?

Simon


Incremental webrev:
http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/03-incremental/
Full webrev:
http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/03/

I'll push it through our tests tonight.

See further comments in line:


-Original Message-
From: David Holmes [mailto:david.hol...@oracle.com]
Sent: Freitag, 20. April 2018 09:25
To: Lindenmaier, Goetz ; hotspot-runtime-
d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; aarch64-
port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; core-libs-
dev Libs 
Subject: Re: RFR(M): 8201593: Print array length in
ArrayIndexOutOfBoundsException.

Hi Goetz,

This is not a file by file review ...

On 19/04/2018 10:24 PM, Lindenmaier, Goetz wrote:

Hi,

New webrev:
http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/02/

I admit my wording is not optimal.

src/hotspot/share/oops/typeArrayKlass.cpp

Sorry but this is still far too verbose for my tastes. The type of the
array is not relevant. For the array copy its okay to indicate src or
dst array. But the message should be clear and succinct not prose-like.
Plus you have a lot of repetition in the ss.print statements when only
one thing is changing.

We discussed this in some further mails. Implemented as proposed there.


src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp

I'm not seeing why the throw_index_out_of_bounds_exception should be
tied to whether or not you have an array reference. Certainly I hate
seeing the array ref being used as an implicit bool!

I split the constructor into two, one for ArrayIndexOutOfBounds, the other
for IndexOutOfBounds.

...


I extended the test to cover the exception thrown in arraycopy better

The test seems somewhat excessive, and I now see it is because of the
more elaborate error messages you have at SAP. But with only the index
and the array length of interest here the test can be considerably smaller.

The creation tests for ArrayIndexOutOfBoundsException don't seem
relevant in this context either. This looks more TCK like.

Yes, the constructor tests are for the code not yet contributed.
I simplified the tests to only check the messages.

Best regards,
  Goetz.






David
-


and added the elementary type to the message text.  This probably
needs improvement in the text, too.  There are (currently) these cases:

bject[]  oa1 = new Object[10];
Object[]  oa2 = new Object[5];
System.arraycopy(oa1, -17, oa2, 0, 5);
"while trying to copy from index -17 of an object array with length 10");
System.arraycopy(oa1, 2, oa2, -18, 5);
"while trying to copy to index -18 of an object array with length 5");
System.arraycopy(oa1, 2, oa2, 0, -19);
"while trying to copy a negative range -19 from an object array with length

10 to an object array with length 5");

System.arraycopy(oa1, 8, oa2, 0, 5);
"while trying to copy from index 13 of an object array with length 10");
System.arraycopy(oa1, 1, oa2, 0, 7);
"while trying to copy to index 7 of an object array with length 5");
double[]  ta1 = new double[10];
double[]  ta2 = new double[5];
System.arraycopy(ta1, -17, ta2, 0, 5);
"while trying to copy from index -17 of a doubl array with length 10");
System.arraycopy(ta1, 2, ta2, -18, 5);
"while trying to copy to index -18 of a double array with length 5");
System.arraycopy(ta1, 2, ta2, 0, -19);
"while trying to copy a negative range -19 from a double array with length

10 to a double array with length 5");

System.arraycopy(ta1, 8, ta2, 0, 5);
"while trying to copy from index 13 of a double array with length 10");
System.arraycopy(ta1, 1, ta2, 0, 7);
"while trying to copy to index 7 of a double array with length 5");

Maybe it should say:
Arraycopy source index -1 out-of-bounds for double array of length 10.
Arraycopy target index 10 out-of-bounds for object array of length 10.
Negative range -19 when copying from an object array of length 10 to an

object array of length 5.

Best regards,
   Goetz.


-Original Message-
From: David Holmes [mailto:david.hol...@oracle.com]
Sent: Mittwoch, 18. April 2018 10:55
To: Lindenmaier, Goetz ; hotspot-runtime-
d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;

aarch64-

port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; core-

libs-

dev Libs

Re: RFR of JDK-8201469,test under java/rmi should be restricted to not run concurrently

2018-04-24 Thread joe darcy

Combining the reformatting and test additions makes the patch more 
difficult to review. The net effect seems to be adding the following 
directories to the exclusiveAccess.dirs list:


  44 sun/management/jmxremote \
  45 sun/rmi \
  46 sun/security/mscapi \
  47 sun/tools/jstatd

Is that correct?

Please characterize the number of tests added to the list and the 
expected performance impact on the tier 3 test group.


Thanks,

-Joe


On 4/23/2018 11:47 AM, Paul Sandoz wrote:

Hi Hamlin,

Do you have a sense of what rmi tests are problematic to run concurrently? 
Maybe you can subset to reduce the increased impact on execution time?

Paul.


On Apr 20, 2018, at 12:21 AM, Hamlin Li  wrote:

Is someone available to review the following patch?

Thank you

-Hamlin


On 19/04/2018 4:17 PM, Hamlin Li wrote:

would you please review the following patch?

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

http://cr.openjdk.java.net/~mli/8201469/webrev.00/

( For othervm.dirs property, I just reformat it. )


In my test result, with jtreg option "-concurrency:4", after apply the patch, 
tier3 tests on different platforms will be slower about 1.5~2.0 times than before.
I think stability of tests are more important than executing time, how do you 
think about it?

Thank you
-Hamlin

Re: [11] RFR: 8202026 8193552 : ISO 4217 Amendment #165 # 166 Update

2018-04-24 Thread Leo Jiang


Forgot to mention, the tests in Currency fold are passed on Mach5.

-Leo

On 04/25/2018 09:33 AM, Leo Jiang wrote:

Hi,

Please review the changes to address the ISO 4217 Amendment 165 166 update.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8193552  165
https://bugs.openjdk.java.net/browse/JDK-8202026  166

CR:
http://cr.openjdk.java.net/~ljiang/8202026/webrev.00/


Detail:
#165
From:
MAURITANIA    Ouguiya    MRO    478    2
To:
MAURITANIA    Ouguiya    MRU    929    2

#166
From:
VENEZUELA (BOLIVARIAN REPUBLIC OF)    Bolívar    VEF    937    2
To:
VENEZUELA (BOLIVARIAN REPUBLIC OF)    Bolívar Soberano    VES    928    2


Thanks,
Leo

Re: RFR: 8201274: Launch Single-File Source-Code Programs

2018-04-24 Thread mandy chung




On 4/25/18 8:53 AM, Jonathan Gibbons wrote:



On 04/12/2018 10:20 PM, mandy chung wrote:



This looks quite good to me.  One small comment on the source 
launcher Main class:


  122 } catch (InvocationTargetException e) {
  123 // leave VM to handle the stacktrace, in the standard manner
  124 throw e.getTargetException();
  125 }
  387 } catch (InvocationTargetException e) {
  388 // remove stack frames for source launcher
  389 int invocationFrames = e.getStackTrace().length;
  390 Throwable target = e.getTargetException();
  391 StackTraceElement[] targetTrace = target.getStackTrace();
  392 target.setStackTrace(Arrays.copyOfRange(targetTrace, 0, 
targetTrace.length - invocationFrames));
  393 throw e;
  394 }

This could simply throw target instead of the InvocationTargetException
and then the main method can propagate the target, if thrown.

Mandy


Mandy,

Yes, but that would require the execute method and its callers to 
declare that they throw Throwable,
or at least Exception. Since the exception is already wrapped, it 
seems better to propagate the

wrapped exception, and to only unwrap it at the last moment.



Either way works for me.

Mandy

Re: [11] RFR: 8202026 8193552 : ISO 4217 Amendment #165 # 166 Update

2018-04-24 Thread Leo Jiang


+ naoto

On 04/25/2018 09:33 AM, Leo Jiang wrote:

Hi,

Please review the changes to address the ISO 4217 Amendment 165 166 update.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8193552  165
https://bugs.openjdk.java.net/browse/JDK-8202026  166

CR:
http://cr.openjdk.java.net/~ljiang/8202026/webrev.00/


Detail:
#165
From:
MAURITANIA    Ouguiya    MRO    478    2
To:
MAURITANIA    Ouguiya    MRU    929    2

#166
From:
VENEZUELA (BOLIVARIAN REPUBLIC OF)    Bolívar    VEF    937    2
To:
VENEZUELA (BOLIVARIAN REPUBLIC OF)    Bolívar Soberano    VES    928    2


Thanks,
Leo

Re: [PATCH] regex matcher opt: remove redundant StringBuilder

2018-04-24 Thread Xueming Shen


for String based replaceAll/First() it might be worth doing something like

http://cr.openjdk.java.net/~sherman/regex_removesb/webrev/

On 4/24/18, 10:47 AM, Isaac Levy wrote:

Hi Sherman,

Thanks for clarifying. Looks like exceptions are caused by invalid
format string. I wouldn't expect most programs to be catching this and
preserving their buffer, but dunno.

How much does it affect perf? Well it depends on use case, a jmh of
replaceAll with a length 200 string of digits and regex "(\w)" shows
about 30% speedup.

[info] Benchmark  Mode  Cnt   Score   Error  Units
[info] origM  avgt   10  11.669 ± 0.211  us/op
[info] newM   avgt   10   8.926 ± 0.095  us/op

Isaac


On Tue, Apr 24, 2018 at 12:53 PM, Xueming Shen  wrote:

Hi Isaac,

I actually meant to say "we are not supposed to output the partial text into
the output buffer in case of an exception". It has nothing to do with the
changeset you cited. This has been the behavior since day one/JDK1.4,
though it is not specified explicitly in the API doc. The newly added
StringBuilder variant simply follows this behavior. If it's really desired
it
is kinda doable to save that StringBuilder without the "incompatible"
behavior
change but just wonder if it is really worth the effort.

Thanks,
Sherman


On 4/24/18, 9:11 AM, Isaac Levy wrote:

(moving this to a separate discussion)


--- a/src/java.base/share/classes/java/util/regex/Matcher.java
+++ b/src/java.base/share/classes/java/util/regex/Matcher.java
@@ -993,13 +993,11 @@
  public Matcher appendReplacement(StringBuilder sb, String
replacement) {
   // If no match, return error
   if (first<   0)
   throw new IllegalStateException("No match available");
-StringBuilder result = new StringBuilder();
-appendExpandedReplacement(replacement, result);
   // Append the intervening text
   sb.append(text, lastAppendPosition, first);
   // Append the match substitution
+appendExpandedReplacement(replacement, sb);
-sb.append(result);



On Mon, Apr 23, 2018 at 5:05 PM Xueming Shen
wrote:


I would assume in case of an exception thrown from
appendExpandedReplacement() we don't
want "text" to be pushed into the "sb".

-sherman


Perhaps. Though the behavior under exception is undefined and this
function is probably primarily used though the replaceAll API, which
wouldn’t return the intermediate sb under the case of exception.

My reading of the blame was the temp StringBuilder was an artifact of
the api previously using StringBuffer externally.  See
http://hg.openjdk.java.net/jdk9/dev/jdk/rev/763c564451b3

[11] RFR: 8202026 8193552 : ISO 4217 Amendment #165 # 166 Update

2018-04-24 Thread Leo Jiang


Hi,

Please review the changes to address the ISO 4217 Amendment 165 166 update.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8193552  165
https://bugs.openjdk.java.net/browse/JDK-8202026  166

CR:
http://cr.openjdk.java.net/~ljiang/8202026/webrev.00/


Detail:
#165
From:
MAURITANIA  Ouguiya MRO 478 2
To:
MAURITANIA  Ouguiya MRU 929 2

#166
From:
VENEZUELA (BOLIVARIAN REPUBLIC OF)  Bolívar VEF 937 2
To:
VENEZUELA (BOLIVARIAN REPUBLIC OF)  Bolívar SoberanoVES 928 
2


Thanks,
Leo

Re: RFR: 8201274: Launch Single-File Source-Code Programs

2018-04-24 Thread Jonathan Gibbons




On 04/12/2018 10:20 PM, mandy chung wrote:



On 4/13/18 4:15 AM, Jonathan Gibbons wrote:

Please review an initial implementation for the feature described in
JEP 330: Launch Single-File Source-Code Programs.

The work is described in the JEP and CSR, and falls into various parts:

 * The part to handle the new command-line options is in the native
   Java launcher code.
 * The part to invoke the compiler and subsequently execute the code
   found in the source file is in a new class in the jdk.compiler 
module.

 * There are some minor Makefile changes, to add support for a new
   resource file.

There are no changes to javac itself.

JEP: http://openjdk.java.net/jeps/330
JBS: https://bugs.openjdk.java.net/browse/JDK-8201274
CSR: https://bugs.openjdk.java.net/browse/JDK-8201275
Webrev: http://cr.openjdk.java.net/~jjg/8201274/webrev.00/


This looks quite good to me.  One small comment on the source launcher 
Main class:


  122 } catch (InvocationTargetException e) {
  123 // leave VM to handle the stacktrace, in the standard manner
  124 throw e.getTargetException();
  125 }
  387 } catch (InvocationTargetException e) {
  388 // remove stack frames for source launcher
  389 int invocationFrames = e.getStackTrace().length;
  390 Throwable target = e.getTargetException();
  391 StackTraceElement[] targetTrace = target.getStackTrace();
  392 target.setStackTrace(Arrays.copyOfRange(targetTrace, 0, 
targetTrace.length - invocationFrames));
  393 throw e;
  394 }

This could simply throw target instead of the InvocationTargetException
and then the main method can propagate the target, if thrown.

Mandy


Mandy,

Yes, but that would require the execute method and its callers to 
declare that they throw Throwable,
or at least Exception. Since the exception is already wrapped, it seems 
better to propagate the

wrapped exception, and to only unwrap it at the last moment.

-- Jon

Re: JDK 11 RFR of 8200478: For boxing conversion javac uses Long.valueOf which does not guarantee caching according to its javadoc

2018-04-24 Thread Brian Burkhalter

Hi Joe,

On Apr 24, 2018, at 5:30 PM, joe darcy  wrote:

> Please review the patch below to update the specification of 
> Long.valueOf(long) to require caching on [-128, 127]. The JDK implementation 
> of this functionality has always cached in that region, even though it is not 
> required.

Looks fine.

> Additionally, please review the corresponding CSR:
> 
> JDK-8202231: For boxing conversion javac uses Long.valueOf which does 
> not guarantee caching according to its javadoc

Done.

Brian

JDK 11 RFR of 8200478: For boxing conversion javac uses Long.valueOf which does not guarantee caching according to its javadoc

2018-04-24 Thread joe darcy


Hello,

Please review the patch below to update the specification of 
Long.valueOf(long) to require caching on [-128, 127]. The JDK 
implementation of this functionality has always cached in that region, 
even though it is not required.


Additionally, please review the corresponding CSR:

        JDK-8202231: For boxing conversion javac uses Long.valueOf 
which does not guarantee caching according to its javadoc


Thanks,

-Joe

diff -r f909f09569ca src/java.base/share/classes/java/lang/Long.java
--- a/src/java.base/share/classes/java/lang/Long.java    Wed Apr 18 
21:10:09 2018 -0700
+++ b/src/java.base/share/classes/java/lang/Long.java    Tue Apr 24 
17:25:24 2018 -0700

@@ -1164,10 +1164,8 @@
  * significantly better space and time performance by caching
  * frequently requested values.
  *
- * Note that unlike the {@linkplain Integer#valueOf(int)
- * corresponding method} in the {@code Integer} class, this method
- * is not required to cache values within a particular
- * range.
+ * This method will always cache values in the range -128 to 127,
+ * inclusive, and may cache other values outside of this range.
  *
  * @param  l a long value.
  * @return a {@code Long} instance representing {@code l}.

Re: [11] RFR: 8181157: CLDR Timezone name fallback implementation

2018-04-24 Thread Stephen Colebourne

I had a quick look through and didn't see anything obvious, but its
not an area I know well.
Stephen


On 17 April 2018 at 22:28, Naoto Sato  wrote:
> Hello,
>
> Please review the changes to the following issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8181157
>
> The proposed fix is located at:
>
> http://cr.openjdk.java.net/~naoto/8181157/webrev.05/
>
> This RFE is to implement CLDR time zone names fallback mechanism [1]. Prior
> to this fix, time zone names are substituted with English names. With this
> change, it is generated according to the logic defined in TR 35. Here are
> the highlight of the changes:
>
> CLDRConverter:
> - CLDRConverter has changed to not substitute/invent time zone display
> names, except English bundle for compatibility & runtime performance.
> - For English bundle, copy over COMPAT's names as before.
> - CLDRConverer now parses regionFormat/gmtZeroFormat/exemplarCity
>
> CLDR provider:
> - CLDRTimeZoneNameProviderImpl is introduced to generate fallback names
> at runtime.
> - If COMPAT provider is present, use it also as a fallback, before the
> last resort (GMT offset format)
>
> Naoto
>
> [1] http://www.unicode.org/reports/tr35/tr35-dates.html#Time_Zone_Names

Re: RFR: JDK-8193877- DateTimeFormatterBuilder throws ClassCastException when using padding

2018-04-24 Thread Stephen Colebourne

To add to Roger's comments, the tests should cover parsing as well as
formatting. It is the adjacent parsing behaviour that will be most
important to test and get right.

For example,
Pattern "dM" cannot be parsed
(is "1122018" the 1st Dec or the 11th Feb?)

But this one should be parsed "ppdppM"
(" 1122018" has fixed width day = 1 and fixed width month = 12)
("11 22018" has fixed width day =11 and fixed width month = 2)

And as Roger says, this should be tested using
DateTimrFormatterBuilder::padNext as well as the pattern letters.

thanks
Stephen


On 18 April 2018 at 19:34, Roger Riggs  wrote:
> Hi Pallavi,
>
> The fix here seems to make padding to a fixed width incompatible with
> adjacent value parsing.
> That's not intuitive, since adjacent value parsing is intended to allow more
> flexible parsing
> of a combination leading fixed-width fields and subsequent variable length
> fields.
> The specification of the behavior of padding vs adjacent value parsing
> should be investigated
> and clarified if necessary.
>
> The implementation would be better expressed as checking whether the active
> PrinterParser
> *is* a NumberPrinterParser as a precondition for entering the adjacent
> parsing mode block
> (and the necessary cast).
> And otherwise, fall into the existing code in which the new Parser becomes
> the new active parser.
>
> The tests should be included in the existing test classes for padding, and
> be written using
> the direct DateTimeFormatterBuilder methods (padNext(), instead of the
> patterns) since the patterns
> are just a front end for the builder methods.
> See test/java/time/format/TestPadPrinterDecorator.java
>
> TestDateTimeFormatter.java:
>
> line 96: please keep the static imports for testng together
>
> Line 662: The odd formatting and incorrect indentation should no longer be a
> problem
> because the indentation will not need to change.
>
> Regards, Roger
>
>
>
> On 4/18/18 8:41 AM, Pallavi Sonal wrote:
>>
>> Hi,
>>
>> Please review the changes to the following issue:
>>
>> Bug :https://bugs.openjdk.java.net/browse/JDK-8193877
>>
>> The proposed fix is located at:
>>
>> Webrev :http://cr.openjdk.java.net/~rpatil/8193877/webrev.00/
>>
>>
>> When padding is used in a pattern where there are unpadded values adjacent
>> to padded ones (like "pdQ") , the previous PrinterParser (used for parsing
>> 'd' in the example ) is fetched to use its settings for parsing the next
>> value('Q' in the example). But , in cases like this , it is a
>> PadPrinterDecoratorParser instead of an assumed NumberPrinterParser and a
>> ClassCastException is thrown.
>>
>> The fix has been done to check such cases where the previous parserprinter
>> is PadPrinterDecoratorParser and use the new NumberPrinterParser instead for
>> parsing the next value.
>>
>>
>> All the tier1 and tier2 Mach 5 tests have passed.
>>
>>
>> Thanks,
>>
>> Pallavi Sonal
>>
>>
>
>

Re: 8202062: Put FileChannel and FileOutpuStream variants of AtomicAppend on problem list

2018-04-24 Thread Lance Andersen

+1
> On Apr 24, 2018, at 6:15 PM, Brian Burkhalter  
> wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8202062
> 
> Recently more frequent failures have been observed on macOS. Put on the 
> problem list until the underlying cause can be determined.
> 
> Thanks,
> 
> Brian
> 
> --- a/test/jdk/ProblemList.txt
> +++ b/test/jdk/ProblemList.txt
> @@ -510,8 +510,10 @@
> 
> # jdk_io
> 
> +java/io/FileOutputStream/AtomicAppend.java  8202062 
> macosx-all
> java/io/pathNames/GeneralWin32.java 8180264 
> windows-all
> 
> +
> 
> 
> # jdk_management
> @@ -545,10 +547,12 @@
> 
> java/nio/Buffer/EqualsCompareTest.java  8193917 
> solaris-all
> 
> +java/nio/channels/DatagramChannel/ChangingAddress.java  7141822 
> macosx-all
> +
> +java/nio/channels/FileChannel/AtomicAppend.java 8202062 
> macosx-all
> +
> java/nio/channels/Selector/Wakeup.java  6963118 
> windows-all
> 
> -java/nio/channels/DatagramChannel/ChangingAddress.java  7141822 
> macosx-all
> -
> java/nio/file/WatchService/Basic.java   7158947 
> solaris-all Solaris 11
> java/nio/file/WatchService/MayFlies.java7158947 
> solaris-all Solaris 11
> java/nio/file/WatchService/LotsOfCancels.java   8188039 
> solaris-all Solaris 11
> 

 
  

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

8202062: Put FileChannel and FileOutpuStream variants of AtomicAppend on problem list

2018-04-24 Thread Brian Burkhalter

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

Recently more frequent failures have been observed on macOS. Put on the problem 
list until the underlying cause can be determined.

Thanks,

Brian

--- a/test/jdk/ProblemList.txt
+++ b/test/jdk/ProblemList.txt
@@ -510,8 +510,10 @@
 
 # jdk_io
 
+java/io/FileOutputStream/AtomicAppend.java  8202062 
macosx-all
 java/io/pathNames/GeneralWin32.java 8180264 
windows-all
 
+
 
 
 # jdk_management
@@ -545,10 +547,12 @@
 
 java/nio/Buffer/EqualsCompareTest.java  8193917 
solaris-all
 
+java/nio/channels/DatagramChannel/ChangingAddress.java  7141822 
macosx-all
+
+java/nio/channels/FileChannel/AtomicAppend.java 8202062 
macosx-all
+
 java/nio/channels/Selector/Wakeup.java  6963118 
windows-all
 
-java/nio/channels/DatagramChannel/ChangingAddress.java  7141822 
macosx-all
-
 java/nio/file/WatchService/Basic.java   7158947 
solaris-all Solaris 11
 java/nio/file/WatchService/MayFlies.java7158947 
solaris-all Solaris 11
 java/nio/file/WatchService/LotsOfCancels.java   8188039 
solaris-all Solaris 11

Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData

2018-04-24 Thread Paul Sandoz

Hi,

This looks good. Took me a while to understand the interactions: you need to 
replace not update otherwise there is a race on isResolved (which currently 
queries multiple state, there is no singular guard here). We could push 
isResolved into the synchronized block and simplify but every findSpecies call 
may take a small hit (or there are potentially other ways looking more closely 
at how ClassSpecializer.Factory initializes state, i.e. a fenced static field 
write, but we go further down the rabbit hole) 

I think this might fix some rare and intermittent recursive exceptions from 
ConcurrentHashMap cache we have been observing, where a collision occurs on 
keys for recursive updates (rather than for the same key).

Paul.

> On Apr 24, 2018, at 9:57 AM, Claes Redestad  wrote:
> 
> Hi,
> 
> the current implementation of ClassSpecializer.findSpecies may cause
> excessive blocking due to a potentially expensive computeIfAbsent, and
> we have reason to believe this might have been cause for a few very
> rare bootstrap issues in tests that attach debuggers to VM in the middle
> of this.
> 
> Breaking this apart so that code generation and linking is done outside
> of the computeIfAbsent helps minimize the risk that a thread gets
> interrupted by a debugger at a critical point in time, while also removing
> a few limitations on what we can do from a findSpecies, e.g., look up other
> SpeciesData.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8202184
> Webrev: http://cr.openjdk.java.net/~redestad/8202184/open.00/
> 
> This implementation inserts a placeholder, unresolved SpeciesData,
> and then replaces it with another instance that is linked together
> fully before publishing it, which ensure safe publication. There might
> be alternatives involving volatiles and/or careful fencing, but I've not
> been able to measure any added startup cost from this anyway.
> 
> Thanks!
> 
> /Claes

Re: [PATCH] regex matcher opt: remove redundant StringBuilder

2018-04-24 Thread Isaac Levy

Hi Sherman,

Thanks for clarifying. Looks like exceptions are caused by invalid
format string. I wouldn't expect most programs to be catching this and
preserving their buffer, but dunno.

How much does it affect perf? Well it depends on use case, a jmh of
replaceAll with a length 200 string of digits and regex "(\w)" shows
about 30% speedup.

[info] Benchmark  Mode  Cnt   Score   Error  Units
[info] origM  avgt   10  11.669 ± 0.211  us/op
[info] newM   avgt   10   8.926 ± 0.095  us/op

Isaac


On Tue, Apr 24, 2018 at 12:53 PM, Xueming Shen  wrote:
> Hi Isaac,
>
> I actually meant to say "we are not supposed to output the partial text into
> the output buffer in case of an exception". It has nothing to do with the
> changeset you cited. This has been the behavior since day one/JDK1.4,
> though it is not specified explicitly in the API doc. The newly added
> StringBuilder variant simply follows this behavior. If it's really desired
> it
> is kinda doable to save that StringBuilder without the "incompatible"
> behavior
> change but just wonder if it is really worth the effort.
>
> Thanks,
> Sherman
>
>
> On 4/24/18, 9:11 AM, Isaac Levy wrote:
>>
>> (moving this to a separate discussion)
>>
>>
>> --- a/src/java.base/share/classes/java/util/regex/Matcher.java
>> +++ b/src/java.base/share/classes/java/util/regex/Matcher.java
>> @@ -993,13 +993,11 @@
>>  public Matcher appendReplacement(StringBuilder sb, String
>> replacement) {
>>   // If no match, return error
>>   if (first<  0)
>>   throw new IllegalStateException("No match available");
>> -StringBuilder result = new StringBuilder();
>> -appendExpandedReplacement(replacement, result);
>>   // Append the intervening text
>>   sb.append(text, lastAppendPosition, first);
>>   // Append the match substitution
>> +appendExpandedReplacement(replacement, sb);
>> -sb.append(result);
>>
>>
>>
>> On Mon, Apr 23, 2018 at 5:05 PM Xueming Shen
>> wrote:
>>>
>>>
>>> I would assume in case of an exception thrown from
>>> appendExpandedReplacement() we don't
>>> want "text" to be pushed into the "sb".
>>>
>>> -sherman
>>
>>
>> Perhaps. Though the behavior under exception is undefined and this
>> function is probably primarily used though the replaceAll API, which
>> wouldn’t return the intermediate sb under the case of exception.
>>
>> My reading of the blame was the temp StringBuilder was an artifact of
>> the api previously using StringBuffer externally.  See
>> http://hg.openjdk.java.net/jdk9/dev/jdk/rev/763c564451b3
>
>

Re: [PATCH] Add compareToUnsigned to java.nio.*Buffer

2018-04-24 Thread Paul Sandoz

Thanks. I attached your patch to the issue:

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



> On Apr 24, 2018, at 5:51 AM, Robert Stupp  wrote:
> 
> Hi Paul,
> 
> Thank you.
> 
> I've filed an RFE via https://bugreport.java.com/bugreport/start_form.do - 
> seems to be JDK-9053522. It just has no option to add an attachment and I 
> have no login to bugs.openjdk.java.net .
> 
> 
> On 04/23/2018 11:16 PM, Paul Sandoz wrote:
>> The addition of mismatch is reasonable. I am unsure about compareToUnsigned, 
>> since we are trying to be a conservative about what we add to Buffer.
> On the one hand, it would be very nice to have compareToUnsigned() since the 
> primitive wrapper classes have compareToUnsigned() as well. On the other 
> hand, it would not be a big issue to build a similar functionality using 
> *Buffer.mismatch().
> 

Exactly.

Paul.

RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData

2018-04-24 Thread Claes Redestad


Hi,

the current implementation of ClassSpecializer.findSpecies may cause
excessive blocking due to a potentially expensive computeIfAbsent, and
we have reason to believe this might have been cause for a few very
rare bootstrap issues in tests that attach debuggers to VM in the middle
of this.

Breaking this apart so that code generation and linking is done outside
of the computeIfAbsent helps minimize the risk that a thread gets
interrupted by a debugger at a critical point in time, while also removing
a few limitations on what we can do from a findSpecies, e.g., look up other
SpeciesData.

Bug: https://bugs.openjdk.java.net/browse/JDK-8202184
Webrev: http://cr.openjdk.java.net/~redestad/8202184/open.00/

This implementation inserts a placeholder, unresolved SpeciesData,
and then replaces it with another instance that is linked together
fully before publishing it, which ensure safe publication. There might
be alternatives involving volatiles and/or careful fencing, but I've not
been able to measure any added startup cost from this anyway.

Thanks!

/Claes

Re: [PATCH] regex matcher opt: remove redundant StringBuilder

2018-04-24 Thread Xueming Shen


Hi Isaac,

I actually meant to say "we are not supposed to output the partial text into
the output buffer in case of an exception". It has nothing to do with the
changeset you cited. This has been the behavior since day one/JDK1.4,
though it is not specified explicitly in the API doc. The newly added
StringBuilder variant simply follows this behavior. If it's really 
desired it
is kinda doable to save that StringBuilder without the "incompatible" 
behavior

change but just wonder if it is really worth the effort.

Thanks,
Sherman

On 4/24/18, 9:11 AM, Isaac Levy wrote:

(moving this to a separate discussion)


--- a/src/java.base/share/classes/java/util/regex/Matcher.java
+++ b/src/java.base/share/classes/java/util/regex/Matcher.java
@@ -993,13 +993,11 @@
 public Matcher appendReplacement(StringBuilder sb, String replacement) {
  // If no match, return error
  if (first<  0)
  throw new IllegalStateException("No match available");
-StringBuilder result = new StringBuilder();
-appendExpandedReplacement(replacement, result);
  // Append the intervening text
  sb.append(text, lastAppendPosition, first);
  // Append the match substitution
+appendExpandedReplacement(replacement, sb);
-sb.append(result);



On Mon, Apr 23, 2018 at 5:05 PM Xueming Shen  wrote:


I would assume in case of an exception thrown from appendExpandedReplacement() 
we don't
want "text" to be pushed into the "sb".

-sherman


Perhaps. Though the behavior under exception is undefined and this
function is probably primarily used though the replaceAll API, which
wouldn’t return the intermediate sb under the case of exception.

My reading of the blame was the temp StringBuilder was an artifact of
the api previously using StringBuffer externally.  See
http://hg.openjdk.java.net/jdk9/dev/jdk/rev/763c564451b3

RE: RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.

2018-04-24 Thread Lindenmaier, Goetz

Hi Simon,

Because as stated here, 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-April/052665.html
it is used in other places like that, too.

Later mails agreed on that usage to keep it consistent.

Best regards,
  Goetz.

> -Original Message-
> From: Simon Nash [mailto:si...@cjnash.com]
> Sent: Dienstag, 24. April 2018 18:17
> To: Lindenmaier, Goetz 
> Cc: David Holmes ; hotspot-runtime-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; aarch64-
> port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; core-libs-
> dev Libs 
> Subject: Re: RFR(M): 8201593: Print array length in
> ArrayIndexOutOfBoundsException.
> 
> On 24/04/2018 15:08, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > I implemented what we figured out in
> > http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-
> April/027555.html
> >
> > Some examples:
> > "Index 12 out-of-bounds for length 10."
> > "arraycopy source index -17 out of bounds for object array[10]."
> > "arraycopy destination index -18 out of bounds for double[5]."
> > "arraycopy length -19 is negative."
> > "arraycopy: last source index 13 out of bounds for double[10]."
> > "arraycopy: last destination index 7 out of bounds for long[5]."
> >
> Is there a reason why the first message says "out-of-bounds" but all others
> say "out of bounds"?
> 
> Simon
> 
> > Incremental webrev:
> > http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/03-
> incremental/
> > Full webrev:
> > http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/03/
> >
> > I'll push it through our tests tonight.
> >
> > See further comments in line:
> >
> >> -Original Message-
> >> From: David Holmes [mailto:david.hol...@oracle.com]
> >> Sent: Freitag, 20. April 2018 09:25
> >> To: Lindenmaier, Goetz ; hotspot-runtime-
> >> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
> aarch64-
> >> port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; core-
> libs-
> >> dev Libs 
> >> Subject: Re: RFR(M): 8201593: Print array length in
> >> ArrayIndexOutOfBoundsException.
> >>
> >> Hi Goetz,
> >>
> >> This is not a file by file review ...
> >>
> >> On 19/04/2018 10:24 PM, Lindenmaier, Goetz wrote:
> >>> Hi,
> >>>
> >>> New webrev:
> >>> http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/02/
> >>>
> >>> I admit my wording is not optimal.
> >> src/hotspot/share/oops/typeArrayKlass.cpp
> >>
> >> Sorry but this is still far too verbose for my tastes. The type of the
> >> array is not relevant. For the array copy its okay to indicate src or
> >> dst array. But the message should be clear and succinct not prose-like.
> >> Plus you have a lot of repetition in the ss.print statements when only
> >> one thing is changing.
> > We discussed this in some further mails. Implemented as proposed there.
> >
> >> src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp
> >>
> >> I'm not seeing why the throw_index_out_of_bounds_exception should
> be
> >> tied to whether or not you have an array reference. Certainly I hate
> >> seeing the array ref being used as an implicit bool!
> > I split the constructor into two, one for ArrayIndexOutOfBounds, the other
> > for IndexOutOfBounds.
> >
> > ...
> >
> >>> I extended the test to cover the exception thrown in arraycopy better
> >> The test seems somewhat excessive, and I now see it is because of the
> >> more elaborate error messages you have at SAP. But with only the index
> >> and the array length of interest here the test can be considerably smaller.
> >>
> >> The creation tests for ArrayIndexOutOfBoundsException don't seem
> >> relevant in this context either. This looks more TCK like.
> > Yes, the constructor tests are for the code not yet contributed.
> > I simplified the tests to only check the messages.
> >
> > Best regards,
> >   Goetz.
> >
> >
> >
> >
> >
> >> David
> >> -
> >>
> >>> and added the elementary type to the message text.  This probably
> >>> needs improvement in the text, too.  There are (currently) these cases:
> >>>
> >>> bject[]  oa1 = new Object[10];
> >>> Object[]  oa2 = new Object[5];
> >>> System.arraycopy(oa1, -17, oa2, 0, 5);
> >>> "while trying to copy from index -17 of an object array with length 10");
> >>> System.arraycopy(oa1, 2, oa2, -18, 5);
> >>> "while trying to copy to index -18 of an object array with length 5");
> >>> System.arraycopy(oa1, 2, oa2, 0, -19);
> >>> "while trying to copy a negative range -19 from an object array with
> length
> >> 10 to an object array with length 5");
> >>> System.arraycopy(oa1, 8, oa2, 0, 5);
> >>> "while trying to copy from index 13 of an object array with length 10");
> >>> System.arraycopy(oa1, 1, oa2, 0, 7);
> >>> "while trying to copy to index 7 of an object array with length 5");
> >>> double[]  ta1 = new double[10];
> >>> double[]  ta2 = new double[5];
> >>> System.arraycopy(ta1, -17, ta2,

[PATCH] regex matcher opt: remove redundant StringBuilder

2018-04-24 Thread Isaac Levy

(moving this to a separate discussion)

--- a/src/java.base/share/classes/java/util/regex/Matcher.java
+++ b/src/java.base/share/classes/java/util/regex/Matcher.java
@@ -993,13 +993,11 @@
public Matcher appendReplacement(StringBuilder sb, String replacement) {
 // If no match, return error
 if (first < 0)
 throw new IllegalStateException("No match available");
-StringBuilder result = new StringBuilder();
-appendExpandedReplacement(replacement, result);
 // Append the intervening text
 sb.append(text, lastAppendPosition, first);
 // Append the match substitution
+appendExpandedReplacement(replacement, sb);
-sb.append(result);

On Mon, Apr 23, 2018 at 5:05 PM Xueming Shen  wrote:
>
>
> I would assume in case of an exception thrown from 
> appendExpandedReplacement() we don't
> want "text" to be pushed into the "sb".
>
> -sherman

Perhaps. Though the behavior under exception is undefined and this
function is probably primarily used though the replaceAll API, which
wouldn’t return the intermediate sb under the case of exception.

My reading of the blame was the temp StringBuilder was an artifact of
the api previously using StringBuffer externally.  See
http://hg.openjdk.java.net/jdk9/dev/jdk/rev/763c564451b3

RE: RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.

2018-04-24 Thread Lindenmaier, Goetz

Hi, 

I implemented what we figured out in 
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-April/027555.html

Some examples:
"Index 12 out-of-bounds for length 10."
"arraycopy source index -17 out of bounds for object array[10]."
"arraycopy destination index -18 out of bounds for double[5]."
"arraycopy length -19 is negative."
"arraycopy: last source index 13 out of bounds for double[10]."
"arraycopy: last destination index 7 out of bounds for long[5]."

Incremental webrev:
http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/03-incremental/
Full webrev:
http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/03/

I'll push it through our tests tonight.

See further comments in line:

> -Original Message-
> From: David Holmes [mailto:david.hol...@oracle.com]
> Sent: Freitag, 20. April 2018 09:25
> To: Lindenmaier, Goetz ; hotspot-runtime-
> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; aarch64-
> port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; core-libs-
> dev Libs 
> Subject: Re: RFR(M): 8201593: Print array length in
> ArrayIndexOutOfBoundsException.
> 
> Hi Goetz,
> 
> This is not a file by file review ...
> 
> On 19/04/2018 10:24 PM, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > New webrev:
> > http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/02/
> >
> > I admit my wording is not optimal.
> 
> src/hotspot/share/oops/typeArrayKlass.cpp
> 
> Sorry but this is still far too verbose for my tastes. The type of the
> array is not relevant. For the array copy its okay to indicate src or
> dst array. But the message should be clear and succinct not prose-like.
> Plus you have a lot of repetition in the ss.print statements when only
> one thing is changing.
We discussed this in some further mails. Implemented as proposed there.

> src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp
> 
> I'm not seeing why the throw_index_out_of_bounds_exception should be
> tied to whether or not you have an array reference. Certainly I hate
> seeing the array ref being used as an implicit bool!
I split the constructor into two, one for ArrayIndexOutOfBounds, the other
for IndexOutOfBounds.

...

> > I extended the test to cover the exception thrown in arraycopy better
> 
> The test seems somewhat excessive, and I now see it is because of the
> more elaborate error messages you have at SAP. But with only the index
> and the array length of interest here the test can be considerably smaller.
> 
> The creation tests for ArrayIndexOutOfBoundsException don't seem
> relevant in this context either. This looks more TCK like.
Yes, the constructor tests are for the code not yet contributed.
I simplified the tests to only check the messages.

Best regards,
  Goetz.





> 
> David
> -
> 
> > and added the elementary type to the message text.  This probably
> > needs improvement in the text, too.  There are (currently) these cases:
> >
> > bject[]  oa1 = new Object[10];
> > Object[]  oa2 = new Object[5];
> > System.arraycopy(oa1, -17, oa2, 0, 5);
> > "while trying to copy from index -17 of an object array with length 10");
> > System.arraycopy(oa1, 2, oa2, -18, 5);
> > "while trying to copy to index -18 of an object array with length 5");
> > System.arraycopy(oa1, 2, oa2, 0, -19);
> > "while trying to copy a negative range -19 from an object array with length
> 10 to an object array with length 5");
> > System.arraycopy(oa1, 8, oa2, 0, 5);
> > "while trying to copy from index 13 of an object array with length 10");
> > System.arraycopy(oa1, 1, oa2, 0, 7);
> > "while trying to copy to index 7 of an object array with length 5");
> > double[]  ta1 = new double[10];
> > double[]  ta2 = new double[5];
> > System.arraycopy(ta1, -17, ta2, 0, 5);
> > "while trying to copy from index -17 of a doubl array with length 10");
> > System.arraycopy(ta1, 2, ta2, -18, 5);
> > "while trying to copy to index -18 of a double array with length 5");
> > System.arraycopy(ta1, 2, ta2, 0, -19);
> > "while trying to copy a negative range -19 from a double array with length
> 10 to a double array with length 5");
> > System.arraycopy(ta1, 8, ta2, 0, 5);
> > "while trying to copy from index 13 of a double array with length 10");
> > System.arraycopy(ta1, 1, ta2, 0, 7);
> > "while trying to copy to index 7 of a double array with length 5");
> >
> > Maybe it should say:
> > Arraycopy source index -1 out-of-bounds for double array of length 10.
> > Arraycopy target index 10 out-of-bounds for object array of length 10.
> > Negative range -19 when copying from an object array of length 10 to an
> object array of length 5.
> >
> > Best regards,
> >Goetz.
> >
> >> -Original Message-
> >> From: David Holmes [mailto:david.hol...@oracle.com]
> >> Sent: Mittwoch, 18. April 2018 10:55
> >> To: Lindenmaier, Goetz ; hotspot-runtime-
> >> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
> aarch64-
> >>

Re: [PATCH] minor regex cleanup: use switch for enum

2018-04-24 Thread Remi Forax

And as part of Amber, we are likely change the bytecode translation of the 
switch on enums (in fact all switches apart the one on integers) to avoid the 
separate compilation issues you mention (and support more kind of switches). 
The idea is that the new translation is to use invokedynamic so the association 
between an enum and the corresponding case is done at runtime and not at 
compile time.

I think there is already codes in the amber repository that deals with enums, 
so another way to optimize the switch on enum is to change the bootstrap method 
associated to the switch on enum (we may also have to introduce a new method 
handle combinator that works like the getter but with the @Stable semantics).

Rémi 

- Mail original -
> De: "Peter Levart" 
> À: "David Lloyd" , "Xueming Shen" 
> 
> Cc: "core-libs-dev" 
> Envoyé: Mardi 24 Avril 2018 14:17:21
> Objet: Re: [PATCH] minor regex cleanup: use switch for enum

> On 04/24/2018 12:41 PM, Peter Levart wrote:
>> public class Code {l
>>
>>     static class m$switch$1 {
>>         static final int[] caseindex = new int[X.values().length];
>>     static {
>>             caseindex[X.A.ordinal()] = 1;
>>             caseindex[X.B.ordinal()] = 2;
>>             caseindex[X.C.ordinal()] = 3;
>>         }
>>     }
>>
>>     public void m(X x) {
>>         switch (m$switch$1.caseindex[x.ordinal()]) {
>>             case 1: // ... branch A
>>                 break;
>>             case 2: // ... branch B
>>                 break;
>>             case 3: // ... branch C
>>                 break;
>>             default: // ... default branch        }
>>     }
>> }
>>
>>
>> Pefrormance-wise this is similar to switch on int. So pretty optimal.
>> Decision should only be made on the count of code clarity here.
>>
>> While speaking of performance of enum switches, I have a question for
>> a more knowledgeable person...
>>
>> Should JIT be able to fold above 'x' variable/parameter into a
>> constant (suppose m() was called in a loop with an explicitly
>> specified constant value such as X.A), it could further expand this
>> decision to the x.ordinal() value (if the final instance 'ordinal'
>> field in the X enum class was marked with @Stable), it could then
>> further expand this decision to the looked up value of the
>> m$switch$1.caseindex[] slot if caseindex array field in sytnhetic
>> class was marked with @Stable, therefore transforming the switch to a
>> switch on int constant, optimizing the JITed code to directly execute
>> branch A and eliminate all other branches from generated code.
>>
>> The question is whether JIT is presently treating those fields (and
>> array) as @Stable or not?
> 
> It seems not. Here's a benchmark:
> 
> http://cr.openjdk.java.net/~plevart/misc/SwitchBench/SwitchBench.java
> 
> 
> See enumConstSwitch between "Original JDK 10" and "JDK 10 + @Stable
> Enum.ordinal field".
> 
> Interesting thing is that JDK 9 seems to be better at JITing code than
> JDK 10 in these tests. Why is that?
> 
> I included results for Graal JIT compiler which shows that it probably
> does not contain support for @Stable annotation.
> 
> 
> The relevant part for this "minor regex cleanup" patch is the following:
> 
> SwitchBench.enumConstIf  avgt   10  2.125 ± 0.002  ns/op
> SwitchBench.enumConstSwitch  avgt   10  2.501 ± 0.025 ns/op
> SwitchBench.enumVarIf    avgt   10  2.593 ± 0.004 ns/op
> SwitchBench.enumVarSwitch    avgt   10  2.862 ± 0.053 ns/op
> 
> which indicates that "switch" is a little slower, but with @Stable
> Enum.ordinal field, it is on par with "if" at least for constant folded
> switched on values:
> 
> SwitchBench.enumConstIf  avgt   10  2.126 ± 0.007 ns/op
> SwitchBench.enumConstSwitch  avgt   10  2.159 ± 0.003 ns/op
> SwitchBench.enumVarIf    avgt   10  2.593 ± 0.004 ns/op
> SwitchBench.enumVarSwitch    avgt   10  2.842 ± 0.006 ns/op
> 
> 
> Regards, Peter

Re: [PATCH] Add compareToUnsigned to java.nio.*Buffer

2018-04-24 Thread Robert Stupp


Hi Paul,

Thank you.

I've filed an RFE via https://bugreport.java.com/bugreport/start_form.do 
- seems to be JDK-9053522. It just has no option to add an attachment 
and I have no login to bugs.openjdk.java.net .



On 04/23/2018 11:16 PM, Paul Sandoz wrote:

The addition of mismatch is reasonable. I am unsure about compareToUnsigned, 
since we are trying to be a conservative about what we add to Buffer.
On the one hand, it would be very nice to have compareToUnsigned() since 
the primitive wrapper classes have compareToUnsigned() as well. On the 
other hand, it would not be a big issue to build a similar functionality 
using *Buffer.mismatch().


Robert


--
Robert Stupp
@snazy

Re: [PATCH] minor regex cleanup: use switch for enum

2018-04-24 Thread Remi Forax

Hi Peter,
instead of using @Stable, John Rose had an interesting idea, we talk about a 
similar issue at last FOSDEM, make all final fields in java.lang trusted by 
default. 
The idea is that classes in java.lang doesn't seems to use Unsafe when 
deserializing (like classes in java.util does), so if this is true, all fields 
of java.lang should be trusted by the VM.

regards,
Rémi

- Mail original -
> De: "Peter Levart" 
> À: "David Lloyd" , "Xueming Shen" 
> 
> Cc: "core-libs-dev" 
> Envoyé: Mardi 24 Avril 2018 14:17:21
> Objet: Re: [PATCH] minor regex cleanup: use switch for enum

> On 04/24/2018 12:41 PM, Peter Levart wrote:
>> public class Code {l
>>
>>     static class m$switch$1 {
>>         static final int[] caseindex = new int[X.values().length];
>>     static {
>>             caseindex[X.A.ordinal()] = 1;
>>             caseindex[X.B.ordinal()] = 2;
>>             caseindex[X.C.ordinal()] = 3;
>>         }
>>     }
>>
>>     public void m(X x) {
>>         switch (m$switch$1.caseindex[x.ordinal()]) {
>>             case 1: // ... branch A
>>                 break;
>>             case 2: // ... branch B
>>                 break;
>>             case 3: // ... branch C
>>                 break;
>>             default: // ... default branch        }
>>     }
>> }
>>
>>
>> Pefrormance-wise this is similar to switch on int. So pretty optimal.
>> Decision should only be made on the count of code clarity here.
>>
>> While speaking of performance of enum switches, I have a question for
>> a more knowledgeable person...
>>
>> Should JIT be able to fold above 'x' variable/parameter into a
>> constant (suppose m() was called in a loop with an explicitly
>> specified constant value such as X.A), it could further expand this
>> decision to the x.ordinal() value (if the final instance 'ordinal'
>> field in the X enum class was marked with @Stable), it could then
>> further expand this decision to the looked up value of the
>> m$switch$1.caseindex[] slot if caseindex array field in sytnhetic
>> class was marked with @Stable, therefore transforming the switch to a
>> switch on int constant, optimizing the JITed code to directly execute
>> branch A and eliminate all other branches from generated code.
>>
>> The question is whether JIT is presently treating those fields (and
>> array) as @Stable or not?
> 
> It seems not. Here's a benchmark:
> 
> http://cr.openjdk.java.net/~plevart/misc/SwitchBench/SwitchBench.java
> 
> 
> See enumConstSwitch between "Original JDK 10" and "JDK 10 + @Stable
> Enum.ordinal field".
> 
> Interesting thing is that JDK 9 seems to be better at JITing code than
> JDK 10 in these tests. Why is that?
> 
> I included results for Graal JIT compiler which shows that it probably
> does not contain support for @Stable annotation.
> 
> 
> The relevant part for this "minor regex cleanup" patch is the following:
> 
> SwitchBench.enumConstIf  avgt   10  2.125 ± 0.002  ns/op
> SwitchBench.enumConstSwitch  avgt   10  2.501 ± 0.025 ns/op
> SwitchBench.enumVarIf    avgt   10  2.593 ± 0.004 ns/op
> SwitchBench.enumVarSwitch    avgt   10  2.862 ± 0.053 ns/op
> 
> which indicates that "switch" is a little slower, but with @Stable
> Enum.ordinal field, it is on par with "if" at least for constant folded
> switched on values:
> 
> SwitchBench.enumConstIf  avgt   10  2.126 ± 0.007 ns/op
> SwitchBench.enumConstSwitch  avgt   10  2.159 ± 0.003 ns/op
> SwitchBench.enumVarIf    avgt   10  2.593 ± 0.004 ns/op
> SwitchBench.enumVarSwitch    avgt   10  2.842 ± 0.006 ns/op
> 
> 
> Regards, Peter

Re: [PATCH] minor regex cleanup: use switch for enum

2018-04-24 Thread David Lloyd

On Mon, Apr 23, 2018 at 7:42 PM, Isaac Levy  wrote:
> On Mon, Apr 23, 2018 at 5:18 PM David Lloyd  wrote:
>>
>> FWIW I strongly doubt this will improve performance; probably the
>> opposite in fact, as IIRC an enum switch generates an extra class
>> (though perhaps this has changed).  The original code was quite
>> compact and utilized identity comparisons, and given there are only
>> three alternatives it probably was able to exploit branch prediction
>> as well (if such a thing even matters in this context).
>
>
> Well, there are enum switches on the same enum elsewhere in the class, so
> should those instead be replaced by if checks?

I think that any change that's made for performance should be tested
against performance regression, generally speaking.  My personal
understanding is that, when there are a small number of alternatives,
an identity "if" tree can perform slightly better in some cases
because HotSpot C2 gathers and utilizes statistics about branches
taken in these cases; for switch, it (still IIRC) does not do so.
Given that this is regex, it might be worth testing.

> A larger change could remove this branch entirely, with different classes for 
> each of the types, which
> are known during compile.

If that is beneficial.  However every new class adds metaspace usage
and (in this case) some kind of polymorphic dispatch, so you'd want to
be fairly confident that in a typical application, only one of the two
would be loaded, or that there would be some other significant gain,
as there is definitely a cost.  Everything has a cost, especially in
hot perf-sensitive code.

Anyway I'm not a reviewer but these are considerations that I would
have were I contributing the change.  JMH might be of use.

-- 
- DML

Re: [PATCH] minor regex cleanup: use switch for enum

2018-04-24 Thread Peter Levart




On 04/24/2018 12:41 PM, Peter Levart wrote:

public class Code {l

    static class m$switch$1 {
        static final int[] caseindex = new int[X.values().length];
    static {
            caseindex[X.A.ordinal()] = 1;
            caseindex[X.B.ordinal()] = 2;
            caseindex[X.C.ordinal()] = 3;
        }
    }

    public void m(X x) {
        switch (m$switch$1.caseindex[x.ordinal()]) {
            case 1: // ... branch A
                break;
            case 2: // ... branch B
                break;
            case 3: // ... branch C
                break;
            default: // ... default branch        }
    }
}


Pefrormance-wise this is similar to switch on int. So pretty optimal. 
Decision should only be made on the count of code clarity here.


While speaking of performance of enum switches, I have a question for 
a more knowledgeable person...


Should JIT be able to fold above 'x' variable/parameter into a 
constant (suppose m() was called in a loop with an explicitly 
specified constant value such as X.A), it could further expand this 
decision to the x.ordinal() value (if the final instance 'ordinal' 
field in the X enum class was marked with @Stable), it could then 
further expand this decision to the looked up value of the 
m$switch$1.caseindex[] slot if caseindex array field in sytnhetic 
class was marked with @Stable, therefore transforming the switch to a 
switch on int constant, optimizing the JITed code to directly execute 
branch A and eliminate all other branches from generated code.


The question is whether JIT is presently treating those fields (and 
array) as @Stable or not? 


It seems not. Here's a benchmark:

http://cr.openjdk.java.net/~plevart/misc/SwitchBench/SwitchBench.java


See enumConstSwitch between "Original JDK 10" and "JDK 10 + @Stable 
Enum.ordinal field".


Interesting thing is that JDK 9 seems to be better at JITing code than 
JDK 10 in these tests. Why is that?


I included results for Graal JIT compiler which shows that it probably 
does not contain support for @Stable annotation.



The relevant part for this "minor regex cleanup" patch is the following:

SwitchBench.enumConstIf  avgt   10  2.125 ± 0.002  ns/op
SwitchBench.enumConstSwitch  avgt   10  2.501 ± 0.025 ns/op
SwitchBench.enumVarIf    avgt   10  2.593 ± 0.004 ns/op
SwitchBench.enumVarSwitch    avgt   10  2.862 ± 0.053 ns/op

which indicates that "switch" is a little slower, but with @Stable 
Enum.ordinal field, it is on par with "if" at least for constant folded 
switched on values:


SwitchBench.enumConstIf  avgt   10  2.126 ± 0.007 ns/op
SwitchBench.enumConstSwitch  avgt   10  2.159 ± 0.003 ns/op
SwitchBench.enumVarIf    avgt   10  2.593 ± 0.004 ns/op
SwitchBench.enumVarSwitch    avgt   10  2.842 ± 0.006 ns/op


Regards, Peter

Re: [PATCH] minor regex cleanup: use switch for enum

2018-04-24 Thread Peter Levart




On 04/23/2018 11:18 PM, David Lloyd wrote:

FWIW I strongly doubt this will improve performance; probably the
opposite in fact, as IIRC an enum switch generates an extra class
(though perhaps this has changed).


switch on enum is basically a switch on Enum's ordinal mapped via the 
int[] array constructed in a synthetic nested class of the class 
containing the switch statement, so that separate compilation of an Enum 
class that changes ordinal values of particular enum constants doesn't 
change the semantics of the switch logic. For example...


// X.java
public enum X {
    A, B, C
}

// Code.java
public class Code {
    public void m(X x) {
        switch (x) {
            case A: // ... branch A
                break;
            case B: // ... branch B
                break;
            case C: // ... branch C
                break;
            default: // ... default branch
        }
    }
}


...Code.java translates to something like the following (modulo error 
handling)...


public class Code {l

    static class m$switch$1 {
        static final int[] caseindex = new int[X.values().length];
    static {
            caseindex[X.A.ordinal()] = 1;
            caseindex[X.B.ordinal()] = 2;
            caseindex[X.C.ordinal()] = 3;
        }
    }

    public void m(X x) {
        switch (m$switch$1.caseindex[x.ordinal()]) {
            case 1: // ... branch A
                break;
            case 2: // ... branch B
                break;
            case 3: // ... branch C
                break;
            default: // ... default branch        }
    }
}


Pefrormance-wise this is similar to switch on int. So pretty optimal. 
Decision should only be made on the count of code clarity here.


While speaking of performance of enum switches, I have a question for a 
more knowledgeable person...


Should JIT be able to fold above 'x' variable/parameter into a constant 
(suppose m() was called in a loop with an explicitly specified constant 
value such as X.A), it could further expand this decision to the 
x.ordinal() value (if the final instance 'ordinal' field in the X enum 
class was marked with @Stable), it could then further expand this 
decision to the looked up value of the m$switch$1.caseindex[] slot if 
caseindex array field in sytnhetic class was marked with @Stable, 
therefore transforming the switch to a switch on int constant, 
optimizing the JITed code to directly execute branch A and eliminate all 
other branches from generated code.


The question is whether JIT is presently treating those fields (and 
array) as @Stable or not?


Regards, Peter


  The original code was quite
compact and utilized identity comparisons, and given there are only
three alternatives it probably was able to exploit branch prediction
as well (if such a thing even matters in this context).

I'm not a reviewer but this change seems pointless without supporting perf data.

On Mon, Apr 23, 2018 at 4:05 PM, Xueming Shen  wrote:

I would assume in case of an exception thrown from
appendExpandedReplacement() we don't
want "text" to be pushed into the "sb".

-sherman

On 4/23/18, 1:49 PM, Isaac Levy wrote:

Thanks. Another small perf patch below -- maybe we can combine. Avoids a
StringBuilder allocation:

--- a/src/java.base/share/classes/java/util/regex/Matcher.java
+++ b/src/java.base/share/classes/java/util/regex/Matcher.java
@@ -993,13 +993,11 @@
 public Matcher appendReplacement(StringBuilder sb, String replacement)
{
  // If no match, return error
  if (first < 0)
  throw new IllegalStateException("No match available");
-StringBuilder result = new StringBuilder();
-appendExpandedReplacement(replacement, result);
  // Append the intervening text
  sb.append(text, lastAppendPosition, first);
  // Append the match substitution
+appendExpandedReplacement(replacement, sb);
-sb.append(result);


On Mon, Apr 23, 2018 at 4:31 PM, Xueming Shen > wrote:

this looks fine.

-sherman

Re: RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.

Re: RFR of JDK-8201469,test under java/rmi should be restricted to not run concurrently

Re: [11] RFR: 8202026 8193552 : ISO 4217 Amendment #165 # 166 Update

Re: RFR: 8201274: Launch Single-File Source-Code Programs

Re: [11] RFR: 8202026 8193552 : ISO 4217 Amendment #165 # 166 Update

Re: [PATCH] regex matcher opt: remove redundant StringBuilder

[11] RFR: 8202026 8193552 : ISO 4217 Amendment #165 # 166 Update

Re: RFR: 8201274: Launch Single-File Source-Code Programs

Re: JDK 11 RFR of 8200478: For boxing conversion javac uses Long.valueOf which does not guarantee caching according to its javadoc

JDK 11 RFR of 8200478: For boxing conversion javac uses Long.valueOf which does not guarantee caching according to its javadoc

Re: [11] RFR: 8181157: CLDR Timezone name fallback implementation

Re: RFR: JDK-8193877- DateTimeFormatterBuilder throws ClassCastException when using padding

Re: 8202062: Put FileChannel and FileOutpuStream variants of AtomicAppend on problem list

8202062: Put FileChannel and FileOutpuStream variants of AtomicAppend on problem list

Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData

Re: [PATCH] regex matcher opt: remove redundant StringBuilder

Re: [PATCH] Add compareToUnsigned to java.nio.*Buffer

RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData

Re: [PATCH] regex matcher opt: remove redundant StringBuilder

RE: RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.

[PATCH] regex matcher opt: remove redundant StringBuilder

RE: RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.

Re: [PATCH] minor regex cleanup: use switch for enum

Re: [PATCH] Add compareToUnsigned to java.nio.*Buffer

Re: [PATCH] minor regex cleanup: use switch for enum

Re: [PATCH] minor regex cleanup: use switch for enum

Re: [PATCH] minor regex cleanup: use switch for enum

Re: [PATCH] minor regex cleanup: use switch for enum

28 matches

Site Navigation

Mail list logo

Footer information