On Tue, 16 Mar 2021 14:47:29 GMT, Jaikiran Pai <j...@openjdk.org> 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=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: > > Add a comment to instruct future maintainers of the code to avoid calling > DynamicConstantDesc.ofCanonical() from static initialization of > java.lang.constant.ConstantDescs Marked as reviewed by chegar (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/2893
