Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Tue, 16 Mar 2021 02:34:39 GMT, Jaikiran Pai wrote: >>> 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. Failure in Linux x86 build in GitHub Actions is unrelated to this change and looks like an environmental issue: 2021-03-16T02:35:22.3488438Z Err:59 https://dl.bintray.com/sbt/debian Release.gpg 2021-03-16T02:35:22.3489341Z 502 Bad Gateway [IP: 35.161.90.47 443] 2021-03-16T02:35:30.2615937Z Reading package lists... 2021-03-16T02:35:30.2842714Z E: The repository 'https://dl.bintray.com/sbt/debian Release' is no longer signed. - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Mon, 15 Mar 2021 17:15:59 GMT, Chris Hegarty 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,
>>> 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
>> private final String constantName;
>> private final ClassDesc constantType;
>>
>> -private static Map,
>> ConstantDesc>> canonicalMap;
>> -
>> /**
>> * Creates a nominal descriptor for a dynamic constant.
>> *
>> @@ -274,25 +272,7 @@ public abstract class DynamicConstantDesc
>> }
>>
>> 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, ConstantDesc> f =
>> canonDescs.get(bootstrapMethod);
>> +Function, ConstantDesc> f =
>> CanonicalMapHolder.CANONICAL_MAP.get(bootstrapMethod);
>> if (f != null) {
>> try {
>> return f.apply(this);
>> @@ -405,4 +385,15 @@ public abstract class DynamicConstantDesc
>> super(bootstrapMethod, constantName, constantType,
>> bootstrapArgs);
>> }
>> }
>> +
>> +private static final class CanonicalMapHolder {
>> +static final Map,
>> 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
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Mon, 15 Mar 2021 08:53:43 GMT, Jaikiran Pai wrote: > 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 ) - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Fri, 12 Mar 2021 14:53:45 GMT, Chris Hegarty 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,
> 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
private final String constantName;
private final ClassDesc constantType;
-private static Map,
ConstantDesc>> canonicalMap;
-
/**
* Creates a nominal descriptor for a dynamic constant.
*
@@ -274,25 +272,7 @@ public abstract class DynamicConstantDesc
}
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, ConstantDesc> f =
canonDescs.get(bootstrapMethod);
+Function, ConstantDesc> f =
CanonicalMapHolder.CANONICAL_MAP.get(bootstrapMethod);
if (f != null) {
try {
return f.apply(this);
@@ -405,4 +385,15 @@ public abstract class DynamicConstantDesc
super(bootstrapMethod, constantName, constantType, bootstrapArgs);
}
}
+
+private static final class CanonicalMapHolder {
+static final Map,
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`.
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Fri, 12 Mar 2021 13:23:39 GMT, Peter Levart wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Follow Peter Levart's review suggestion - remove volatile
>
> This looks good to me. Maybe if you wanted the previous performance back
> (when the `cannonicalMap` field was static final and therefore JIT could
> constant-fold the Map instance), you could now annotate the field with
> `@Stable` annotation to allow JIT to produce similar code. I would also move
> the `Map.ofEntries(...)` expression into a separate private static method
> since I believe it has substantial bytecode size. `tryCanonicalize()` method
> bytecode size would then more likely fall below the inlining threshold.
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,
ConstantDesc>> CANONICAL_MAP
= Map.ofEntries(..);
}
-
PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Wed, 10 Mar 2021 02:15:28 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed patch for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8263108? >> >> As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and >> `java.lang.constant.ConstantDescs` can end up in a classloading deadlock due >> to the nature of the code involved in their static blocks. A thread dump of >> one such deadlock (reproduced using the code attached to that issue) is as >> follows: >> >> "Thread A" #14 prio=5 os_prio=31 cpu=101.45ms elapsed=8.30s >> tid=0x7ff88e01ca00 nid=0x6003 in Object.wait() [0x7a4c6000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.DynamicConstantDesc.(java.base@16-ea/DynamicConstantDesc.java:67) >> - waiting on the Class initialization monitor for >> java.lang.constant.ConstantDescs >> at Deadlock.threadA(Deadlock.java:14) >> at Deadlock$$Lambda$1/0x000800c00a08.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> "Thread B" #15 prio=5 os_prio=31 cpu=103.15ms elapsed=8.30s >> tid=0x7ff88b936a00 nid=0x9b03 in Object.wait() [0x7a5c9000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.ClassDesc.ofDescriptor(java.base@16-ea/ClassDesc.java:145) >> - waiting on the Class initialization monitor for >> java.lang.constant.DynamicConstantDesc >> at >> java.lang.constant.ConstantDescs.(java.base@16-ea/ConstantDescs.java:239) >> at Deadlock.threadB(Deadlock.java:24) >> at Deadlock$$Lambda$2/0x000800c00c28.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> The commit in this PR resolves that deadlock by moving the `canonicalMap` >> initialization in `DynamicConstantDesc`, from the static block, to a lazily >> initialized map, into the `tryCanonicalize` (private) method of the same >> class. That's the only method which uses this map. >> >> The implementation in `tryCanonicalize` carefully ensures that the deadlock >> isn't shifted from the static block to this method, by making sure that the >> `synchronization` on `DynamicConstantDesc` in this method happens only when >> it's time to write the state to the shared `canonicalMap`. This has an >> implication that the method local variable `canonDescs` can get initialized >> by multiple threads unnecessarily but from what I can see, that's the only >> way we can avoid this deadlock. This penalty of multiple threads >> initializing and then throwing away that map isn't too huge because that >> will happen only once when the `canonicalMap` is being initialized for the >> first time. >> >> 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 commit in this PR also includes a jtreg testcase which (consistently) >> reproduces the deadlock without the fix and passes when this fix is applied. >> Extra manual testing has been done to verify that the classes of interest >> (noted in that testcase) are indeed getting loaded in those concurrent >> threads and not in the main thread. For future maintainers, there's a >> implementation note added on that testcase explaining why it cannot be moved >> into testng test. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Follow Peter Levart's review suggestion - remove volatile This looks good to me. Maybe if you wanted the previous performance back (when the `cannonicalMap` field was static final and therefore JIT could constant-fold the Map instance), you could now annotate the field with `@Stable` annotation to allow JIT to produce similar code. I would also move the `Map.ofEntries(...)` expression into a separate private static method since I believe it has substantial bytecode size. `tryCanonicalize()` method bytecode size would then more likely fall below the inlining threshold. - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Wed, 10 Mar 2021 06:36:58 GMT, Jaikiran Pai wrote: >> Your code change Looks reasonable to me. Although i am not export in this >> area but what i observed is the test case which was attached by original >> submitter passes null to DynamicConstantDesc as follows >> "DynamicConstantDesc.of(null)". If you pass valid argument >> like(ConstantDescs.BSM_INVOKE) no deadlock. > >> but what i observed is the test case which was attached by original >> submitter passes null to DynamicConstantDesc as follows >> "DynamicConstantDesc.of(null)". If you pass valid argument >> like(ConstantDescs.BSM_INVOKE) no deadlock. > > Hello Vyom, > > The deadlock is reproducible with non-null params too. For example, the test > case in this PR consistently reproduces this deadlock when the fix in the > source code isn't applied. The `null` in the original report IMO was used > just to explain the issue. Any reviews please? - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Wed, 10 Mar 2021 05:23:51 GMT, Vyom Tewari wrote: > but what i observed is the test case which was attached by original submitter > passes null to DynamicConstantDesc as follows "DynamicConstantDesc.of(null)". > If you pass valid argument like(ConstantDescs.BSM_INVOKE) no deadlock. Hello Vyom, The deadlock is reproducible with non-null params too. For example, the test case in this PR consistently reproduces this deadlock when the fix in the source code isn't applied. The `null` in the original report IMO was used just to explain the issue. - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Wed, 10 Mar 2021 02:15:28 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed patch for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8263108? >> >> As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and >> `java.lang.constant.ConstantDescs` can end up in a classloading deadlock due >> to the nature of the code involved in their static blocks. A thread dump of >> one such deadlock (reproduced using the code attached to that issue) is as >> follows: >> >> "Thread A" #14 prio=5 os_prio=31 cpu=101.45ms elapsed=8.30s >> tid=0x7ff88e01ca00 nid=0x6003 in Object.wait() [0x7a4c6000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.DynamicConstantDesc.(java.base@16-ea/DynamicConstantDesc.java:67) >> - waiting on the Class initialization monitor for >> java.lang.constant.ConstantDescs >> at Deadlock.threadA(Deadlock.java:14) >> at Deadlock$$Lambda$1/0x000800c00a08.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> "Thread B" #15 prio=5 os_prio=31 cpu=103.15ms elapsed=8.30s >> tid=0x7ff88b936a00 nid=0x9b03 in Object.wait() [0x7a5c9000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.ClassDesc.ofDescriptor(java.base@16-ea/ClassDesc.java:145) >> - waiting on the Class initialization monitor for >> java.lang.constant.DynamicConstantDesc >> at >> java.lang.constant.ConstantDescs.(java.base@16-ea/ConstantDescs.java:239) >> at Deadlock.threadB(Deadlock.java:24) >> at Deadlock$$Lambda$2/0x000800c00c28.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> The commit in this PR resolves that deadlock by moving the `canonicalMap` >> initialization in `DynamicConstantDesc`, from the static block, to a lazily >> initialized map, into the `tryCanonicalize` (private) method of the same >> class. That's the only method which uses this map. >> >> The implementation in `tryCanonicalize` carefully ensures that the deadlock >> isn't shifted from the static block to this method, by making sure that the >> `synchronization` on `DynamicConstantDesc` in this method happens only when >> it's time to write the state to the shared `canonicalMap`. This has an >> implication that the method local variable `canonDescs` can get initialized >> by multiple threads unnecessarily but from what I can see, that's the only >> way we can avoid this deadlock. This penalty of multiple threads >> initializing and then throwing away that map isn't too huge because that >> will happen only once when the `canonicalMap` is being initialized for the >> first time. >> >> 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 commit in this PR also includes a jtreg testcase which (consistently) >> reproduces the deadlock without the fix and passes when this fix is applied. >> Extra manual testing has been done to verify that the classes of interest >> (noted in that testcase) are indeed getting loaded in those concurrent >> threads and not in the main thread. For future maintainers, there's a >> implementation note added on that testcase explaining why it cannot be moved >> into testng test. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Follow Peter Levart's review suggestion - remove volatile Your code change Looks reasonable to me. Although i am not export in this area but what i observed is the test case which was attached by original submitter passes null to DynamicConstantDesc as follows "DynamicConstantDesc.of(null)". If you pass valid argument like(ConstantDescs.BSM_INVOKE) no deadlock. - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
> Can I please get a review for this proposed patch for the issue reported in > https://bugs.openjdk.java.net/browse/JDK-8263108? > > As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and > `java.lang.constant.ConstantDescs` can end up in a classloading deadlock due > to the nature of the code involved in their static blocks. A thread dump of > one such deadlock (reproduced using the code attached to that issue) is as > follows: > > "Thread A" #14 prio=5 os_prio=31 cpu=101.45ms elapsed=8.30s > tid=0x7ff88e01ca00 nid=0x6003 in Object.wait() [0x7a4c6000] >java.lang.Thread.State: RUNNABLE > at > java.lang.constant.DynamicConstantDesc.(java.base@16-ea/DynamicConstantDesc.java:67) > - waiting on the Class initialization monitor for > java.lang.constant.ConstantDescs > at Deadlock.threadA(Deadlock.java:14) > at Deadlock$$Lambda$1/0x000800c00a08.run(Unknown Source) > at java.lang.Thread.run(java.base@16-ea/Thread.java:831) > > "Thread B" #15 prio=5 os_prio=31 cpu=103.15ms elapsed=8.30s > tid=0x7ff88b936a00 nid=0x9b03 in Object.wait() [0x7a5c9000] >java.lang.Thread.State: RUNNABLE > at > java.lang.constant.ClassDesc.ofDescriptor(java.base@16-ea/ClassDesc.java:145) > - waiting on the Class initialization monitor for > java.lang.constant.DynamicConstantDesc > at > java.lang.constant.ConstantDescs.(java.base@16-ea/ConstantDescs.java:239) > at Deadlock.threadB(Deadlock.java:24) > at Deadlock$$Lambda$2/0x000800c00c28.run(Unknown Source) > at java.lang.Thread.run(java.base@16-ea/Thread.java:831) > > The commit in this PR resolves that deadlock by moving the `canonicalMap` > initialization in `DynamicConstantDesc`, from the static block, to a lazily > initialized map, into the `tryCanonicalize` (private) method of the same > class. That's the only method which uses this map. > > The implementation in `tryCanonicalize` carefully ensures that the deadlock > isn't shifted from the static block to this method, by making sure that the > `synchronization` on `DynamicConstantDesc` in this method happens only when > it's time to write the state to the shared `canonicalMap`. This has an > implication that the method local variable `canonDescs` can get initialized > by multiple threads unnecessarily but from what I can see, that's the only > way we can avoid this deadlock. This penalty of multiple threads initializing > and then throwing away that map isn't too huge because that will happen only > once when the `canonicalMap` is being initialized for the first time. > > 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 commit in this PR also includes a jtreg testcase which (consistently) > reproduces the deadlock without the fix and passes when this fix is applied. > Extra manual testing has been done to verify that the classes of interest > (noted in that testcase) are indeed getting loaded in those concurrent > threads and not in the main thread. For future maintainers, there's a > implementation note added on that testcase explaining why it cannot be moved > into testng test. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Follow Peter Levart's review suggestion - remove volatile - Changes: - all: https://git.openjdk.java.net/jdk/pull/2893/files - new: https://git.openjdk.java.net/jdk/pull/2893/files/153139bc..1b59dc14 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2893=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2893=00-01 Stats: 5 lines in 1 file changed: 2 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2893.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2893/head:pull/2893 PR: https://git.openjdk.java.net/jdk/pull/2893
