RFR: 8263108: Class initialization deadlock in java.lang.constant
Jaikiran Pai
jpai at openjdk.java.net
Wed Mar 10 02:15:28 UTC 2021
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
More information about the core-libs-dev
mailing list