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

[Ivan Gerasimov]

So, I filed a Jira bug:

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.

With kind regards,

Ivan


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