Re: RFR: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
On Tue, 15 Sep 2020 20:02:38 GMT, Coleen Phillimore wrote: >> The change fixes a 'deadlock' situation in >> VM_RedefineClasses::lock_classes() when the current thread is in the middle >> of redefining the same class. The change is based on the fix [1] suggested >> in the Jira issue [2] comment. >> [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/ >> [2] https://bugs.openjdk.java.net/browse/JDK-8241390 >> >> Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass. > > Changes requested by coleenp (Reviewer). The identifier 'cls' does not reflect the nature of the object and needs to be replaces with something like: classes, redef_classes or classes_list This impacts the files: jvmtiThreadState.hpp and jvmtiRedefineClasses.cpp. - PR: https://git.openjdk.java.net/jdk/pull/190
Re: RFR: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
On Tue, 15 Sep 2020 20:02:33 GMT, Coleen Phillimore wrote:
>> The change fixes a 'deadlock' situation in
>> VM_RedefineClasses::lock_classes() when the current thread is in the middle
>> of redefining the same class. The change is based on the fix [1] suggested
>> in the Jira issue [2] comment.
>> [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/
>> [2] https://bugs.openjdk.java.net/browse/JDK-8241390
>>
>> Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass.
>
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 159:
>
>> 157: if (!cls->contains(def_ik)) {
>> 158: def_ik->set_is_being_redefined(false);
>> 159: }
>
> Ok, so adding the Klass to the thread local list for each recursion works
> like ref counting. Can't think of a simpler
> way to do this. Looks good.
Yes, the same class can be pushed to the list multiple times (not more than
once by each recursive redefinition). It'd
make sense to add a comment about it as it is not obvious.
> test/jdk/java/lang/instrument/MakeAgentJAR.sh line 1:
>
>> 1: #!/bin/sh
>
> There are tests in test/hotspot/jtreg/serviceability/jvmti/RedefineClasses
> that don't use shell scripts that are much
> better. Can you add this test using that framework instead?
I'm second for this suggestion from Coleen to get rid of the shell script in
the test.
-
PR: https://git.openjdk.java.net/jdk/pull/190
Re: RFR: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
On Tue, 15 Sep 2020 16:43:01 GMT, Daniil Titov wrote:
> The change fixes a 'deadlock' situation in VM_RedefineClasses::lock_classes()
> when the current thread is in the middle
> of redefining the same class. The change is based on the fix [1] suggested in
> the Jira issue [2] comment.
> [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/
> [2] https://bugs.openjdk.java.net/browse/JDK-8241390
>
> Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass.
Changes requested by coleenp (Reviewer).
test/jdk/java/lang/instrument/MakeAgentJAR.sh line 1:
> 1: #!/bin/sh
There are tests in test/hotspot/jtreg/serviceability/jvmti/RedefineClasses that
don't use shell scripts that are much
better. Can you add this test using that framework instead?
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 159:
> 157: if (!cls->contains(def_ik)) {
> 158: def_ik->set_is_being_redefined(false);
> 159: }
Ok, so adding the Klass to the thread local list for each recursion works like
ref counting. Can't think of a simpler
way to do this. Looks good.
-
PR: https://git.openjdk.java.net/jdk/pull/190
RFR: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
The change fixes a 'deadlock' situation in VM_RedefineClasses::lock_classes() when the current thread is in the middle of redefining the same class. The change is based on the fix [1] suggested in the Jira issue [2] comment. [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/ [2] https://bugs.openjdk.java.net/browse/JDK-8241390 Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass. - Commit messages: - 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes() Changes: https://git.openjdk.java.net/jdk/pull/190/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=190=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8241390 Stats: 202 lines in 6 files changed: 193 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/190.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/190/head:pull/190 PR: https://git.openjdk.java.net/jdk/pull/190
