Re: RFR: 8323183: ClassFile API performance improvements [v7]
On Mon, 4 Mar 2024 10:08:27 GMT, Claes Redestad wrote:
>> 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
>
> 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.
Fixed, thanks.
> 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(
Fixed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17306#discussion_r1511040395
PR Review Comment: https://git.openjdk.org/jdk/pull/17306#discussion_r1511042556
Re: RFR: 8323183: ClassFile API performance improvements [v7]
On Mon, 15 Jan 2024 12:01:37 GMT, Adam Sotona 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
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
Re: RFR: 8323183: ClassFile API performance improvements [v7]
On Mon, 15 Jan 2024 12:01:37 GMT, Adam Sotona 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 Please review. Thank you! - PR Comment: https://git.openjdk.org/jdk/pull/17306#issuecomment-1976089621
Re: RFR: 8323183: ClassFile API performance improvements [v7]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/17306/files - new: https://git.openjdk.org/jdk/pull/17306/files/5f67f8c1..5a04ec59 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17306=06 - incr: https://webrevs.openjdk.org/?repo=jdk=17306=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17306.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17306/head:pull/17306 PR: https://git.openjdk.org/jdk/pull/17306
