Re: RFR (S) 8220512: Deoptimize redefinition functions that have dirty ICs
On 3/14/19 2:24 PM, serguei.spit...@oracle.com wrote: Hi Coleen, It looks good to me. Just one minor suggestion: http://cr.openjdk.java.net/~coleenp/2019/8220512.01/webrev/src/hotspot/share/classfile/metadataOnStackMark.cpp.udiff.html - Threads::metadata_do(Metadata::mark_on_stack); - CodeCache::metadata_do(Metadata::mark_on_stack); + MetadataOnStackClosure mon_stack; + Threads::metadata_do(&mon_stack); + CodeCache::metadata_do(&mon_stack); The 'mon_stack' can be associated with monitors. How about to rename it to something like 'md_on_stack'? Okay, I'll change the name to md_on_stack. Thanks for the code review! Coleen Thanks, Serguei On 3/14/19 10:40, coleen.phillim...@oracle.com wrote: Summary: Walk ICs to determine whether nmethods are dependent on redefined classes. See bug for more details. Tested with redefinition tests: #redefinition tests. make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jvmti >&jvmti.out make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jdi >&jdi.out make test TEST=open/test/hotspot/jtreg/runtime/RedefineTests >&redefine.out make test TEST=open/test/jdk/java/lang/instrument >&instrument.out make test TEST=open/test/jdk/com/sun/jdi >&jtreg.jdi.out hs-tier1-6 as well as java/lang/instrument tests with -Xcomp. open webrev at http://cr.openjdk.java.net/~coleenp/2019/8220512.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8220512 Thanks, Coleen
Re: RFR (S) 8220512: Deoptimize redefinition functions that have dirty ICs
Hi Coleen, It looks good to me. Just one minor suggestion: http://cr.openjdk.java.net/~coleenp/2019/8220512.01/webrev/src/hotspot/share/classfile/metadataOnStackMark.cpp.udiff.html -Threads::metadata_do(Metadata::mark_on_stack); -CodeCache::metadata_do(Metadata::mark_on_stack); +MetadataOnStackClosure mon_stack; +Threads::metadata_do(&mon_stack); +CodeCache::metadata_do(&mon_stack); The 'mon_stack' can be associated with monitors. How about to rename it to something like 'md_on_stack'? Thanks, Serguei On 3/14/19 10:40, coleen.phillim...@oracle.com wrote: Summary: Walk ICs to determine whether nmethods are dependent on redefined classes. See bug for more details. Tested with redefinition tests: #redefinition tests. make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jvmti >&jvmti.out make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jdi >&jdi.out make test TEST=open/test/hotspot/jtreg/runtime/RedefineTests >&redefine.out make test TEST=open/test/jdk/java/lang/instrument >&instrument.out make test TEST=open/test/jdk/com/sun/jdi >&jtreg.jdi.out hs-tier1-6 as well as java/lang/instrument tests with -Xcomp. open webrev at http://cr.openjdk.java.net/~coleenp/2019/8220512.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8220512 Thanks, Coleen
Re: RFR: 8220579: [Containers] SubSystem.java out of sync with osContainer_linux.cpp
On Thu, 2019-03-14 at 13:58 -0400, Bob Vandette wrote: > The change looks good. Thanks for fixing this. Thanks for the review, Bob! Cheers, Severin > I’d send this to core-libs (cc’d). > > Bob. > > > > On Mar 14, 2019, at 12:51 PM, Severin Gehwolf > > wrote: > > > > Hi, > > > > I'm not sure what the right list for Metrics.java[1] is. Assuming > > it's > > serviceability-dev: > > > > Please review this one-liner for for SubSystem.java which currently > > behaves differently from the native implementation in > > osContainer_linux.cpp. Please see the details in the bug. > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8220579 > > webrev: > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8220579/01/webrev/ > > > > Testing: > > Manual testing of JDK-8217338 with Metrics.java support > > with/without > > this fix on Linux x86_64. Metrics tests and Docker tests continue > > to > > pass for fastdebug jvms (NOT for release jvms. see JDK-8220674, > > which > > was fun). > > > > Thoughts? > > > > Thanks, > > Severin > > > > [1] > > http://hg.openjdk.java.net/jdk/jdk/file/641768acb12e/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/Metrics.java > >
Re: RFR: 8220579: [Containers] SubSystem.java out of sync with osContainer_linux.cpp
The change looks good. Thanks for fixing this. I’d send this to core-libs (cc’d). Bob. > On Mar 14, 2019, at 12:51 PM, Severin Gehwolf wrote: > > Hi, > > I'm not sure what the right list for Metrics.java[1] is. Assuming it's > serviceability-dev: > > Please review this one-liner for for SubSystem.java which currently > behaves differently from the native implementation in > osContainer_linux.cpp. Please see the details in the bug. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8220579 > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8220579/01/webrev/ > > Testing: > Manual testing of JDK-8217338 with Metrics.java support with/without > this fix on Linux x86_64. Metrics tests and Docker tests continue to > pass for fastdebug jvms (NOT for release jvms. see JDK-8220674, which > was fun). > > Thoughts? > > Thanks, > Severin > > [1] > http://hg.openjdk.java.net/jdk/jdk/file/641768acb12e/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/Metrics.java >
RFR (S) 8220512: Deoptimize redefinition functions that have dirty ICs
Summary: Walk ICs to determine whether nmethods are dependent on redefined classes. See bug for more details. Tested with redefinition tests: #redefinition tests. make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jvmti >&jvmti.out make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jdi >&jdi.out make test TEST=open/test/hotspot/jtreg/runtime/RedefineTests >&redefine.out make test TEST=open/test/jdk/java/lang/instrument >&instrument.out make test TEST=open/test/jdk/com/sun/jdi >&jtreg.jdi.out hs-tier1-6 as well as java/lang/instrument tests with -Xcomp. open webrev at http://cr.openjdk.java.net/~coleenp/2019/8220512.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8220512 Thanks, Coleen
Re: RFR: JDK-8220678: unquarantine nsk/jdi/ThreadReference/setEnabled/setenabled003
+1 On 3/14/19 10:24 AM, serguei.spit...@oracle.com wrote: Okay. Thanks, Serguei On 3/14/19 10:22, Gary Adams wrote: This trivial changeset will restore setenabled003 testing to jdk 13 where the test does not appear to be failing. The original bug JDK-8066993 will be left open in case sustaining team needs to apply changes in older releases. diff --git a/test/hotspot/jtreg/ProblemList.txt b/test/hotspot/jtreg/ProblemList.txt --- a/test/hotspot/jtreg/ProblemList.txt +++ b/test/hotspot/jtreg/ProblemList.txt @@ -158,7 +158,6 @@ vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java 8060733 generic-all vmTestbase/nsk/jdi/ThreadReference/stop/stop001/TestDescription.java 7034630 generic-all -vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003/TestDescription.java 8066993 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 8065773 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 8065773 generic-all
Re: RFR: JDK-8220678: unquarantine nsk/jdi/ThreadReference/setEnabled/setenabled003
Okay. Thanks, Serguei On 3/14/19 10:22, Gary Adams wrote: This trivial changeset will restore setenabled003 testing to jdk 13 where the test does not appear to be failing. The original bug JDK-8066993 will be left open in case sustaining team needs to apply changes in older releases. diff --git a/test/hotspot/jtreg/ProblemList.txt b/test/hotspot/jtreg/ProblemList.txt --- a/test/hotspot/jtreg/ProblemList.txt +++ b/test/hotspot/jtreg/ProblemList.txt @@ -158,7 +158,6 @@ vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java 8060733 generic-all vmTestbase/nsk/jdi/ThreadReference/stop/stop001/TestDescription.java 7034630 generic-all -vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003/TestDescription.java 8066993 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 8065773 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 8065773 generic-all
RFR: JDK-8220678: unquarantine nsk/jdi/ThreadReference/setEnabled/setenabled003
This trivial changeset will restore setenabled003 testing to jdk 13 where the test does not appear to be failing. The original bug JDK-8066993 will be left open in case sustaining team needs to apply changes in older releases. diff --git a/test/hotspot/jtreg/ProblemList.txt b/test/hotspot/jtreg/ProblemList.txt --- a/test/hotspot/jtreg/ProblemList.txt +++ b/test/hotspot/jtreg/ProblemList.txt @@ -158,7 +158,6 @@ vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java 8060733 generic-all vmTestbase/nsk/jdi/ThreadReference/stop/stop001/TestDescription.java 7034630 generic-all -vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003/TestDescription.java 8066993 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 8065773 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 8065773 generic-all
RFR: 8220579: [Containers] SubSystem.java out of sync with osContainer_linux.cpp
Hi, I'm not sure what the right list for Metrics.java[1] is. Assuming it's serviceability-dev: Please review this one-liner for for SubSystem.java which currently behaves differently from the native implementation in osContainer_linux.cpp. Please see the details in the bug. Bug: https://bugs.openjdk.java.net/browse/JDK-8220579 webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8220579/01/webrev/ Testing: Manual testing of JDK-8217338 with Metrics.java support with/without this fix on Linux x86_64. Metrics tests and Docker tests continue to pass for fastdebug jvms (NOT for release jvms. see JDK-8220674, which was fun). Thoughts? Thanks, Severin [1] http://hg.openjdk.java.net/jdk/jdk/file/641768acb12e/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/Metrics.java
Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't
Hi Daniel, On Thu, 2019-03-14 at 11:24 +, Daniel Fuchs wrote: > Hi Severin, > > If you can update WAIT_FOR_JMX_AGENT_TIMEOUT_MS in > JMXAgentInterfaceBinding.java > to 2, in you webrev.04, then I believe you should > be good to go and push the changes. > > This seems to fix the instability I had observed with > fastdebug VMs. Great! It works fine for me on my box now too. 100 iterations no failure. I'm going to push it. Thanks again for your help! Thanks, Severin > > best regards, > > -- daniel > > On 12/03/2019 18:22, Daniel Fuchs wrote: > > > This is what I have tried: > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/04/webrev/ > > > > > > If you think we should try this one. That's fine with me. > > > > I have now also tested webrev.04 with a test config that uses > > the fastdebug VM and observed many intermittent failures. > > The logs let me think that maybe the JMX agent needed to be > > given more time to come up: > > > > So I am now experimenting with your webrev.04 above - but with the > > following additional change to JMXAgentInterfaceBinding.java: > > > > private static final int WAIT_FOR_JMX_AGENT_TIMEOUT_MS = > > 2; > > > > So far the result are looking better, but tests are still running. > > I wonder whether the same change would also reduce the number of > > intermittent failures on your box? > > > > best regards, > > > > -- daniel
Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't
Hi Severin, If you can update WAIT_FOR_JMX_AGENT_TIMEOUT_MS in JMXAgentInterfaceBinding.java to 2, in you webrev.04, then I believe you should be good to go and push the changes. This seems to fix the instability I had observed with fastdebug VMs. best regards, -- daniel On 12/03/2019 18:22, Daniel Fuchs wrote: This is what I have tried: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/04/webrev/ If you think we should try this one. That's fine with me. I have now also tested webrev.04 with a test config that uses the fastdebug VM and observed many intermittent failures. The logs let me think that maybe the JMX agent needed to be given more time to come up: So I am now experimenting with your webrev.04 above - but with the following additional change to JMXAgentInterfaceBinding.java: private static final int WAIT_FOR_JMX_AGENT_TIMEOUT_MS = 2; So far the result are looking better, but tests are still running. I wonder whether the same change would also reduce the number of intermittent failures on your box? best regards, -- daniel
Re: RFR/RFC: 8220342: Remove scavenge_root_nmethods_do from VM_HeapWalkOperation::collect_simple_roots
Thanks, Serguei! StefanK On 2019-03-12 22:50, serguei.spit...@oracle.com wrote: Hi Stefan, The fix looks good to me. Testing the tiers 1-7 for different GC's has to be good enough. Thanks, Serguei On 3/12/19 8:19 AM, Stefan Karlsson wrote: Hi all, Please review and/or comment on this change to remove CodeCache::scavenge_root_nmehods_do from VM_HeapWalkOperation::collect_simple_roots. http://cr.openjdk.java.net/~stefank/8220342/webrev.01/ https://bugs.openjdk.java.net/browse/JDK-8220342 VM_HeapWalkOperation::collect_simple_roots is used to implement the following JVMTI functionality: IterateOverReachableObjects IterateOverObjectsReachableFromObject FollowReferences From: https://docs.oracle.com/en/java/javase/11/docs/specs/jvmti.html#FollowReferences "This function initiates a traversal over the objects that are directly and indirectly reachable from the specified object or, if initial_object is not specified, all objects reachable from the heap roots. The heap root are the set of system classes, JNI globals, references from thread stacks, and other objects used as roots for the purposes of garbage collection." The set of roots in collect_simple_roots matches this, and mostly visits the set of roots that one of our class unloading enabled GCs would visit. There are some roots missing. For example: Management::oops_do JvmtiExport::oops_do AOTLoader::oops_do And there's one set of roots that is present in collect_simple_roots, that is not visited by our unloading GCs: CodeCache::scavenge_root_nmethods_do As an example, in PSMarkSweep we have the following comment in the root scanning code: // Do not treat nmethods as strong roots for mark/sweep, since we can unload them. //CodeCache::scavenge_root_nmethods_do(...); The CodeCache::scavenge_root_nmethods_do is only used by Serial, Parallel, and CMS, to scan pointers into young gen. Other GCs don't use it at all, and if we run with G1 this call does nothing at all. CodeCache::scavenge_root_nmethods_do is an GC implementation detail that I want to confine with the following RFE: https://bugs.openjdk.java.net/browse/JDK-8220343 "Move scavenge_root_nmethods from shared code" and this is the only external usage of it. Note also that the only effect of this code is that it adds a set of roots that point into the young gen, but only for some of our GCs. There are other roots that also point into the young gen that we don't visit. For example, the non-system classes. See how collect_simple_roots use ClassLoaderDataGraph::always_strong_cld_do instead of ClassLoaderDataGraph::cld_do. I've run through tier1-7 with this removal, without any problems. I'd be interested in hearing if others have a justification for having this code in collect_simple_roots. Or a test-case showing why this is needed. There has been some brief, internal discussions that maybe we want to visit all sets of roots in the vm, both strong and weak. A quick implementation of that causes problem in testing when objects tagged by JVMTI, and JNI weak global handles, gets reported as roots. Because of that, such change requires more investigation and work than simply extending the set of roots. However, if one were to go that route the above call to CodeCache::scavenge_root_nmethods_do would be replaced with CodeCache::blobs_do, the function used when we turn off class unloading and use our weak roots as strong roots. As an example, see GenCollectedHeap::process_roots: // CMSCollector uses this to do intermediate-strength collections. // We scan the entire code cache, since CodeCache::do_unloading is not called. CodeCache::blobs_do(code_roots); Thanks, StefanK
Re: RFR/RFC: 8220342: Remove scavenge_root_nmethods_do from VM_HeapWalkOperation::collect_simple_roots
On 2019-03-14 10:21, Erik Helin wrote: On 12 Mar 2019, at 16:19, Stefan Karlsson wrote: Hi all, Hey StefanK, Please review and/or comment on this change to remove CodeCache::scavenge_root_nmehods_do from VM_HeapWalkOperation::collect_simple_roots. http://cr.openjdk.java.net/~stefank/8220342/webrev.01/ I think this patch makes sense, removing the call CodeCache::scavenge_root_nmethods_do makes the semantics of the JVMTI operations you mentioned much more clear. Reviewed. Thanks! On 12 Mar 2019, at 16:19, Stefan Karlsson wrote: There has been some brief, internal discussions that maybe we want to visit all sets of roots in the vm, both strong and weak. A quick implementation of that causes problem in testing when objects tagged by JVMTI, and JNI weak global handles, gets reported as roots. Because of that, such change requires more investigation and work than simply extending the set of roots. It would be nice if we could add a note somewhere about how HotSpot interprets “...and other objects used as roots for the purposes of garbage collection” in the JVMTI specification. As you have highlighted, and as we discussed, this wording in the JVMTI specification is not precise enough. Different GC algorithms (and different phases of a single GC algorithm) often have different root sets, rendering the wording “…objects used as roots for the purpose of garbage collection” unclear, since the “objects used as roots” can differ depending on GC algorithm and phase. An implementation of the JVMTI specification has to interpret this sentence and choose which objects to include in the root set for these JVMTI operations. I think it would be helpful for HotSpot JVMTI users if there is a place where we could document which objects HotSpot includes in this root set (we should preferably use the same set of root objects for all GC algorithms). Should we create a hotspot/jvmti RFE for this? StefanK Thanks, Erik
Re: RFR/RFC: 8220342: Remove scavenge_root_nmethods_do from VM_HeapWalkOperation::collect_simple_roots
> On 12 Mar 2019, at 16:19, Stefan Karlsson wrote: > > Hi all, Hey StefanK, > Please review and/or comment on this change to remove > CodeCache::scavenge_root_nmehods_do from > VM_HeapWalkOperation::collect_simple_roots. > > http://cr.openjdk.java.net/~stefank/8220342/webrev.01/ I think this patch makes sense, removing the call CodeCache::scavenge_root_nmethods_do makes the semantics of the JVMTI operations you mentioned much more clear. Reviewed. > On 12 Mar 2019, at 16:19, Stefan Karlsson wrote: > There has been some brief, internal discussions that maybe we want to visit > all sets of roots in the vm, both strong and weak. A quick > implementation of that causes problem in testing when objects tagged by > JVMTI, and JNI weak global handles, gets reported as roots. > Because of that, such change requires more investigation and work than simply > extending the set of roots. It would be nice if we could add a note somewhere about how HotSpot interprets “...and other objects used as roots for the purposes of garbage collection” in the JVMTI specification. As you have highlighted, and as we discussed, this wording in the JVMTI specification is not precise enough. Different GC algorithms (and different phases of a single GC algorithm) often have different root sets, rendering the wording “…objects used as roots for the purpose of garbage collection” unclear, since the “objects used as roots” can differ depending on GC algorithm and phase. An implementation of the JVMTI specification has to interpret this sentence and choose which objects to include in the root set for these JVMTI operations. I think it would be helpful for HotSpot JVMTI users if there is a place where we could document which objects HotSpot includes in this root set (we should preferably use the same set of root objects for all GC algorithms). Thanks, Erik
