On Tue, 9 Mar 2021 13:46:04 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> An alternate approach that I thought of was to completely get rid of this > shared cache canonicalMap and instead just use method level map (that gets > initialized each time) in the tryCanonicalize method (thus requiring no > locks/synchronization). I ran a JMH benchmark with the current change > proposed in this PR and with the alternate approach of using the method level > map. The performance number from that run showed that the approach of using > the method level map has relatively big impact on performance (even when > there's a cache hit in that method). So I decided to not pursue that > approach. I'll include the benchmark code and the numbers in a comment in > this PR, for reference. The benchmark code: package org.myapp; import org.openjdk.jmh.annotations.*; import java.lang.constant.*; import java.util.concurrent.TimeUnit; public class MyBenchmark { enum MyEnum {A, B} @Benchmark @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.NANOSECONDS) @Threads(10) @Fork(3) public void dynamicConstantDesc_ofCanonical() { final ConstantDesc desc = DynamicConstantDesc.ofCanonical(ConstantDescs.BSM_ENUM_CONSTANT, "A", ClassDesc.of("org.myapp.MyBenchmark").nested("MyEnum"), new ConstantDesc[0]); } } Benchmark results: Current proposed patch in this PR: Benchmark Mode Cnt Score Error Units MyBenchmark.dynamicConstantDesc_ofCanonical avgt 15 2295.714 ± 228.951 ns/op Alternate approach of _not_ using the `canonicalMap` and instead using method level map: Benchmark Mode Cnt Score Error Units MyBenchmark.dynamicConstantDesc_ofCanonical avgt 15 4220.446 ± 831.905 ns/op ------------- PR: https://git.openjdk.java.net/jdk/pull/2893