On Fri, 12 Mar 2021 14:53:45 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)
-------------
PR: https://git.openjdk.java.net/jdk/pull/2893
