RFR: 8265075: Improve and simplify Class.resolveName()

[Сергей Цыпанов]

On Tue, 13 Apr 2021 14:36:39 GMT, Alan Bateman <alanb at 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