RFR: 8323183: ClassFile API performance improvements [v7]

[Claes Redestad]

On Mon, 15 Jan 2024 12:01:37 GMT, Adam Sotona <asotona at openjdk.org> wrote:

>> ClassFile API performance related improvements have been separated from #17121 into this PR.
>> 
>> These improvements are important to minimize performance regression of
>> 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes #17121
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update src/java.base/share/classes/jdk/internal/classfile/impl/StackMapDecoder.java
>   
>   Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>

Improvement looks good, but the microbenchmark needs to sink new instances into a blackhole to make sure we measure the right thing.

src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java line 36:

> 34: import java.lang.classfile.constantpool.MemberRefEntry;
> 35: import static java.lang.classfile.ClassFile.*;
> 36: import java.lang.constant.MethodTypeDesc;

Imports are strangely out-of-order and imports in the classfile package look haphazard. Is there some system to it that I don't see? I don't know if we have an applicable style guide to lean on, but alphabetically sorted and static imports split out at the end seem like the convention in the OpenJDK sources.

src/java.base/share/classes/jdk/internal/classfile/impl/StackCounter.java line 103:

> 101:         this.methodDesc = methodDesc;
> 102:         this.cp = cp;
> 103:         targets = new ArrayDeque<>();

Seeing `ArrayDeque` being put to good use always makes me happy.

test/micro/org/openjdk/bench/jdk/classfile/CodeAttributeTools.java line 108:

> 106:     @Benchmark
> 107:     public void benchmarkStackMapsGenerator() {
> 108:         for (var d : data) new StackMapGenerator(

When we return something from a benchmark JMH makes sure to sink that something into a blackhole to avoid JIT assuming the result is unused and eliminating it, wholly or in parts. When it's impractical to return a single thing that captures all the computation in the benchmark it's good practice to let a `org.openjdk.jmh.infra.Blackhole` consume effects to attain the same effect:

    public void benchmarkStackMapsGenerator(Blackhole bh) {
        for (var d : data) bh.consume(new StackMapGenerator(

-------------

Changes requested by redestad (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17306#pullrequestreview-1913911634
PR Review Comment: https://git.openjdk.org/jdk/pull/17306#discussion_r1510912679
PR Review Comment: https://git.openjdk.org/jdk/pull/17306#discussion_r1510921619
PR Review Comment: https://git.openjdk.org/jdk/pull/17306#discussion_r1510933717