Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor} [v5]
On Fri, 31 May 2024 17:59:18 GMT, jengebr wrote: >> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of >> Class[0] instances. This cloning is intended to prevent callers from >> changing array contents, but many Methods have zero exceptions or zero >> parameters, and returning the original `Class[0]` is sufficient. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt Score Error Units >> ConstructorBenchmark.getExceptionTypes avgt5 6.526 ± 0.183 ns/op >> ConstructorBenchmark.getExceptionTypesEmpty avgt5 5.803 ± 0.073 ns/op >> ConstructorBenchmark.getParameterTypes avgt5 6.521 ± 0.188 ns/op >> ConstructorBenchmark.getParameterTypesEmpty avgt5 5.747 ± 0.087 ns/op >> MethodBenchmark.getExceptionTypesavgt5 6.525 ± 0.163 ns/op >> MethodBenchmark.getExceptionTypesEmpty avgt5 5.783 ± 0.130 ns/op >> MethodBenchmark.getParameterTypesavgt5 6.518 ± 0.195 ns/op >> MethodBenchmark.getParameterTypesEmpty avgt5 5.742 ± 0.028 ns/op >> >> >> After: >> >> BenchmarkMode Cnt Score Error Units >> ConstructorBenchmark.getExceptionTypes avgt5 6.590 ± 0.124 ns/op >> ConstructorBenchmark.getExceptionTypesEmpty avgt5 1.351 ± 0.061 ns/op >> ConstructorBenchmark.getParameterTypes avgt5 6.651 ± 0.132 ns/op >> ConstructorBenchmark.getParameterTypesEmpty avgt5 1.353 ± 0.150 ns/op >> MethodBenchmark.getExceptionTypesavgt5 6.701 ± 0.151 ns/op >> MethodBenchmark.getExceptionTypesEmpty avgt5 1.422 ± 0.025 ns/op >> MethodBenchmark.getParameterTypesavgt5 6.629 ± 0.142 ns/op >> MethodBenchmark.getParameterTypesEmpty avgt5 1.273 ± 0.169 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Fixing copyright message, returning values from benchmark Thanks! Blank line removed, fields and methods alphabetized. - PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2142793269
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor} [v5]
On Fri, 31 May 2024 17:59:18 GMT, jengebr wrote:
>> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of
>> Class[0] instances. This cloning is intended to prevent callers from
>> changing array contents, but many Methods have zero exceptions or zero
>> parameters, and returning the original `Class[0]` is sufficient.
>>
>> Results from the included JMH benchmark:
>> Before:
>>
>> BenchmarkMode Cnt Score Error Units
>> ConstructorBenchmark.getExceptionTypes avgt5 6.526 ± 0.183 ns/op
>> ConstructorBenchmark.getExceptionTypesEmpty avgt5 5.803 ± 0.073 ns/op
>> ConstructorBenchmark.getParameterTypes avgt5 6.521 ± 0.188 ns/op
>> ConstructorBenchmark.getParameterTypesEmpty avgt5 5.747 ± 0.087 ns/op
>> MethodBenchmark.getExceptionTypesavgt5 6.525 ± 0.163 ns/op
>> MethodBenchmark.getExceptionTypesEmpty avgt5 5.783 ± 0.130 ns/op
>> MethodBenchmark.getParameterTypesavgt5 6.518 ± 0.195 ns/op
>> MethodBenchmark.getParameterTypesEmpty avgt5 5.742 ± 0.028 ns/op
>>
>>
>> After:
>>
>> BenchmarkMode Cnt Score Error Units
>> ConstructorBenchmark.getExceptionTypes avgt5 6.590 ± 0.124 ns/op
>> ConstructorBenchmark.getExceptionTypesEmpty avgt5 1.351 ± 0.061 ns/op
>> ConstructorBenchmark.getParameterTypes avgt5 6.651 ± 0.132 ns/op
>> ConstructorBenchmark.getParameterTypesEmpty avgt5 1.353 ± 0.150 ns/op
>> MethodBenchmark.getExceptionTypesavgt5 6.701 ± 0.151 ns/op
>> MethodBenchmark.getExceptionTypesEmpty avgt5 1.422 ± 0.025 ns/op
>> MethodBenchmark.getParameterTypesavgt5 6.629 ± 0.142 ns/op
>> MethodBenchmark.getParameterTypesEmpty avgt5 1.273 ± 0.169 ns/op
>
> jengebr has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Fixing copyright message, returning values from benchmark
Thanks for adding the benchmark. This looks good to me. Other nits below, your
call if you want to fix them or not. (This is usually done if there are other
changes requested, but they are not important enough to require standalone
work.)
test/micro/org/openjdk/bench/java/lang/reflect/ConstructorBenchmark.java line
64:
> 62: throw new RuntimeException(e);
> 63: }
> 64:
Here and in another benchmark. Excess newline?
test/micro/org/openjdk/bench/java/lang/reflect/ConstructorBenchmark.java line
85:
> 83: public Object[] getParameterTypes() throws Exception {
> 84: return oneParameterConstructor.getParameterTypes();
> 85: }
Here and in another benchmark. Order looks weird: non-empty, empty, empty,
non-empty.
-
Marked as reviewed by shade (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19327#pullrequestreview-2091420074
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1622774426
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1622775072
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor} [v5]
> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of > Class[0] instances. This cloning is intended to prevent callers from > changing array contents, but many Methods have zero exceptions or zero > parameters, and returning the original `Class[0]` is sufficient. > > Results from the included JMH benchmark: > Before: > > BenchmarkMode Cnt Score Error Units > ConstructorBenchmark.getExceptionTypes avgt5 6.526 ± 0.183 ns/op > ConstructorBenchmark.getExceptionTypesEmpty avgt5 5.803 ± 0.073 ns/op > ConstructorBenchmark.getParameterTypes avgt5 6.521 ± 0.188 ns/op > ConstructorBenchmark.getParameterTypesEmpty avgt5 5.747 ± 0.087 ns/op > MethodBenchmark.getExceptionTypesavgt5 6.525 ± 0.163 ns/op > MethodBenchmark.getExceptionTypesEmpty avgt5 5.783 ± 0.130 ns/op > MethodBenchmark.getParameterTypesavgt5 6.518 ± 0.195 ns/op > MethodBenchmark.getParameterTypesEmpty avgt5 5.742 ± 0.028 ns/op > > > After: > > BenchmarkMode Cnt Score Error Units > ConstructorBenchmark.getExceptionTypes avgt5 6.590 ± 0.124 ns/op > ConstructorBenchmark.getExceptionTypesEmpty avgt5 1.351 ± 0.061 ns/op > ConstructorBenchmark.getParameterTypes avgt5 6.651 ± 0.132 ns/op > ConstructorBenchmark.getParameterTypesEmpty avgt5 1.353 ± 0.150 ns/op > MethodBenchmark.getExceptionTypesavgt5 6.701 ± 0.151 ns/op > MethodBenchmark.getExceptionTypesEmpty avgt5 1.422 ± 0.025 ns/op > MethodBenchmark.getParameterTypesavgt5 6.629 ± 0.142 ns/op > MethodBenchmark.getParameterTypesEmpty avgt5 1.273 ± 0.169 ns/op jengebr has updated the pull request incrementally with one additional commit since the last revision: Fixing copyright message, returning values from benchmark - Changes: - all: https://git.openjdk.org/jdk/pull/19327/files - new: https://git.openjdk.org/jdk/pull/19327/files/e043fd57..b0d7942f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19327&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19327&range=03-04 Stats: 54 lines in 2 files changed: 38 ins; 0 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/19327.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19327/head:pull/19327 PR: https://git.openjdk.org/jdk/pull/19327
