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
