RFR: 8263108: Class initialization deadlock in java.lang.constant [v5]

[Chris Hegarty]

On Tue, 16 Mar 2021 14:47:29 GMT, Jaikiran Pai <jpai at 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 at 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 at 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 at 16-ea/ClassDesc.java:145)
>> 	- waiting on the Class initialization monitor for java.lang.constant.DynamicConstantDesc
>> 	at java.lang.constant.ConstantDescs.<clinit>(java.base at 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 at 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