RFR: 8263108: Class initialization deadlock in java.lang.constant

[Jaikiran Pai]

On Tue, 9 Mar 2021 17:50:26 GMT, Peter Levart <plevart at 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
>
> 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