Re: [PATCH] minor regex cleanup: use switch for enum
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] minor regex cleanup: use switch for enum
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
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
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
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 mailto:xueming.s...@oracle.com>> wrote:
this looks fine.
-sherman
Re: [PATCH] minor regex cleanup: use switch for enum
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. >
Re: [PATCH] minor regex cleanup: use switch for enum
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? A larger change could remove this branch entirely, with different classes for each of the types, which are known during compile.
Re: [PATCH] minor regex cleanup: use switch for enum
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).
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
>
>
--
- DML
Re: [PATCH] minor regex cleanup: use switch for enum
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: [PATCH] minor regex cleanup: use switch for enum
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: [PATCH] minor regex cleanup: use switch for enum
this looks fine.
-sherman
On 4/23/18, 12:26 PM, Isaac Levy wrote:
ping?
Isaac
On Wed, Apr 18, 2018 at 2:58 PM, Isaac Levy wrote:
Hi,
Minor improvement in readability (and probably perf) for Pattern. Switch
is more consistent with the rest of the impl and the resulting tableswitch
avoids a comparison for possessives.
-Isaac
--- a/src/java.base/share/classes/java/util/regex/Pattern.java
+++ b/src/java.base/share/classes/java/util/regex/Pattern.java
@@ -4356,7 +4356,9 @@
-if (type == Qtype.GREEDY)
+switch (type) {
+case GREEDY:
return match0(matcher, i, j, seq);
-else if (type == Qtype.LAZY)
+case LAZY:
return match1(matcher, i, j, seq);
-else
+default:
return match2(matcher, i, j, seq);
+}
@@ -4527,7 +4529,10 @@
-if (type == Qtype.GREEDY) {
+switch (type) {
+case GREEDY:
ret = match0(matcher, i, cmin, seq);
+break;
-} else if (type == Qtype.LAZY) {
+case LAZY:
ret = match1(matcher, i, cmin, seq);
+break;
-} else {
+default:
ret = match2(matcher, i, cmin, seq);
}
Re: [PATCH] minor regex cleanup: use switch for enum
ping?
Isaac
On Wed, Apr 18, 2018 at 2:58 PM, Isaac Levy wrote:
> Hi,
>
> Minor improvement in readability (and probably perf) for Pattern. Switch
> is more consistent with the rest of the impl and the resulting tableswitch
> avoids a comparison for possessives.
>
> -Isaac
>
> --- a/src/java.base/share/classes/java/util/regex/Pattern.java
> +++ b/src/java.base/share/classes/java/util/regex/Pattern.java
> @@ -4356,7 +4356,9 @@
> -if (type == Qtype.GREEDY)
> +switch (type) {
> +case GREEDY:
> return match0(matcher, i, j, seq);
> -else if (type == Qtype.LAZY)
> +case LAZY:
> return match1(matcher, i, j, seq);
> -else
> +default:
> return match2(matcher, i, j, seq);
> +}
>
> @@ -4527,7 +4529,10 @@
> -if (type == Qtype.GREEDY) {
> +switch (type) {
> +case GREEDY:
> ret = match0(matcher, i, cmin, seq);
> +break;
> -} else if (type == Qtype.LAZY) {
> +case LAZY:
> ret = match1(matcher, i, cmin, seq);
> +break;
> -} else {
> +default:
> ret = match2(matcher, i, cmin, seq);
> }
>
>
[PATCH] minor regex cleanup: use switch for enum
Hi,
Minor improvement in readability (and probably perf) for Pattern. Switch is
more consistent with the rest of the impl and the resulting tableswitch
avoids a comparison for possessives.
-Isaac
--- a/src/java.base/share/classes/java/util/regex/Pattern.java
+++ b/src/java.base/share/classes/java/util/regex/Pattern.java
@@ -4356,7 +4356,9 @@
-if (type == Qtype.GREEDY)
+switch (type) {
+case GREEDY:
return match0(matcher, i, j, seq);
-else if (type == Qtype.LAZY)
+case LAZY:
return match1(matcher, i, j, seq);
-else
+default:
return match2(matcher, i, j, seq);
+}
@@ -4527,7 +4529,10 @@
-if (type == Qtype.GREEDY) {
+switch (type) {
+case GREEDY:
ret = match0(matcher, i, cmin, seq);
+break;
-} else if (type == Qtype.LAZY) {
+case LAZY:
ret = match1(matcher, i, cmin, seq);
+break;
-} else {
+default:
ret = match2(matcher, i, cmin, seq);
}
