Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8263108: Class initialization deadlock in java.lang.constant #2893

Closed
wants to merge 5 commits into from

Conversation

jaikiran
Copy link
Member

@jaikiran jaikiran commented Mar 9, 2021

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=0x00007ff88e01ca00 nid=0x6003 in Object.wait()  [0x000070000a4c6000]
   java.lang.Thread.State: RUNNABLE
	at java.lang.constant.DynamicConstantDesc.<clinit>(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/0x0000000800c00a08.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=0x00007ff88b936a00 nid=0x9b03 in Object.wait()  [0x000070000a5c9000]
   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.<clinit>(java.base@16-ea/ConstantDescs.java:239)
	at Deadlock.threadB(Deadlock.java:24)
	at Deadlock$$Lambda$2/0x0000000800c00c28.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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263108: Class initialization deadlock in java.lang.constant

Reviewers

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2893/head:pull/2893
$ git checkout pull/2893

To update a local copy of the PR:
$ git checkout pull/2893
$ git pull https://git.openjdk.java.net/jdk pull/2893/head

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 9, 2021

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 9, 2021
@openjdk
Copy link

openjdk bot commented Mar 9, 2021

@jaikiran The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@jaikiran
Copy link
Member Author

jaikiran commented Mar 9, 2021

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

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Mar 9, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 9, 2021

Webrevs

@plevart
Copy link
Contributor

plevart commented Mar 9, 2021

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

@jaikiran
Copy link
Member Author

Hello Peter,

Thank you for your inputs. I think what you suggest is a good idea. I have updated the PR with this change.

Copy link
Contributor

@vyommani vyommani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jaikiran
Copy link
Member Author

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.

@jaikiran
Copy link
Member Author

Any reviews please?

Copy link
Contributor

@plevart plevart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ChrisHegarty
Copy link
Member

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(..);
 }

@jaikiran
Copy link
Member Author

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)

@ChrisHegarty
Copy link
Member

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 )

@jaikiran
Copy link
Member Author

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.

@jaikiran
Copy link
Member Author

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.

Copy link
Contributor

@vyommani vyommani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@plevart
Copy link
Contributor

plevart commented Mar 16, 2021

Holder pattern is easier to understand, I agree. Perhaps you could just add a warning to the DynamicConstantDesc.ofCanonical() method javadoc/comment about what NOT to do in order to not fall into the deadlock trap again... (like: don't call this method from static initializer of ConstantDescs or such).
Otherwise it looks good to me to.

…ling DynamicConstantDesc.ofCanonical() from static initialization of java.lang.constant.ConstantDescs
@jaikiran
Copy link
Member Author

Perhaps you could just add a warning to the DynamicConstantDesc.ofCanonical() method javadoc/comment about what NOT to do in order to not fall into the deadlock trap again... (like: don't call this method from static initializer of ConstantDescs or such).

I think that's a good idea. I went ahead and added a regular comment at the top of that method to warn about this potential deadlock. I decided not to use a javadoc comment since IMO, this is too much of an internal implementation detail to end up being part of the javadoc.

I ran make docs-jdk after this change just to sure this comment doesn't cause any javadoc generation failures. The build went fine and the generated javadoc isn't impacted.

Copy link
Contributor

@plevart plevart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, Jaikiran.

@openjdk
Copy link

openjdk bot commented Mar 17, 2021

@jaikiran This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8263108: Class initialization deadlock in java.lang.constant

Reviewed-by: vtewari, plevart, chegar

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 151 new commits pushed to the master branch:

  • 24afa36: 8263726: divideToIntegralValue typo on BigDecimal documentation
  • cdf78e4: 8262298: G1BarrierSetC2::step_over_gc_barrier fails with assert "bad barrier shape"
  • 7674da4: 8262398: Shenandoah: Disable nmethod barrier and stack watermark when running with passive mode
  • 4f4ca0e: 8261671: X86 I2L conversion can be skipped for certain masked positive values
  • 5d87a21: 8263361: Incorrect arraycopy stub selected by C2 for SATB collectors
  • e152cc0: 8263677: Improve Character.isLowerCase/isUpperCase lookups
  • b63b5d4: 8263732: ProblemList serviceability/sa/ClhsdbSymbol.java on ZGC
  • 000012a: 8148937: (str) Adapt StringJoiner for Compact Strings
  • a707fcb: 8263723: [BACKOUT] MoveAndUpdateClosure::do_addr calls function with side-effects in an assert
  • 86e9cd9: 8263667: Avoid running GitHub actions on branches named pr/*
  • ... and 141 more: https://git.openjdk.java.net/jdk/compare/a0c3f2421862790cb64fb6d4a0d00c666345c090...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 17, 2021
@jaikiran
Copy link
Member Author

Thank you Peter, Chris and Vyom for the reviews. I'll go ahead and integrate this in a few hours.

@jaikiran
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Mar 18, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 18, 2021
@openjdk
Copy link

openjdk bot commented Mar 18, 2021

@jaikiran Since your change was applied there have been 156 commits pushed to the master branch:

  • 5d5813a: 8263757: Remove serviceability/sa/ClhsdClasses.java from ZGC problem list
  • 50ff0d4: 8263756: Fix ZGC ProblemList entry for serviceability/sa/ClhsdbSymbol.java
  • 99b39aa: 8262807: Note assumptions of core reflection modeling and parameter handling
  • 26234b5: 8254979: Class.getSimpleName() returns non-empty for lambda and method
  • 83a49ef: 8263753: two new tests from JDK-8261671 fail with "Error. can not find ClassFileInstaller in test directory or libraries"
  • 24afa36: 8263726: divideToIntegralValue typo on BigDecimal documentation
  • cdf78e4: 8262298: G1BarrierSetC2::step_over_gc_barrier fails with assert "bad barrier shape"
  • 7674da4: 8262398: Shenandoah: Disable nmethod barrier and stack watermark when running with passive mode
  • 4f4ca0e: 8261671: X86 I2L conversion can be skipped for certain masked positive values
  • 5d87a21: 8263361: Incorrect arraycopy stub selected by C2 for SATB collectors
  • ... and 146 more: https://git.openjdk.java.net/jdk/compare/a0c3f2421862790cb64fb6d4a0d00c666345c090...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9225a23.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants