On Tue, 13 Apr 2021 14:36:39 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> In mentioned method this code snippet
>>
>> int len = baseName.length() + 1 + name.length();
>> StringBuilder sb = new StringBuilder(len);
>> name = sb.append(baseName.replace('.', '/'))
>> .append('/')
>> .append(name)
>> .toString();
>>
>>
>> can be simplified with performance improvement as
>>
>> name = baseName.replace('.', '/') + '/' + name;
>>
>> Also null check of `baseName` can be removed as Class.getPackageName() never
>> returns null.
>>
>> This benchmark
>>
>> @State(Scope.Thread)
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class ResolveNameBenchmark {
>>
>> private final Class<? extends ResolveNameBenchmark> klazz = getClass();
>>
>> @Benchmark
>> public Object original() {
>> return original("com/tsypanov/ovn/ResolveNameBenchmark.class");
>> }
>>
>> @Benchmark
>> public Object patched() {
>> return patched("com/tsypanov/ovn/ResolveNameBenchmark.class");
>> }
>>
>> private String original(String name) {
>> if (!name.startsWith("/")) {
>> String baseName = getPackageName();
>> if (baseName != null && !baseName.isEmpty()) {
>> int len = baseName.length() + 1 + name.length();
>> StringBuilder sb = new StringBuilder(len);
>> name = sb.append(baseName.replace('.', '/'))
>> .append('/')
>> .append(name)
>> .toString();
>> }
>> } else {
>> name = name.substring(1);
>> }
>> return name;
>> }
>>
>> private String patched(String name) {
>> if (!name.startsWith("/")) {
>> String baseName = getPackageName();
>> if (!baseName.isEmpty()) {
>> return baseName.replace('.', '/') + '/' + name;
>> }
>> return name;
>> }
>> return name.substring(1);
>> }
>>
>> private String getPackageName() {
>> return klazz.getPackageName();
>> }
>> }
>>
>> demonstrates good improvement, especially as of memory consumption:
>>
>> Mode Cnt Score Error
>> Units
>>
>> original avgt 50 57.974 ± 0.365
>> ns/op
>> original:·gc.alloc.rate avgt 50 3419.447 ± 21.154
>> MB/sec
>> original:·gc.alloc.rate.norm avgt 50 312.006 ± 0.001
>> B/op
>> original:·gc.churn.G1_Eden_Space avgt 50 3399.396 ± 149.836
>> MB/sec
>> original:·gc.churn.G1_Eden_Space.norm avgt 50 310.198 ± 13.628
>> B/op
>> original:·gc.churn.G1_Survivor_Space avgt 50 0.004 ± 0.001
>> MB/sec
>> original:·gc.churn.G1_Survivor_Space.norm avgt 50 ≈ 10⁻³
>> B/op
>> original:·gc.count avgt 50 208.000
>> counts
>> original:·gc.time avgt 50 224.000
>> ms
>>
>> patched avgt 50 44.367 ± 0.162
>> ns/op
>> patched:·gc.alloc.rate avgt 50 2749.265 ± 10.014
>> MB/sec
>> patched:·gc.alloc.rate.norm avgt 50 192.004 ± 0.001
>> B/op
>> patched:·gc.churn.G1_Eden_Space avgt 50 2729.221 ± 193.552
>> MB/sec
>> patched:·gc.churn.G1_Eden_Space.norm avgt 50 190.615 ± 13.539
>> B/op
>> patched:·gc.churn.G1_Survivor_Space avgt 50 0.003 ± 0.001
>> MB/sec
>> patched:·gc.churn.G1_Survivor_Space.norm avgt 50 ≈ 10⁻⁴
>> B/op
>> patched:·gc.count avgt 50 167.000
>> counts
>> patched:·gc.time avgt 50 181.000
>> ms
>
> src/java.base/share/classes/java/lang/Class.java line 3067:
>
>> 3065: */
>> 3066: private String resolveName(String name) {
>> 3067: if (!name.startsWith("/")) {
>
> I expect this would be more more readable if you invert it, i.e. "if
> (name.startsWith("/") { return name.substring(1); } else { ... }.
>
> Note that the baseName == null check was needed when it was originally
> created because getPackageName could return null in the initial version.
@AlanBateman As Peter commented below there's likely to be no improvement when
the code is called from `j.l.Class` itself, and indeed there's no any.
However, there's still unnecessary null check here and in other places, so is
it reasonable to reword the ticker and this PR to rid that check, or create
another ticket, or it's not worth the effort? What do you thinl?
-------------
PR: https://git.openjdk.java.net/jdk/pull/3464
