RFR: 8283541: Add Statical counters and some comments in PhaseStringOpts [v2]

[Xin Liu]

On Tue, 19 Apr 2022 16:25:43 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Xin Liu has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>> 
>>  - Merge branch 'master' into JDK-8283541
>>  - bring back multiple. further simplify the interface.
>>  - 8283541: Add Statical counters and some comments in PhaseStringOpts
>
> Changes seem fine.
> 
> This code become almost obsolete after [JEP 280](https://openjdk.java.net/jeps/280)
> 
> You may need to test with `-XDstringConcat=inline` to exercise this C2's code more.

hi, @vnkozlov, @TobiHartmann 
Thank you for taking a look at this.
 
> This code become almost obsolete after [JEP 280](https://openjdk.java.net/jeps/280) 

I am actually quite confused about `StringOpts`.  After stepping into code detail, I realize the phase is designed for "String concatenation" produced by javac.  As you said, javac has changed its default concatenation policy in JEP-280. **Will we delete this phase in near future?**

IIUC, now PhaseStringOpts can improve code in 2 cases. The first case is the legacy bytecodes, or bytecodes generated by "-XDstringConcat=inline", like you hint. The second one is hand-writing StringBuilder.  For the second case, current implementation is incomplete.  I guess many Java developers (including me) don't realize that "fluent-chain" is mandatory here. if you accidentally break the fluent chain,  PhaseStringOpts will fail to recognize the pattern.   here I modify the microbenchmark from [Jose's.](https://github.com/JosePaumard/jep-cafe-07-string-concatenation/blob/master/src/main/java/org/paumard/jepcafe/StringConcat.java#L69)  and show the difference. 


Benchmark                            (converted)  (size)  Mode  Cnt    Score    Error  Units
StringConcat.stringBuilder4_2               1000      10  avgt   30  657.011 ± 18.321  ns/op
StringConcat.stringBuilder4_fluent2         1000      10  avgt   30   74.649 ±  1.085  ns/op

    @Benchmark
    public String stringBuilder4_2() {
        StringBuilder sb = new StringBuilder(1024);
        sb.append(s0).append(s1).append(s2).append(s3);
        return sb.toString();
    }

    @Benchmark
    public String stringBuilder4_fluent2() {
        StringBuilder sb = new StringBuilder(1024);
        return sb.append(s0).append(s1).append(s2).append(s3).toString();
    }


I have a patch to recognize the loose form, but I am not sure we should pursuit it or not. In real world,  it's very unlikely to use StringBuilder to concatenate some strings or integers, right?  "+" is the right way to go, right?

-------------

PR: https://git.openjdk.java.net/jdk/pull/7933