Use single character replace variant in Resources.toPackageName(String)

[Christoph Dreis]

Hi Ivan,

I was indeed running with JDK 11 instead of JDK 13 (sorry), but like you I still think it's worthwhile.

>    BUGURL: https://bugs.openjdk.java.net/browse/JDK-8236705
>    WEBREV: http://cr.openjdk.java.net/~igerasim/8236705/00/webrev/
>   If we agree on the content of the fix, I can push it after a sanity build.
    
Looks good to me. Thanks.

I wonder if the copyright should change to 2020, but I don't how this is handled in the JDK.

Cheers,
Christoph
    
    On 1/6/20 10:41 PM, Ivan Gerasimov wrote:
    > Hi Christoph!
    >
    > First, the same optimization can be done in 
    > src/java.base/share/classes/jdk/internal/module/ModulePath.java:
    >                 mainClass = mainClass.replace("/", ".");
    >
    > Second, I wonder what was the JDK version you ran your benchmark on?
    >
    > After the fix for JDK-8222955 the method replace(CharSequence, 
    > CharSequence) now detects one-char arguments as a special case.
    >
    > Thus, I think, more work should actually reduce the difference in 
    > performance between two versions of replace.
    >
    > Still, I think this optimization is worthwhile.  I can sponsor it for 
    > you.
    >
    > With kind regards,
    >
    > Ivan
    >
    >
    > On 1/6/20 9:33 AM, Christoph Dreis wrote:
    >> Hi and a happy new year,
    >>
    >> I recently noticed that 
    >> jdk.internal.module.Resources.toPackageName(String) makes use of 
    >> String.replace(CharSequence, CharSequence) while it could use the 
    >> single char variant in my opinion:
    >>
    >> diff --git 
    >> a/src/java.base/share/classes/jdk/internal/module/Resources.java 
    >> b/src/java.base/share/classes/jdk/internal/module/Resources.java
    >> --- a/src/java.base/share/classes/jdk/internal/module/Resources.java
    >> +++ b/src/java.base/share/classes/jdk/internal/module/Resources.java
    >> @@ -64,7 +64,7 @@
    >>           if (index == -1 || index == name.length()-1) {
    >>               return "";
    >>           } else {
    >> -            return name.substring(0, index).replace("/", ".");
    >> +            return name.substring(0, index).replace('/', '.');
    >>           }
    >>       }
    >>
    >> I ran an isolated benchmark with some test data on it, which shows 
    >> the following results
    >>
    >> Benchmark (param)     Mode  Cnt     Score     Error   Units
    >> MyBenchmark.testNew java/lang  avgt      10    14,905 ±   0,130   ns/op
    >> MyBenchmark.testNew:·gc.alloc.rate.norm    java/lang  avgt 10    
    >> 48,000 ±   0,001    B/op
    >> MyBenchmark.testNew a/b/c/d/e  avgt      10    23,122 ±   0,389   ns/op
    >> MyBenchmark.testNew:·gc.alloc.rate.norm   a/b/c/d/e  avgt 10    
    >> 96,000 ±   0,001    B/op
    >> MyBenchmark.testOld java/lang  avgt       10    16,614 ±   0,420   ns/op
    >> MyBenchmark.testOld:·gc.alloc.rate.norm     java/lang avgt       
    >> 10    48,000 ±   0,001    B/op
    >> MyBenchmark.testOld a/b/c/d/e  avgt       10    84,745 ±   1,329   ns/op
    >> MyBenchmark.testOld:·gc.alloc.rate.norm    a/b/c/d/e  avgt 10   
    >> 120,000 ±   0,001    B/op
    >>
    >> As you can see the more replacing needs to be done the better the 
    >> newer version seems to be perform.
    >>
    >> In case this is considered worthwhile I would need someone to sponsor 
    >> the patch. If not, I'm sorry for the noise.
    >>
    >> Cheers,
    >> Christoph Dreis
    >>
    >>
    >>
    -- 
    With kind regards,
    Ivan Gerasimov