On Tue, 9 Mar 2021 13:50:05 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. > >> 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 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 ------------- PR: https://git.openjdk.java.net/jdk/pull/2893
