On Tue, 9 Mar 2021 17:50:26 GMT, Peter Levart <[email protected]> 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
>
> Hi Jaikiran,
>
> Since the object assigned to cannonicalMap is an immutable Map created with
> `Map.ofEntries()` which is able to be safely published via data-race, the
> `cannonicalMap` field need not be volatile which could be more optimal on
> ARM. In that case you must avoid reading the field twice outside the
> synchronized block. So you could remove the volatile modifier from the field
> and do the following:
>
> private ConstantDesc tryCanonicalize() {
> var canonDescs = canonicalMap;
> if (canonDescs == null) {
> canonDescs = Map.ofEntries(...);
> synchronized (DynamicConstantDesc.class) {
> if (canonicalMap == null) {
> canonicalMap = canonDescs;
> } else {
> canonDescs = canonicalMap;
> }
> }
> }
> ... use canonDescs ...
> }
>
>
> Peter
Hello Peter,
Thank you for your inputs. I think what you suggest is a good idea. I have
updated the PR with this change.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2893
