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.
-------------
Commit messages:
- 8263108: Class initialization deadlock in java.lang.constant
Changes: https://git.openjdk.java.net/jdk/pull/2893/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2893&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8263108
Stats: 170 lines in 2 files changed: 160 ins; 7 del; 3 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
