> 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.
Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Update the testcase to improve the chances of triggering a deadlock ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/2893/files - new: https://git.openjdk.java.net/jdk/pull/2893/files/1b59dc14..2b51ec9d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2893&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2893&range=01-02 Stats: 15 lines in 1 file changed: 13 ins; 0 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
