On Mon, 15 Mar 2021 17:15:59 GMT, Chris Hegarty <che...@openjdk.org> wrote:
>>> What you have is probably fine, but has any consideration been given to
>>> using the initialization-on-demand holder idiom? e.g.
>>>
>>> ```
>>> static private class CanonicalMapHolder {
>>> static final Map<MethodHandleDesc, Function<DynamicConstantDesc<?>,
>>> ConstantDesc>> CANONICAL_MAP
>>> = Map.ofEntries(..);
>>> }
>>> ```
>>
>> Hello Chris,
>>
>> Although I had thought of some other alternate ways to fix this, this idiom
>> isn't something that I had thought of. Now that you showed this, I thought
>> about it a bit more and it does look a lot more "natural" to read and
>> maintain as compared to the current changes in this PR which does some
>> juggling to avoid this deadlock. However, having thought about your proposed
>> solution, I think _in theory_ it still leaves open the possibility of a
>> deadlock.
>>
>> Here's what the patch looks like with your suggested change:
>>
>> diff --git
>> a/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
>> b/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
>> index 976b09e5665..c7bdcf4ce32 100644
>> --- a/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
>> +++ b/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
>> @@ -64,8 +64,6 @@ public abstract class DynamicConstantDesc<T>
>> private final String constantName;
>> private final ClassDesc constantType;
>>
>> - private static Map<MethodHandleDesc, Function<DynamicConstantDesc<?>,
>> ConstantDesc>> canonicalMap;
>> -
>> /**
>> * Creates a nominal descriptor for a dynamic constant.
>> *
>> @@ -274,25 +272,7 @@ public abstract class DynamicConstantDesc<T>
>> }
>>
>> private ConstantDesc tryCanonicalize() {
>> - var canonDescs = canonicalMap;
>> - if (canonDescs == null) {
>> - canonDescs = Map.ofEntries(
>> - Map.entry(ConstantDescs.BSM_PRIMITIVE_CLASS,
>> DynamicConstantDesc::canonicalizePrimitiveClass),
>> - Map.entry(ConstantDescs.BSM_ENUM_CONSTANT,
>> DynamicConstantDesc::canonicalizeEnum),
>> - Map.entry(ConstantDescs.BSM_NULL_CONSTANT,
>> DynamicConstantDesc::canonicalizeNull),
>> - Map.entry(ConstantDescs.BSM_VARHANDLE_STATIC_FIELD,
>> DynamicConstantDesc::canonicalizeStaticFieldVarHandle),
>> - Map.entry(ConstantDescs.BSM_VARHANDLE_FIELD,
>> DynamicConstantDesc::canonicalizeFieldVarHandle),
>> - Map.entry(ConstantDescs.BSM_VARHANDLE_ARRAY,
>> DynamicConstantDesc::canonicalizeArrayVarHandle));
>> - synchronized (DynamicConstantDesc.class) {
>> - if (canonicalMap == null) {
>> - canonicalMap = canonDescs;
>> - } else {
>> - canonDescs = canonicalMap;
>> - }
>> - }
>> - }
>> -
>> - Function<DynamicConstantDesc<?>, ConstantDesc> f =
>> canonDescs.get(bootstrapMethod);
>> + Function<DynamicConstantDesc<?>, ConstantDesc> f =
>> CanonicalMapHolder.CANONICAL_MAP.get(bootstrapMethod);
>> if (f != null) {
>> try {
>> return f.apply(this);
>> @@ -405,4 +385,15 @@ public abstract class DynamicConstantDesc<T>
>> super(bootstrapMethod, constantName, constantType,
>> bootstrapArgs);
>> }
>> }
>> +
>> + private static final class CanonicalMapHolder {
>> + static final Map<MethodHandleDesc, Function<DynamicConstantDesc<?>,
>> ConstantDesc>> CANONICAL_MAP =
>> + Map.ofEntries(
>> + Map.entry(ConstantDescs.BSM_PRIMITIVE_CLASS,
>> DynamicConstantDesc::canonicalizePrimitiveClass),
>> + Map.entry(ConstantDescs.BSM_ENUM_CONSTANT,
>> DynamicConstantDesc::canonicalizeEnum),
>> + Map.entry(ConstantDescs.BSM_NULL_CONSTANT,
>> DynamicConstantDesc::canonicalizeNull),
>> + Map.entry(ConstantDescs.BSM_VARHANDLE_STATIC_FIELD,
>> DynamicConstantDesc::canonicalizeStaticFieldVarHandle),
>> + Map.entry(ConstantDescs.BSM_VARHANDLE_FIELD,
>> DynamicConstantDesc::canonicalizeFieldVarHandle),
>> + Map.entry(ConstantDescs.BSM_VARHANDLE_ARRAY,
>> DynamicConstantDesc::canonicalizeArrayVarHandle));
>> + }
>> }
>>
>>
>> Please consider the following, where I try and explain the theoretical
>> possibility of a deadlock with this approach:
>>
>> 1. Let's consider 2 threads T1 and T2 doing concurrent execution
>>
>> 2. Let's for a moment assume that neither `DynamicConstantDesc` nor
>> `ConstantDescs` classes have been initialized.
>>
>> 3. T1 does a call to `DynamicConstantDesc.ofCanonical(...)` and T2 does a
>> call to something/anything on `ConstantDescs`, which triggers a class
>> initialization of `ConstantDescs`.
>>
>> 4. T1 (which called `DynamicConstantDesc.ofCanonical(...)`) will reach the
>> `tryCanonicalize` and will access `CanonicalMapHolder.CANONICAL_MAP` in the
>> implementation of that method. Because of this access (and since
>> `CanonicalMapHolder` hasn't yet been initialized), T1 will obtain (an
>> implicit) lock on the `CanonicalMapHolder` class (for the class
>> initialization). Let's assume T1 is granted this lock on
>> `CanonicalMapHolder` class and it goes ahead into the static block of that
>> holder class to do the initialization of `CANONICAL_MAP`. To do so, because
>> of the code in the static block of `CanonicalMapHolder`, it now requires a
>> class initialization lock on `ConstantDescs` (since `ConstantDescs` hasn't
>> yet been initialized). So it requests for that lock on `ConstantDescs` class.
>>
>> 5. Concurrently, let's say T2 has initiated a class initialization of
>> `ConstantDescs`. So T2 is currently holding a class initialization lock on
>> `ConstantDescs`. While the static initialization of `ConstantDescs` is going
>> on, let's assume (in theory), due to the whole lot of code in the static
>> initialization of `ConstantDescs`, it either directly or indirectly ends up
>> calling `DynamicConstantDesc.ofCanonical(...)`. This then means that T2 now
>> enters the `tryCanonicalize` and accesses `CanonicalMapHolder.CANONICAL_MAP`
>> and since `CanonicalMapHolder` hasn't yet been (fully) initialized (remember
>> T1 is still in the static block of `CanonicalMapHolder`), it tries to obtain
>> a class initialization lock on `CanonicalMapHolder` which is currently held
>> by T1. T1 won't let go that lock since it wants the lock on `ConstantDescs`
>> which is currently held by T2. This effectively ends up triggering the
>> deadlock.
>>
>> This deadlock isn't possible with the current approach that this PR has.
>>
>> I want to clarify that, the deadlock that I explain above with your proposed
>> approach is just a theoretical possibility. The `ConstantDescs` doesn't
>> currently have any direct call to `DynamicConstantDesc.ofCanonical` in its
>> static initialization nor can I see or think of any indirect calls to
>> `DynamicConstantDesc.ofCanonical` from its static block. I need input from
>> you whether I should keep aside this theoretic issue and deal with it
>> if/when we run into it (the test case in this PR, IMO, is robust enough to
>> catch that deadlock if/when it happens due to any future code changes in
>> this area). If you and others think that we can ignore this case, then your
>> proposed approach of using this lazy holder for initialization, IMO, is much
>> cleaner and natural to read and I will go ahead and update this PR to use it.
>>
>> For those interested in seeing this potential theoretical deadlock with the
>> lazy holder approach in action, I just created a separate branch here
>> https://github.com/jaikiran/jdk/commits/8263108-demo-deadlock-theory which
>> consistently reproduces the deadlock when the `DynamicConstantDescTest`
>> jtreg test is run. The crucial part is that I introduced the following
>> (dummy) call in `ConstantDescs`:
>>
>> diff --git
>> a/src/java.base/share/classes/java/lang/constant/ConstantDescs.java
>> b/src/java.base/share/classes/java/lang/constant/ConstantDescs.java
>> index 3a000d1bd4a..3f793f5a8b3 100644
>> --- a/src/java.base/share/classes/java/lang/constant/ConstantDescs.java
>> +++ b/src/java.base/share/classes/java/lang/constant/ConstantDescs.java
>> @@ -286,6 +286,13 @@ public final class ConstantDescs {
>> static final DirectMethodHandleDesc MHD_METHODHANDLE_ASTYPE
>> = MethodHandleDesc.ofMethod(Kind.VIRTUAL, CD_MethodHandle,
>> "asType",
>> MethodTypeDesc.of(CD_MethodHandle,
>> CD_MethodType));
>> +
>> + static {
>> + // just a dummy call to DynamicConstantDesc.ofCanonical
>> + DynamicConstantDesc.ofCanonical(ConstantDescs.BSM_ENUM_CONSTANT,
>> "SHOW_REFLECT_FRAMES",
>> + ClassDesc.of("java.lang.StackWalker").nested("Option"), new
>> ConstantDesc[0]);
>> + }
>> +
>>
>>
>> The following thread dump shows the deadlock with that lazy holder approach:
>>
>> "MainThread" #15 prio=5 os_prio=31 cpu=6.26ms elapsed=120.18s
>> tid=0x00007fe0e58df800 nid=0x5e03 waiting on condition [0x000070000d71d000]
>> java.lang.Thread.State: WAITING (parking)
>> at jdk.internal.misc.Unsafe.park(java.base@17-internal/Native Method)
>> - parking to wait for <0x000000070ff65d90> (a
>> java.util.concurrent.FutureTask)
>> at
>> java.util.concurrent.locks.LockSupport.park(java.base@17-internal/LockSupport.java:211)
>> at
>> java.util.concurrent.FutureTask.awaitDone(java.base@17-internal/FutureTask.java:447)
>> at
>> java.util.concurrent.FutureTask.get(java.base@17-internal/FutureTask.java:190)
>> at DynamicConstantDescTest.main(DynamicConstantDescTest.java:80)
>> at
>> jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base@17-internal/Native
>> Method)
>> at
>> jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base@17-internal/NativeMethodAccessorImpl.java:78)
>> at
>> jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base@17-internal/DelegatingMethodAccessorImpl.java:43)
>> at
>> java.lang.reflect.Method.invoke(java.base@17-internal/Method.java:566)
>> at
>> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
>> at java.lang.Thread.run(java.base@17-internal/Thread.java:831)
>>
>> "pool-1-thread-1" #16 prio=5 os_prio=31 cpu=14.32ms elapsed=120.18s
>> tid=0x00007fe0e58dea00 nid=0x9903 waiting on condition [0x000070000d820000]
>> java.lang.Thread.State: WAITING (parking)
>> at jdk.internal.misc.Unsafe.park(java.base@17-internal/Native Method)
>> - parking to wait for <0x000000070ff59d30> (a
>> java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject)
>> at
>> java.util.concurrent.locks.LockSupport.park(java.base@17-internal/LockSupport.java:341)
>> at
>> java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionNode.block(java.base@17-internal/AbstractQueuedSynchronizer.java:506)
>> at
>> java.util.concurrent.ForkJoinPool.unmanagedBlock(java.base@17-internal/ForkJoinPool.java:3454)
>> at
>> java.util.concurrent.ForkJoinPool.managedBlock(java.base@17-internal/ForkJoinPool.java:3425)
>> at
>> java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(java.base@17-internal/AbstractQueuedSynchronizer.java:1623)
>> at
>> java.util.concurrent.LinkedBlockingQueue.take(java.base@17-internal/LinkedBlockingQueue.java:435)
>> at
>> java.util.concurrent.ThreadPoolExecutor.getTask(java.base@17-internal/ThreadPoolExecutor.java:1061)
>> at
>> java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@17-internal/ThreadPoolExecutor.java:1121)
>> at
>> java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@17-internal/ThreadPoolExecutor.java:635)
>> at java.lang.Thread.run(java.base@17-internal/Thread.java:831)
>>
>> "pool-1-thread-2" #17 prio=5 os_prio=31 cpu=7.88ms elapsed=120.18s
>> tid=0x00007fe0e4012c00 nid=0x6003 in Object.wait() [0x000070000d923000]
>> java.lang.Thread.State: RUNNABLE
>> at
>> java.lang.constant.DynamicConstantDesc.tryCanonicalize(java.base@17-internal/DynamicConstantDesc.java:275)
>> - waiting on the Class initialization monitor for
>> java.lang.constant.DynamicConstantDesc$CanonicalMapHolder
>> at
>> java.lang.constant.DynamicConstantDesc.ofCanonical(java.base@17-internal/DynamicConstantDesc.java:136)
>> at
>> DynamicConstantDescTest$InvokeOfCanonical.call(DynamicConstantDescTest.java:129)
>> at
>> java.util.concurrent.FutureTask.run(java.base@17-internal/FutureTask.java:264)
>> at
>> java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@17-internal/ThreadPoolExecutor.java:1135)
>> at
>> java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@17-internal/ThreadPoolExecutor.java:635)
>> at java.lang.Thread.run(java.base@17-internal/Thread.java:831)
>>
>> "pool-1-thread-3" #18 prio=5 os_prio=31 cpu=15.43ms elapsed=120.18s
>> tid=0x00007fe0e4070600 nid=0x9803 in Object.wait() [0x000070000da26000]
>> java.lang.Thread.State: RUNNABLE
>> at
>> java.lang.constant.DynamicConstantDesc.tryCanonicalize(java.base@17-internal/DynamicConstantDesc.java:275)
>> - waiting on the Class initialization monitor for
>> java.lang.constant.DynamicConstantDesc$CanonicalMapHolder
>> at
>> java.lang.constant.DynamicConstantDesc.ofCanonical(java.base@17-internal/DynamicConstantDesc.java:136)
>> at
>> java.lang.constant.ConstantDescs.<clinit>(java.base@17-internal/ConstantDescs.java:292)
>> at java.lang.Class.forName0(java.base@17-internal/Native Method)
>> at java.lang.Class.forName(java.base@17-internal/Class.java:375)
>> at DynamicConstantDescTest$Task.call(DynamicConstantDescTest.java:104)
>> at DynamicConstantDescTest$Task.call(DynamicConstantDescTest.java:87)
>> at
>> java.util.concurrent.FutureTask.run(java.base@17-internal/FutureTask.java:264)
>> at
>> java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@17-internal/ThreadPoolExecutor.java:1135)
>> at
>> java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@17-internal/ThreadPoolExecutor.java:635)
>> at java.lang.Thread.run(java.base@17-internal/Thread.java:831)
>>
>> "pool-1-thread-4" #19 prio=5 os_prio=31 cpu=5.39ms elapsed=120.18s
>> tid=0x00007fe0e4070c00 nid=0x9603 in Object.wait() [0x000070000db29000]
>> java.lang.Thread.State: RUNNABLE
>> at
>> java.lang.constant.DynamicConstantDesc$CanonicalMapHolder.<clinit>(java.base@17-internal/DynamicConstantDesc.java:390)
>> - waiting on the Class initialization monitor for
>> java.lang.constant.ConstantDescs
>> at
>> java.lang.constant.DynamicConstantDesc.tryCanonicalize(java.base@17-internal/DynamicConstantDesc.java:275)
>> at
>> java.lang.constant.DynamicConstantDesc.ofCanonical(java.base@17-internal/DynamicConstantDesc.java:136)
>> at
>> DynamicConstantDescTest$InvokeOfCanonical.call(DynamicConstantDescTest.java:129)
>> at
>> java.util.concurrent.FutureTask.run(java.base@17-internal/FutureTask.java:264)
>> at
>> java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@17-internal/ThreadPoolExecutor.java:1135)
>> at
>> java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@17-internal/ThreadPoolExecutor.java:635)
>> at java.lang.Thread.run(java.base@17-internal/Thread.java:831)
>
>> If you and others think that we can ignore this case, then your proposed
>> approach of using this lazy holder for initialization, IMO, is much cleaner
>> and natural to read and I will go ahead and update this PR to use it.
>
> For me, at least, the holder pattern is clearer. I'm happy with that
> approach. ( I don't have an objection to the alternative, just a mild
> preference for the holder )
Hello Chris, using the holder pattern sounds fine to me then. I've updated this
PR accordingly. The test continues to pass with this fix.
Peter, I didn't get a chance to try out the `@Stable` for the previous approach
but given that we decided to use this holder pattern, we will no longer need
the performance tweaks.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2893
