[PATCH] minor regex cleanup: use switch for enum

[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 <xueming.shen at oracle.com> 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 <xueming.shen at oracle.com
>>> <mailto:xueming.shen at oracle.com>> wrote:
>>>> this looks fine.
>>>>
>>>> -sherman
>>
>
>