Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Dear All, Sorry to bother again, I just want to make sure that is this change worth to be continue to work on? If decision is made to not. I think I can drop this work and stop asking for help reviewing... Thanks for all your help about reviewing this previously. BRs, Lin On 2020/5/9, 3:47 PM, "linzang(臧琳)" wrote: Dear All, May I ask your help again for review the latest change? Thanks! BRs, Lin On 2020/4/28, 1:54 PM, "linzang(臧琳)" wrote: Hi Stefan, >> - Adding Atomic::load/store. >> - Removing the time measurement in the run_task. I renamed G1's function >> to run_task_timed. If we need this outside of G1, we can rethink the API >> at that point. >> - ZGC style cleanups Thanks for revising the patch, they are all good to me, and I have made a tiny change based on it: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_04/ http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_04-delta/ it reduce the scope of mutex in ParHeapInspectTask, and delete unnecessary comments. BRs, Lin On 2020/4/27, 4:34 PM, "Stefan Karlsson" wrote: Hi Lin, On 2020-04-26 05:10, linzang(臧琳) wrote: > Hi Stefan and Paul, > I have made a new patch based on your comments and Stefan's Poc code: > Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03/ > Delta(based on Stefan's change:) : http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03-delta/webrev_03-delta/ Thanks for providing a delta patch. It makes it much easier to look at, and more likely for reviewers to continue reviewing. I'm going to continue focusing on the GC parts, and leave the rest to others to review. > > And Here are main changed I made and want to discuss with you: > 1. changed"parallelThreadNum=" to "parallel=" for jmap -histo options. > 2. Add logic to test where parallelHeapInspection is fail, in heapInspection.cpp >This is because the parHeapInspectTask create thread local KlassInfoTable in it's work() method, and this may fail because of native OOM, in this case, the parallel should fail and serial heap inspection can be tried. >One more thing I want discuss with you is about the member "_success" of parHeapInspectTask, when native OOM happenes, it is set to false. And since this "set" operation can be conducted in multiple threads, should it be atomic ops? IMO, this is not necessary because "_success" can only be set to false, and there is no way to change it from back to true after the ParHeapInspectTask instance is created, so it is save to be non-atomic, do you agree with that? In these situations you should be using the Atomic::load/store primitives. We're moving toward a later C++ standard were data races are considered undefined behavior. > 3. make CollectedHeap::run_task() be an abstract virtual func, so that every subclass of collectedHeap should support it, so later implementation of new collectedHeap will not miss the "parallel" features. > The problem I want to discuss with you is about epsilonHeap and SerialHeap, as they may not need parallel heap iteration, so I only make task->work(0), in case the run_task() is invoked someway in future. Another way is to left run_task() unimplemented, which one do you think is better? I don't have a strong opinion about this. And also please help take a look at the zHeap, as there is a class zTask that wrap the abstractGangTask, and the collectedHeap::run_task() only accept AbstraceGangTask* as argument, so I made a delegate class to adapt it , please see src/hotspot/share/gc/z/zHeap.cpp. > >There maybe other better ways to sovle the above problems, welcome for any comments, Thanks! I've created a few cleanups and changes on top of your latest patch: https://cr.openjdk.java.net/~stefank/8215624/webrev.02.delta https://cr.openjdk.java.net/~stefank/8215624/webrev.02 - Adding Atomic::load/store. - Removing the time measurement in the run_task. I renamed G1's function to run_task_timed. If we need this outside of G1, we can rethink the API at that point. - ZGC style cleanups Thanks, StefanK > > BRs, > Lin > > On 2020/4/23, 11:08 AM, "linzang(臧琳)" wrote: > > Thanks Paul! I agree with us
Re: RFR (Preliminary): 8248194: Need better support for running SA tests on core files
Hi Leonid, I'm starting to think that this should all go in a new CoreUtils.java file. I experimented with moving getCoreFileLocation() to OutputAnalyzer. It worked, but one adjustment I had to make is also moving SATestUtils.unzipCores() there also and make it private (no one else calls it). But that just got me thinking that maybe CoreUtils.java would be a better solution. I think I would put the addCoreUlimitCommand() API discussed below there also. What do you think? thanks, Chris On 6/28/20 5:29 PM, Chris Plummer wrote: Hi Leonid, I think getCoreFileLocation() can simply move to OutputAnalyzer. No need for it to be in SAUtils and be passed the String argument that comes from OutputAnalyzer.getOutput(). For the ulimit support, how about if in ProcessTools I add: public static ProcessBuilder addCoreUlimitCommand(ProcessBuilder pb); All the ulimit logic would move there from SATestUtils. It's straight forward to use this API in LingeredApp and TestJmapCore. For ClhsdbCDSCore I'll need to rework the getTestJvmCommandlineWithPrefix() code a bit, since it creates a pb, but doesn't save it. It only uses it to get the cmd String. Also, there's one new finding since I sent out the review. I found the following in CiReplayBase.java: // lets search few possible locations using process output and return existing location private String getCoreFileLocation(String crashOutputString) { This is identical to the code I pulled from ClhsdbCDSCore and is now in SATestUtils.parseCoreFileLocationFromOutput(). Although this is in the compiler directory, it is in fact an SA test that uses clhsdb, although directly via the CLHSDB class rather than through "jhsdb clhsdb". This also explains why ClhsdbCDSCore had some logic to move and rename the core file to "cds_core_file". I removed this logic because it seemed unnecessary, but for CiReplayBase.java it needs to be in a known location so SABase.java can find it. It's still fine for ClhsdbCDSCore to not do the rename, and renaming is independent of any code that locates the core file. I'm not going to update CiReplayBase.java as part of these changes because the two tests that use it both have issues. TestSAServer is problem listed, and when I removed it from the problem list it failed with every run on every platform. There's also TestSAClient, but it relies on client VM, which we don't support anymore. So with neither of these tests running, I'd rather not introduce changes I can't really test. However, there was something good that came out of the CiReplayBase.java discovery. I had previously noted that ClhsdbCDSCore is excluded from running on windows. When I removed the @requires for this, it failed for a reason I didn't quite understand. The complaint was about the path to java.exe when running the process that was suppose to crash, although the path looked fine. However, I found that TestSAServer ran fine on Windows, even though it was basically the process launching code for causing the crash. I looked closer and found one difference. In getTestJvmCommandlineWithPrefix(), which both tests have, the CiReplayBase version had some extra code for Windows: return new String[]{"sh", "-c", prefix + (Platform.isWindows() ? cmd.replace('\\', '/').replace(";", "\\;").replace("|", "\\|") : cmd)}; So on Windows it's doing a path conversion. Once I started doing the same with ClhsdbCDSCore, it started to run fine on Windwos also. thanks, Chris On 6/26/20 8:42 PM, Chris Plummer wrote: Hi Leonid, On 6/26/20 7:51 PM, Leonid Mesnik wrote: Hi The idea basically looks good. I think it just make a sense to polish it a little bit to hide "sh" usage from test and get core from OutputAnalyzer. Ok, I'll look into both of those. Also, there is a 'CrashApp' in ClhsdbCDSCore.java. Makes it sense to unify it with LingeredApp crasher? Currently, it uses Unsafe to crash application. Yes, I purposely didn't not make that change. My main goal with the LingeredApp changes is to make it easier to make existing LingeredApp SA tests run on both a live process and on a core, and my main goal with ClhsdbCDSCore and TestJmapCore was to move the core finding code and ulimit code to a common location that could be reused by other tests. Keep in mind that ClhsdbLauncher and LingeredApp are independent of each other. You can have a LingeredApp tests that use or don't use ClhsdbLauncher, and you can have a non-LingeredApp tests that use or don't use ClhsdbLauncher. So I didn't want to go down the path of changing ClhsdbCDSCore (a non LingeredApp test) to use LingeredApp. Likewise I did not change TestJmapCore to use LingeredApp or ClhsdbLauncher. Possibly there is good reason to convert some of the tests to start using LingeredApp and/or ClhsdbLauncher, but that should be done under a separate RFE. Also, crashes are used in other tests, I see some implementations in open/t
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Hi David, Serguei, I updated webrev for 8242428. Could you review again? This change migrate to use direct handshake for GetStackTrace() and GetThreadListStackTraces() (when thread_count == 1). http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/ VM_GetThreadListStackTrace (for GetThreadListStackTraces) and VM_GetAllStackTraces (for GetAllStackTraces) have inherited VM_GetMultipleStackTraces VM operation which provides the feature to generate jvmtiStackInfo. I modified VM_GetMultipleStackTraces to a normal C++ class to share with HandshakeClosure for GetThreadListStackTraces (GetSingleStackTraceClosure). Also I added new testcases which test GetThreadListStackTraces() with thread_count == 1 and with all threads. This change has been tested in serviceability/jvmti serviceability/jdwp vmTestbase/nsk/jvmti vmTestbase/nsk/jdi vmTestbase/nsk/jdwp. Thanks, Yasumasa On 2020/06/24 15:50, Yasumasa Suenaga wrote: Hi all, Please review this change: JBS: https://bugs.openjdk.java.net/browse/JDK-8242428 webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/ This change replace following VM operations to direct handshake. - VM_GetFrameCount (GetFrameCount()) - VM_GetFrameLocation (GetFrameLocation()) - VM_GetThreadListStackTraces (GetThreadListStackTrace()) - VM_GetCurrentLocation GetThreadListStackTrace() uses direct handshake if thread count == 1. In other case (thread count > 1), it would be performed as VM operation (VM_GetThreadListStackTraces). Caller of VM_GetCurrentLocation (JvmtiEnvThreadState::reset_current_location()) might be called at safepoint. So I added safepoint check in its caller. This change has been tested in serviceability/jvmti serviceability/jdwp vmTestbase/nsk/jvmti vmTestbase/nsk/jdi vmTestbase/ns k/jdwp. Also I tested it on submit repo, then it has execution error (mach5-one-ysuenaga-JDK-8242428-20200624-0054-12034717) due to dependency error. So I think it does not occur by this change. Thanks, Yasumasa
Re: RFR(S): 8246493: JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support
I'll add a note to 8246493. Dan On 6/29/20 5:02 PM, Chris Plummer wrote: Ok, in that case it sounds best not to backport. It would be best to make this clear in the bug so there is no future attempt to backport this change except to versions that have already done the WhiteBox.deflateIdleMonitors() backport. thanks, Chris On 6/29/20 1:53 PM, Daniel D. Daugherty wrote: The WhiteBox.deflateIdleMonitors() support is not in JDK15. That's something that added in JDK16 so I'd have to also backport that support. That support was included with another change (getting rid of the special deflation request mechanism) that is not appropriate for JDK15. Short version: I don't think we want to back port part of a patch from JDK16 -> JDK15 in order to fix this test bug. Dan On 6/29/20 4:41 PM, Chris Plummer wrote: Hi Dan, I think you should push it directly to 15 since it's a new issue. thanks, Chris On 6/29/20 12:53 PM, Daniel D. Daugherty wrote: Chris, Thanks. One last thing... since this is a test bug, I wasn't planning to backport the fix to JDK15. The test is ProblemListed there so we won't see the intermittent failures. Are you and Serguei good with not fixing this test bug in JDK15? Dan P.S. Thanks again for your sleuthing that linked the bug to my fix for JDK-8153224. On 6/29/20 3:49 PM, Chris Plummer wrote: Looks good. Chris On 6/29/20 12:45 PM, Daniel D. Daugherty wrote: Chris and Serguei, Thanks for the fast reviews!! I generated the webrev in my "mach5" directory and that was baselined on the jdk-16+3 snapshot and that doesn't include the ProblemList change. Sigh... I have updated the repo to "current" and regenerated the webrev. test/hotspot/jtreg/ProblemList.txt now shows: @@ -126,11 +126,10 @@ 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/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 8065773 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 8065773 generic-all -vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java 8246493 generic-all vmTestbase/nsk/jdb/eval/eval001/eval001.java 8221503 generic-all vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 generic-all vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 generic-all Thanks again for the fast reviews!! Dan On 6/29/20 3:41 PM, serguei.spit...@oracle.com wrote: Hi Dan, The same as from Chris. The ProblemList.txt has no changes. Otherwise, it looks good. Thanks, Serguei On 6/29/20 12:37, Chris Plummer wrote: Hi Dan, Something is wrong with ProblemList.txt. It doesn't show any changes, but I also don't see mixed002 in the file anymore. Otherwise the changes look good. thanks, Chris On 6/29/20 12:21 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following bug: JDK-8246493 JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support https://bugs.openjdk.java.net/browse/JDK-8246493 Here's the webrev URL: http://cr.openjdk.java.net/~dcubed/8246493-webrev/0_for_jdk16/ The test bug that's being fixed: vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java fails intermittently with the following message: nsk.share.TestBug: There are more than one(2) instance of 'nsk.share.jpda.StateTestThread in debuggee Summary of the fix: Use WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This fix has been tested with a Mach5 Tier5 test run that executes all of the JDI tests (along with JDWP, JVM/TI and other Serviceability tests). I also did five 100 iteration runs of the failing mix002 test. Each Mach5 job set ran the test 100 times on Linux-X64, macOSX, and Win-X64 for a total of (5 * 100 * 3) iterations of nsk/jdi/stress/serial/mixed002. There were no failures. Thanks, in advance, for any comments, questions or suggestions. Dan Gory details: The primary focus of the fix is in the first three files in the webrev: test/hotspot/jtreg/vmTestbase/nsk/share/jdi/SerialExecutionDebuggee.java test/hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java test/hotspot/jtreg/ProblemList.txt nsk.share.jdi.SerialExecutionDebuggee is the class that used to serially execute the debuggee portion of a specific list of tests. After this class is done executing a debuggee class, it needs to deflate idle monitors in order to prevent a StateTestThread object created by one debuggee class from confusing the next debuggee class. Each of the debuggee classes that use StateTestThread expect there to be only one of these objects. However, since we are running multiple debuggee classes serially *in the same VM*, the StateTestThread
Re: RFR(S): 8246493: JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support
Thanks for review! Dan On 6/29/20 5:16 PM, serguei.spit...@oracle.com wrote: +1 Thanks, Serguei On 6/29/20 12:49, Chris Plummer wrote: Looks good. Chris On 6/29/20 12:45 PM, Daniel D. Daugherty wrote: Chris and Serguei, Thanks for the fast reviews!! I generated the webrev in my "mach5" directory and that was baselined on the jdk-16+3 snapshot and that doesn't include the ProblemList change. Sigh... I have updated the repo to "current" and regenerated the webrev. test/hotspot/jtreg/ProblemList.txt now shows: @@ -126,11 +126,10 @@ 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/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 8065773 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 8065773 generic-all -vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java 8246493 generic-all vmTestbase/nsk/jdb/eval/eval001/eval001.java 8221503 generic-all vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 generic-all vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 generic-all Thanks again for the fast reviews!! Dan On 6/29/20 3:41 PM, serguei.spit...@oracle.com wrote: Hi Dan, The same as from Chris. The ProblemList.txt has no changes. Otherwise, it looks good. Thanks, Serguei On 6/29/20 12:37, Chris Plummer wrote: Hi Dan, Something is wrong with ProblemList.txt. It doesn't show any changes, but I also don't see mixed002 in the file anymore. Otherwise the changes look good. thanks, Chris On 6/29/20 12:21 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following bug: JDK-8246493 JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support https://bugs.openjdk.java.net/browse/JDK-8246493 Here's the webrev URL: http://cr.openjdk.java.net/~dcubed/8246493-webrev/0_for_jdk16/ The test bug that's being fixed: vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java fails intermittently with the following message: nsk.share.TestBug: There are more than one(2) instance of 'nsk.share.jpda.StateTestThread in debuggee Summary of the fix: Use WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This fix has been tested with a Mach5 Tier5 test run that executes all of the JDI tests (along with JDWP, JVM/TI and other Serviceability tests). I also did five 100 iteration runs of the failing mix002 test. Each Mach5 job set ran the test 100 times on Linux-X64, macOSX, and Win-X64 for a total of (5 * 100 * 3) iterations of nsk/jdi/stress/serial/mixed002. There were no failures. Thanks, in advance, for any comments, questions or suggestions. Dan Gory details: The primary focus of the fix is in the first three files in the webrev: test/hotspot/jtreg/vmTestbase/nsk/share/jdi/SerialExecutionDebuggee.java test/hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java test/hotspot/jtreg/ProblemList.txt nsk.share.jdi.SerialExecutionDebuggee is the class that used to serially execute the debuggee portion of a specific list of tests. After this class is done executing a debuggee class, it needs to deflate idle monitors in order to prevent a StateTestThread object created by one debuggee class from confusing the next debuggee class. Each of the debuggee classes that use StateTestThread expect there to be only one of these objects. However, since we are running multiple debuggee classes serially *in the same VM*, the StateTestThread object created in one debuggee can still be around when the next debuggee runs. The COMMAND_CLEAR_DEBUGGEE implementation clears the currentDebuggee variable which permits the debuggee to be GC'ed and is modified by this fix to call WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This takes care of any pinned StateTestThread objects (and any other inflated ObjectMonitors). vmTestbase/nsk/jdi/stress/serial/mixed002 is a wrapper style stress test that executes the debugger and debuggee parts of a specific list of tests serially *in the same VM*. Several of the tests executed by mixed002 make use of the StateTestThread class. The failure is intermittent because the order of test execution is shuffled automatically and sometimes the ServiceThread manages to execute deflation at the right time to prevent more than one StateTestThread object from existing at the same time. The additions to vmTestbase/nsk/jdi/stress/serial/mixed002 are the standard boilerplate needed to call WhiteBox functions from test code. The actual call to WhiteBox.deflateIdleMonitors() is made in SerialExecutionDebuggee. I did atte
Re: RFR(S): 8246493: JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support
+1 Thanks, Serguei On 6/29/20 12:49, Chris Plummer wrote: Looks good. Chris On 6/29/20 12:45 PM, Daniel D. Daugherty wrote: Chris and Serguei, Thanks for the fast reviews!! I generated the webrev in my "mach5" directory and that was baselined on the jdk-16+3 snapshot and that doesn't include the ProblemList change. Sigh... I have updated the repo to "current" and regenerated the webrev. test/hotspot/jtreg/ProblemList.txt now shows: @@ -126,11 +126,10 @@ 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/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 8065773 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 8065773 generic-all -vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java 8246493 generic-all vmTestbase/nsk/jdb/eval/eval001/eval001.java 8221503 generic-all vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 generic-all vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 generic-all Thanks again for the fast reviews!! Dan On 6/29/20 3:41 PM, serguei.spit...@oracle.com wrote: Hi Dan, The same as from Chris. The ProblemList.txt has no changes. Otherwise, it looks good. Thanks, Serguei On 6/29/20 12:37, Chris Plummer wrote: Hi Dan, Something is wrong with ProblemList.txt. It doesn't show any changes, but I also don't see mixed002 in the file anymore. Otherwise the changes look good. thanks, Chris On 6/29/20 12:21 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following bug: JDK-8246493 JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support https://bugs.openjdk.java.net/browse/JDK-8246493 Here's the webrev URL: http://cr.openjdk.java.net/~dcubed/8246493-webrev/0_for_jdk16/ The test bug that's being fixed: vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java fails intermittently with the following message: nsk.share.TestBug: There are more than one(2) instance of 'nsk.share.jpda.StateTestThread in debuggee Summary of the fix: Use WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This fix has been tested with a Mach5 Tier5 test run that executes all of the JDI tests (along with JDWP, JVM/TI and other Serviceability tests). I also did five 100 iteration runs of the failing mix002 test. Each Mach5 job set ran the test 100 times on Linux-X64, macOSX, and Win-X64 for a total of (5 * 100 * 3) iterations of nsk/jdi/stress/serial/mixed002. There were no failures. Thanks, in advance, for any comments, questions or suggestions. Dan Gory details: The primary focus of the fix is in the first three files in the webrev: test/hotspot/jtreg/vmTestbase/nsk/share/jdi/SerialExecutionDebuggee.java test/hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java test/hotspot/jtreg/ProblemList.txt nsk.share.jdi.SerialExecutionDebuggee is the class that used to serially execute the debuggee portion of a specific list of tests. After this class is done executing a debuggee class, it needs to deflate idle monitors in order to prevent a StateTestThread object created by one debuggee class from confusing the next debuggee class. Each of the debuggee classes that use StateTestThread expect there to be only one of these objects. However, since we are running multiple debuggee classes serially *in the same VM*, the StateTestThread object created in one debuggee can still be around when the next debuggee runs. The COMMAND_CLEAR_DEBUGGEE implementation clears the currentDebuggee variable which permits the debuggee to be GC'ed and is modified by this fix to call WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This takes care of any pinned StateTestThread objects (and any other inflated ObjectMonitors). vmTestbase/nsk/jdi/stress/serial/mixed002 is a wrapper style stress test that executes the debugger and debuggee parts of a specific list of tests serially *in the same VM*. Several of the tests executed by mixed002 make use of the StateTestThread class. The failure is intermittent because the order of test execution is shuffled automatically and sometimes the ServiceThread manages to execute deflation at the right time to prevent more than one StateTestThread object from existing at the same time. The additions to vmTestbase/nsk/jdi/stress/serial/mixed002 are the standard boilerplate needed to call WhiteBox functions from test code. The actual call to WhiteBox.deflateIdleMonitors() is made in SerialExecutionDebuggee. I did attempt a fix where I modified the StateTestThread class to make the call to WhiteBox.
Re: RFR(S): 8246493: JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support
Ok, in that case it sounds best not to backport. It would be best to make this clear in the bug so there is no future attempt to backport this change except to versions that have already done the WhiteBox.deflateIdleMonitors() backport. thanks, Chris On 6/29/20 1:53 PM, Daniel D. Daugherty wrote: The WhiteBox.deflateIdleMonitors() support is not in JDK15. That's something that added in JDK16 so I'd have to also backport that support. That support was included with another change (getting rid of the special deflation request mechanism) that is not appropriate for JDK15. Short version: I don't think we want to back port part of a patch from JDK16 -> JDK15 in order to fix this test bug. Dan On 6/29/20 4:41 PM, Chris Plummer wrote: Hi Dan, I think you should push it directly to 15 since it's a new issue. thanks, Chris On 6/29/20 12:53 PM, Daniel D. Daugherty wrote: Chris, Thanks. One last thing... since this is a test bug, I wasn't planning to backport the fix to JDK15. The test is ProblemListed there so we won't see the intermittent failures. Are you and Serguei good with not fixing this test bug in JDK15? Dan P.S. Thanks again for your sleuthing that linked the bug to my fix for JDK-8153224. On 6/29/20 3:49 PM, Chris Plummer wrote: Looks good. Chris On 6/29/20 12:45 PM, Daniel D. Daugherty wrote: Chris and Serguei, Thanks for the fast reviews!! I generated the webrev in my "mach5" directory and that was baselined on the jdk-16+3 snapshot and that doesn't include the ProblemList change. Sigh... I have updated the repo to "current" and regenerated the webrev. test/hotspot/jtreg/ProblemList.txt now shows: @@ -126,11 +126,10 @@ 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/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 8065773 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 8065773 generic-all -vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java 8246493 generic-all vmTestbase/nsk/jdb/eval/eval001/eval001.java 8221503 generic-all vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 generic-all vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 generic-all Thanks again for the fast reviews!! Dan On 6/29/20 3:41 PM, serguei.spit...@oracle.com wrote: Hi Dan, The same as from Chris. The ProblemList.txt has no changes. Otherwise, it looks good. Thanks, Serguei On 6/29/20 12:37, Chris Plummer wrote: Hi Dan, Something is wrong with ProblemList.txt. It doesn't show any changes, but I also don't see mixed002 in the file anymore. Otherwise the changes look good. thanks, Chris On 6/29/20 12:21 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following bug: JDK-8246493 JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support https://bugs.openjdk.java.net/browse/JDK-8246493 Here's the webrev URL: http://cr.openjdk.java.net/~dcubed/8246493-webrev/0_for_jdk16/ The test bug that's being fixed: vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java fails intermittently with the following message: nsk.share.TestBug: There are more than one(2) instance of 'nsk.share.jpda.StateTestThread in debuggee Summary of the fix: Use WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This fix has been tested with a Mach5 Tier5 test run that executes all of the JDI tests (along with JDWP, JVM/TI and other Serviceability tests). I also did five 100 iteration runs of the failing mix002 test. Each Mach5 job set ran the test 100 times on Linux-X64, macOSX, and Win-X64 for a total of (5 * 100 * 3) iterations of nsk/jdi/stress/serial/mixed002. There were no failures. Thanks, in advance, for any comments, questions or suggestions. Dan Gory details: The primary focus of the fix is in the first three files in the webrev: test/hotspot/jtreg/vmTestbase/nsk/share/jdi/SerialExecutionDebuggee.java test/hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java test/hotspot/jtreg/ProblemList.txt nsk.share.jdi.SerialExecutionDebuggee is the class that used to serially execute the debuggee portion of a specific list of tests. After this class is done executing a debuggee class, it needs to deflate idle monitors in order to prevent a StateTestThread object created by one debuggee class from confusing the next debuggee class. Each of the debuggee classes that use StateTestThread expect there to be only one of these objects. However, since we are running multiple debuggee classes serially *in the same VM*, the StateTestThread object created in one debuggee can still be around when the next debuggee
Re: RFR(S): 8246493: JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support
The WhiteBox.deflateIdleMonitors() support is not in JDK15. That's something that added in JDK16 so I'd have to also backport that support. That support was included with another change (getting rid of the special deflation request mechanism) that is not appropriate for JDK15. Short version: I don't think we want to back port part of a patch from JDK16 -> JDK15 in order to fix this test bug. Dan On 6/29/20 4:41 PM, Chris Plummer wrote: Hi Dan, I think you should push it directly to 15 since it's a new issue. thanks, Chris On 6/29/20 12:53 PM, Daniel D. Daugherty wrote: Chris, Thanks. One last thing... since this is a test bug, I wasn't planning to backport the fix to JDK15. The test is ProblemListed there so we won't see the intermittent failures. Are you and Serguei good with not fixing this test bug in JDK15? Dan P.S. Thanks again for your sleuthing that linked the bug to my fix for JDK-8153224. On 6/29/20 3:49 PM, Chris Plummer wrote: Looks good. Chris On 6/29/20 12:45 PM, Daniel D. Daugherty wrote: Chris and Serguei, Thanks for the fast reviews!! I generated the webrev in my "mach5" directory and that was baselined on the jdk-16+3 snapshot and that doesn't include the ProblemList change. Sigh... I have updated the repo to "current" and regenerated the webrev. test/hotspot/jtreg/ProblemList.txt now shows: @@ -126,11 +126,10 @@ 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/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 8065773 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 8065773 generic-all -vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java 8246493 generic-all vmTestbase/nsk/jdb/eval/eval001/eval001.java 8221503 generic-all vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 generic-all vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 generic-all Thanks again for the fast reviews!! Dan On 6/29/20 3:41 PM, serguei.spit...@oracle.com wrote: Hi Dan, The same as from Chris. The ProblemList.txt has no changes. Otherwise, it looks good. Thanks, Serguei On 6/29/20 12:37, Chris Plummer wrote: Hi Dan, Something is wrong with ProblemList.txt. It doesn't show any changes, but I also don't see mixed002 in the file anymore. Otherwise the changes look good. thanks, Chris On 6/29/20 12:21 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following bug: JDK-8246493 JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support https://bugs.openjdk.java.net/browse/JDK-8246493 Here's the webrev URL: http://cr.openjdk.java.net/~dcubed/8246493-webrev/0_for_jdk16/ The test bug that's being fixed: vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java fails intermittently with the following message: nsk.share.TestBug: There are more than one(2) instance of 'nsk.share.jpda.StateTestThread in debuggee Summary of the fix: Use WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This fix has been tested with a Mach5 Tier5 test run that executes all of the JDI tests (along with JDWP, JVM/TI and other Serviceability tests). I also did five 100 iteration runs of the failing mix002 test. Each Mach5 job set ran the test 100 times on Linux-X64, macOSX, and Win-X64 for a total of (5 * 100 * 3) iterations of nsk/jdi/stress/serial/mixed002. There were no failures. Thanks, in advance, for any comments, questions or suggestions. Dan Gory details: The primary focus of the fix is in the first three files in the webrev: test/hotspot/jtreg/vmTestbase/nsk/share/jdi/SerialExecutionDebuggee.java test/hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java test/hotspot/jtreg/ProblemList.txt nsk.share.jdi.SerialExecutionDebuggee is the class that used to serially execute the debuggee portion of a specific list of tests. After this class is done executing a debuggee class, it needs to deflate idle monitors in order to prevent a StateTestThread object created by one debuggee class from confusing the next debuggee class. Each of the debuggee classes that use StateTestThread expect there to be only one of these objects. However, since we are running multiple debuggee classes serially *in the same VM*, the StateTestThread object created in one debuggee can still be around when the next debuggee runs. The COMMAND_CLEAR_DEBUGGEE implementation clears the currentDebuggee variable which permits the debuggee to be GC'ed and is modified by this fix to call WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This takes care o
Re: RFR (S): 8245129: Enhance jstat gc option output and tests
Thanks, Daniil! Pushed. Paul On 6/24/20, 4:07 PM, "Daniil Titov" wrote: Hi Paul, The change looks good to me. Thanks! --Daniil On 6/22/20, 8:48 AM, "serviceability-dev on behalf of Hohensee, Paul" wrote: Thanks very much for review, Volker. I'll file a follow-up issue. One more reviewer, please? :) Paul On 6/22/20, 8:10 AM, "serviceability-dev on behalf of Volker Simonis" wrote: Hi Paul, thanks for fixing jstat for larger heaps. I like that you've added explicit tests for ParallelGC which hasn't been tested since G1 was made the default collector. I also agree that sizes should all be right justified. I only wonder if the header of a right justified column shouldn't be right justified as well? However, taking into account that this already hasn't been handled consistently before your change, I'm fine to postpone that to a follow-up cleanup change. I think the change looks good so thumbs up from me. Thank you and best regards, Volker On Thu, Jun 18, 2020 at 11:53 PM Hohensee, Paul wrote: > > Ping. Any takers for this simple patch? > > > > Thanks, > > Paul > > > > From: serviceability-dev on behalf of "Hohensee, Paul" > Date: Monday, May 18, 2020 at 8:25 AM > To: serviceability-dev > Subject: RFR (S): 8245129: Enhance jstat gc option output and tests > > > > Please review an enhancement to the jstat gc option output to make the columns wider (for up to a 2TB heap) so one can read the output without going cross-eyed. > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8245129 > > Webrev: http://cr.openjdk.java.net/~phh/8245129/webrev.00/ > > > > I added tests using ParallelGC since the output can differ for non-G1 collectors. Successfully ran the test/hotspot/jtreg/serviceability/tmtools/jstat and test/jdk/sun/tools/jstat tests. A submit repo run had one failure > > > > runtime/MemberName/MemberNameLeak.java > > tier1 > > macosx-x64-debug > > > > but rerunning it on my laptop succeeded, and there’s no connection between this test and my patch. > > > > Thanks, > > Paul > > > >
Re: RFR(S): 8246493: JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support
Hi Dan, I think you should push it directly to 15 since it's a new issue. thanks, Chris On 6/29/20 12:53 PM, Daniel D. Daugherty wrote: Chris, Thanks. One last thing... since this is a test bug, I wasn't planning to backport the fix to JDK15. The test is ProblemListed there so we won't see the intermittent failures. Are you and Serguei good with not fixing this test bug in JDK15? Dan P.S. Thanks again for your sleuthing that linked the bug to my fix for JDK-8153224. On 6/29/20 3:49 PM, Chris Plummer wrote: Looks good. Chris On 6/29/20 12:45 PM, Daniel D. Daugherty wrote: Chris and Serguei, Thanks for the fast reviews!! I generated the webrev in my "mach5" directory and that was baselined on the jdk-16+3 snapshot and that doesn't include the ProblemList change. Sigh... I have updated the repo to "current" and regenerated the webrev. test/hotspot/jtreg/ProblemList.txt now shows: @@ -126,11 +126,10 @@ 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/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 8065773 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 8065773 generic-all -vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java 8246493 generic-all vmTestbase/nsk/jdb/eval/eval001/eval001.java 8221503 generic-all vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 generic-all vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 generic-all Thanks again for the fast reviews!! Dan On 6/29/20 3:41 PM, serguei.spit...@oracle.com wrote: Hi Dan, The same as from Chris. The ProblemList.txt has no changes. Otherwise, it looks good. Thanks, Serguei On 6/29/20 12:37, Chris Plummer wrote: Hi Dan, Something is wrong with ProblemList.txt. It doesn't show any changes, but I also don't see mixed002 in the file anymore. Otherwise the changes look good. thanks, Chris On 6/29/20 12:21 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following bug: JDK-8246493 JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support https://bugs.openjdk.java.net/browse/JDK-8246493 Here's the webrev URL: http://cr.openjdk.java.net/~dcubed/8246493-webrev/0_for_jdk16/ The test bug that's being fixed: vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java fails intermittently with the following message: nsk.share.TestBug: There are more than one(2) instance of 'nsk.share.jpda.StateTestThread in debuggee Summary of the fix: Use WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This fix has been tested with a Mach5 Tier5 test run that executes all of the JDI tests (along with JDWP, JVM/TI and other Serviceability tests). I also did five 100 iteration runs of the failing mix002 test. Each Mach5 job set ran the test 100 times on Linux-X64, macOSX, and Win-X64 for a total of (5 * 100 * 3) iterations of nsk/jdi/stress/serial/mixed002. There were no failures. Thanks, in advance, for any comments, questions or suggestions. Dan Gory details: The primary focus of the fix is in the first three files in the webrev: test/hotspot/jtreg/vmTestbase/nsk/share/jdi/SerialExecutionDebuggee.java test/hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java test/hotspot/jtreg/ProblemList.txt nsk.share.jdi.SerialExecutionDebuggee is the class that used to serially execute the debuggee portion of a specific list of tests. After this class is done executing a debuggee class, it needs to deflate idle monitors in order to prevent a StateTestThread object created by one debuggee class from confusing the next debuggee class. Each of the debuggee classes that use StateTestThread expect there to be only one of these objects. However, since we are running multiple debuggee classes serially *in the same VM*, the StateTestThread object created in one debuggee can still be around when the next debuggee runs. The COMMAND_CLEAR_DEBUGGEE implementation clears the currentDebuggee variable which permits the debuggee to be GC'ed and is modified by this fix to call WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This takes care of any pinned StateTestThread objects (and any other inflated ObjectMonitors). vmTestbase/nsk/jdi/stress/serial/mixed002 is a wrapper style stress test that executes the debugger and debuggee parts of a specific list of tests serially *in the same VM*. Several of the tests executed by mixed002 make use of the StateTestThread class. The failure is intermittent because the order of test execution is shuffled automatically and sometimes the
Re: RFR(S): 8246493: JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support
Chris, Thanks. One last thing... since this is a test bug, I wasn't planning to backport the fix to JDK15. The test is ProblemListed there so we won't see the intermittent failures. Are you and Serguei good with not fixing this test bug in JDK15? Dan P.S. Thanks again for your sleuthing that linked the bug to my fix for JDK-8153224. On 6/29/20 3:49 PM, Chris Plummer wrote: Looks good. Chris On 6/29/20 12:45 PM, Daniel D. Daugherty wrote: Chris and Serguei, Thanks for the fast reviews!! I generated the webrev in my "mach5" directory and that was baselined on the jdk-16+3 snapshot and that doesn't include the ProblemList change. Sigh... I have updated the repo to "current" and regenerated the webrev. test/hotspot/jtreg/ProblemList.txt now shows: @@ -126,11 +126,10 @@ 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/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 8065773 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 8065773 generic-all -vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java 8246493 generic-all vmTestbase/nsk/jdb/eval/eval001/eval001.java 8221503 generic-all vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 generic-all vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 generic-all Thanks again for the fast reviews!! Dan On 6/29/20 3:41 PM, serguei.spit...@oracle.com wrote: Hi Dan, The same as from Chris. The ProblemList.txt has no changes. Otherwise, it looks good. Thanks, Serguei On 6/29/20 12:37, Chris Plummer wrote: Hi Dan, Something is wrong with ProblemList.txt. It doesn't show any changes, but I also don't see mixed002 in the file anymore. Otherwise the changes look good. thanks, Chris On 6/29/20 12:21 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following bug: JDK-8246493 JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support https://bugs.openjdk.java.net/browse/JDK-8246493 Here's the webrev URL: http://cr.openjdk.java.net/~dcubed/8246493-webrev/0_for_jdk16/ The test bug that's being fixed: vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java fails intermittently with the following message: nsk.share.TestBug: There are more than one(2) instance of 'nsk.share.jpda.StateTestThread in debuggee Summary of the fix: Use WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This fix has been tested with a Mach5 Tier5 test run that executes all of the JDI tests (along with JDWP, JVM/TI and other Serviceability tests). I also did five 100 iteration runs of the failing mix002 test. Each Mach5 job set ran the test 100 times on Linux-X64, macOSX, and Win-X64 for a total of (5 * 100 * 3) iterations of nsk/jdi/stress/serial/mixed002. There were no failures. Thanks, in advance, for any comments, questions or suggestions. Dan Gory details: The primary focus of the fix is in the first three files in the webrev: test/hotspot/jtreg/vmTestbase/nsk/share/jdi/SerialExecutionDebuggee.java test/hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java test/hotspot/jtreg/ProblemList.txt nsk.share.jdi.SerialExecutionDebuggee is the class that used to serially execute the debuggee portion of a specific list of tests. After this class is done executing a debuggee class, it needs to deflate idle monitors in order to prevent a StateTestThread object created by one debuggee class from confusing the next debuggee class. Each of the debuggee classes that use StateTestThread expect there to be only one of these objects. However, since we are running multiple debuggee classes serially *in the same VM*, the StateTestThread object created in one debuggee can still be around when the next debuggee runs. The COMMAND_CLEAR_DEBUGGEE implementation clears the currentDebuggee variable which permits the debuggee to be GC'ed and is modified by this fix to call WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This takes care of any pinned StateTestThread objects (and any other inflated ObjectMonitors). vmTestbase/nsk/jdi/stress/serial/mixed002 is a wrapper style stress test that executes the debugger and debuggee parts of a specific list of tests serially *in the same VM*. Several of the tests executed by mixed002 make use of the StateTestThread class. The failure is intermittent because the order of test execution is shuffled automatically and sometimes the ServiceThread manages to execute deflation at the right time to prevent more than one StateTestThread object from existing at the same time.
RFR: 8205467: javax/management/remote/mandatory/connection/MultiThreadDeadLockTest.java possible deadlock
Please review a tiny change that adjusts the wait timeout the test uses for "test.timeout.factor" system property. Please note that a trivial merge with fix [4] that is currently on review [3] will be required. Since issues [2] and [4] describe different problems I decided to not combine these both changes in the single fix. Testing: Mach5 tests tier1-tier3 successfully passed. [1] Web rev: https://cr.openjdk.java.net/~dtitov/8205467/webrev.01/ [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8205467 [3] https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-June/032098.html [4] https://bugs.openjdk.java.net/browse/JDK-8227337 Thank you, Daniil
Re: [15] RFR(XXS): 7107012: sun.jvm.hostspot.code.CompressedReadStream readDouble() conversion to long mishandled
Hi Chris, This one caught my eye just because it was such an old bug number... :-) On 6/26/20 7:03 PM, Chris Plummer wrote: Hello, Please help review the following: http://cr.openjdk.java.net/~cjplummer/7107012/webrev.00/index.html src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CompressedReadStream.java Nice catch! Thumbs up. Dan https://bugs.openjdk.java.net/browse/JDK-7107012 This bug is filed as confidential, although the issue is trivial. In the following line of code: return Double.longBitsToDouble((h << 32) | ((long)l & 0xL)); Since h is an int, it's subject to the following: https://docs.oracle.com/javase/specs/jls/se14/html/jls-15.html#jls-15.19 "If the promoted type of the left-hand operand is int, then only the five lowest-order bits of the right-hand operand are used as the shift distance. It is as if the right-hand operand were subjected to a bitwise logical AND operator & (§15.22.1) with the mask value 0x1f (0b1). The shift distance actually used is therefore always in the range 0 to 31, inclusive." So (h << 32) is the same as (h << 0), which is not what was intended. The spec also calls out another issue: "The type of the shift expression is the promoted type of the left-hand operand." So even if it did left shift 32 bits, the result would have been truncated to an int, meaning the result would always be 0. The fix is to first cast h to a long. Doing this addresses both these problems, allowing a full 32 bit left shift to be done, and leaving the result as an untruncated long. I was unable to trigger use of this code in SA. It seems to be used to pull locals out of a CompiledVFrame. I don't see any clhsdb paths to this code. It appears the GUI hsdb uses it via a complex call path I could not fully decipher, but I could not trigger its use from hsdb. In any case, the fix is straight forward and trivial, so I'd rather not have to spend more time digging deeper into its use and providing a test case. thanks, Chris
Re: RFR(S): 8246493: JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support
Looks good. Chris On 6/29/20 12:45 PM, Daniel D. Daugherty wrote: Chris and Serguei, Thanks for the fast reviews!! I generated the webrev in my "mach5" directory and that was baselined on the jdk-16+3 snapshot and that doesn't include the ProblemList change. Sigh... I have updated the repo to "current" and regenerated the webrev. test/hotspot/jtreg/ProblemList.txt now shows: @@ -126,11 +126,10 @@ 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/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 8065773 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 8065773 generic-all -vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java 8246493 generic-all vmTestbase/nsk/jdb/eval/eval001/eval001.java 8221503 generic-all vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 generic-all vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 generic-all Thanks again for the fast reviews!! Dan On 6/29/20 3:41 PM, serguei.spit...@oracle.com wrote: Hi Dan, The same as from Chris. The ProblemList.txt has no changes. Otherwise, it looks good. Thanks, Serguei On 6/29/20 12:37, Chris Plummer wrote: Hi Dan, Something is wrong with ProblemList.txt. It doesn't show any changes, but I also don't see mixed002 in the file anymore. Otherwise the changes look good. thanks, Chris On 6/29/20 12:21 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following bug: JDK-8246493 JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support https://bugs.openjdk.java.net/browse/JDK-8246493 Here's the webrev URL: http://cr.openjdk.java.net/~dcubed/8246493-webrev/0_for_jdk16/ The test bug that's being fixed: vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java fails intermittently with the following message: nsk.share.TestBug: There are more than one(2) instance of 'nsk.share.jpda.StateTestThread in debuggee Summary of the fix: Use WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This fix has been tested with a Mach5 Tier5 test run that executes all of the JDI tests (along with JDWP, JVM/TI and other Serviceability tests). I also did five 100 iteration runs of the failing mix002 test. Each Mach5 job set ran the test 100 times on Linux-X64, macOSX, and Win-X64 for a total of (5 * 100 * 3) iterations of nsk/jdi/stress/serial/mixed002. There were no failures. Thanks, in advance, for any comments, questions or suggestions. Dan Gory details: The primary focus of the fix is in the first three files in the webrev: test/hotspot/jtreg/vmTestbase/nsk/share/jdi/SerialExecutionDebuggee.java test/hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java test/hotspot/jtreg/ProblemList.txt nsk.share.jdi.SerialExecutionDebuggee is the class that used to serially execute the debuggee portion of a specific list of tests. After this class is done executing a debuggee class, it needs to deflate idle monitors in order to prevent a StateTestThread object created by one debuggee class from confusing the next debuggee class. Each of the debuggee classes that use StateTestThread expect there to be only one of these objects. However, since we are running multiple debuggee classes serially *in the same VM*, the StateTestThread object created in one debuggee can still be around when the next debuggee runs. The COMMAND_CLEAR_DEBUGGEE implementation clears the currentDebuggee variable which permits the debuggee to be GC'ed and is modified by this fix to call WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This takes care of any pinned StateTestThread objects (and any other inflated ObjectMonitors). vmTestbase/nsk/jdi/stress/serial/mixed002 is a wrapper style stress test that executes the debugger and debuggee parts of a specific list of tests serially *in the same VM*. Several of the tests executed by mixed002 make use of the StateTestThread class. The failure is intermittent because the order of test execution is shuffled automatically and sometimes the ServiceThread manages to execute deflation at the right time to prevent more than one StateTestThread object from existing at the same time. The additions to vmTestbase/nsk/jdi/stress/serial/mixed002 are the standard boilerplate needed to call WhiteBox functions from test code. The actual call to WhiteBox.deflateIdleMonitors() is made in SerialExecutionDebuggee. I did attempt a fix where I modified the StateTestThread class to make the call to WhiteBox.deflateIdleMonitors() after the internal waitOnObject is no longer
Re: RFR(S): 8246493: JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support
Chris and Serguei, Thanks for the fast reviews!! I generated the webrev in my "mach5" directory and that was baselined on the jdk-16+3 snapshot and that doesn't include the ProblemList change. Sigh... I have updated the repo to "current" and regenerated the webrev. test/hotspot/jtreg/ProblemList.txt now shows: @@ -126,11 +126,10 @@ 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/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java 8065773 generic-all vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 8065773 generic-all -vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java 8246493 generic-all vmTestbase/nsk/jdb/eval/eval001/eval001.java 8221503 generic-all vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 generic-all vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250 generic-all Thanks again for the fast reviews!! Dan On 6/29/20 3:41 PM, serguei.spit...@oracle.com wrote: Hi Dan, The same as from Chris. The ProblemList.txt has no changes. Otherwise, it looks good. Thanks, Serguei On 6/29/20 12:37, Chris Plummer wrote: Hi Dan, Something is wrong with ProblemList.txt. It doesn't show any changes, but I also don't see mixed002 in the file anymore. Otherwise the changes look good. thanks, Chris On 6/29/20 12:21 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following bug: JDK-8246493 JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support https://bugs.openjdk.java.net/browse/JDK-8246493 Here's the webrev URL: http://cr.openjdk.java.net/~dcubed/8246493-webrev/0_for_jdk16/ The test bug that's being fixed: vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java fails intermittently with the following message: nsk.share.TestBug: There are more than one(2) instance of 'nsk.share.jpda.StateTestThread in debuggee Summary of the fix: Use WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This fix has been tested with a Mach5 Tier5 test run that executes all of the JDI tests (along with JDWP, JVM/TI and other Serviceability tests). I also did five 100 iteration runs of the failing mix002 test. Each Mach5 job set ran the test 100 times on Linux-X64, macOSX, and Win-X64 for a total of (5 * 100 * 3) iterations of nsk/jdi/stress/serial/mixed002. There were no failures. Thanks, in advance, for any comments, questions or suggestions. Dan Gory details: The primary focus of the fix is in the first three files in the webrev: test/hotspot/jtreg/vmTestbase/nsk/share/jdi/SerialExecutionDebuggee.java test/hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java test/hotspot/jtreg/ProblemList.txt nsk.share.jdi.SerialExecutionDebuggee is the class that used to serially execute the debuggee portion of a specific list of tests. After this class is done executing a debuggee class, it needs to deflate idle monitors in order to prevent a StateTestThread object created by one debuggee class from confusing the next debuggee class. Each of the debuggee classes that use StateTestThread expect there to be only one of these objects. However, since we are running multiple debuggee classes serially *in the same VM*, the StateTestThread object created in one debuggee can still be around when the next debuggee runs. The COMMAND_CLEAR_DEBUGGEE implementation clears the currentDebuggee variable which permits the debuggee to be GC'ed and is modified by this fix to call WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This takes care of any pinned StateTestThread objects (and any other inflated ObjectMonitors). vmTestbase/nsk/jdi/stress/serial/mixed002 is a wrapper style stress test that executes the debugger and debuggee parts of a specific list of tests serially *in the same VM*. Several of the tests executed by mixed002 make use of the StateTestThread class. The failure is intermittent because the order of test execution is shuffled automatically and sometimes the ServiceThread manages to execute deflation at the right time to prevent more than one StateTestThread object from existing at the same time. The additions to vmTestbase/nsk/jdi/stress/serial/mixed002 are the standard boilerplate needed to call WhiteBox functions from test code. The actual call to WhiteBox.deflateIdleMonitors() is made in SerialExecutionDebuggee. I did attempt a fix where I modified the StateTestThread class to make the call to WhiteBox.deflateIdleMonitors() after the internal waitOnObject is no longer contended or waited on. That fix reduced the frequency of the fail
Re: RFR(S): 8246493: JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support
Hi Dan, The same as from Chris. The ProblemList.txt has no changes. Otherwise, it looks good. Thanks, Serguei On 6/29/20 12:37, Chris Plummer wrote: Hi Dan, Something is wrong with ProblemList.txt. It doesn't show any changes, but I also don't see mixed002 in the file anymore. Otherwise the changes look good. thanks, Chris On 6/29/20 12:21 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following bug: JDK-8246493 JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support https://bugs.openjdk.java.net/browse/JDK-8246493 Here's the webrev URL: http://cr.openjdk.java.net/~dcubed/8246493-webrev/0_for_jdk16/ The test bug that's being fixed: vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java fails intermittently with the following message: nsk.share.TestBug: There are more than one(2) instance of 'nsk.share.jpda.StateTestThread in debuggee Summary of the fix: Use WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This fix has been tested with a Mach5 Tier5 test run that executes all of the JDI tests (along with JDWP, JVM/TI and other Serviceability tests). I also did five 100 iteration runs of the failing mix002 test. Each Mach5 job set ran the test 100 times on Linux-X64, macOSX, and Win-X64 for a total of (5 * 100 * 3) iterations of nsk/jdi/stress/serial/mixed002. There were no failures. Thanks, in advance, for any comments, questions or suggestions. Dan Gory details: The primary focus of the fix is in the first three files in the webrev: test/hotspot/jtreg/vmTestbase/nsk/share/jdi/SerialExecutionDebuggee.java test/hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java test/hotspot/jtreg/ProblemList.txt nsk.share.jdi.SerialExecutionDebuggee is the class that used to serially execute the debuggee portion of a specific list of tests. After this class is done executing a debuggee class, it needs to deflate idle monitors in order to prevent a StateTestThread object created by one debuggee class from confusing the next debuggee class. Each of the debuggee classes that use StateTestThread expect there to be only one of these objects. However, since we are running multiple debuggee classes serially *in the same VM*, the StateTestThread object created in one debuggee can still be around when the next debuggee runs. The COMMAND_CLEAR_DEBUGGEE implementation clears the currentDebuggee variable which permits the debuggee to be GC'ed and is modified by this fix to call WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This takes care of any pinned StateTestThread objects (and any other inflated ObjectMonitors). vmTestbase/nsk/jdi/stress/serial/mixed002 is a wrapper style stress test that executes the debugger and debuggee parts of a specific list of tests serially *in the same VM*. Several of the tests executed by mixed002 make use of the StateTestThread class. The failure is intermittent because the order of test execution is shuffled automatically and sometimes the ServiceThread manages to execute deflation at the right time to prevent more than one StateTestThread object from existing at the same time. The additions to vmTestbase/nsk/jdi/stress/serial/mixed002 are the standard boilerplate needed to call WhiteBox functions from test code. The actual call to WhiteBox.deflateIdleMonitors() is made in SerialExecutionDebuggee. I did attempt a fix where I modified the StateTestThread class to make the call to WhiteBox.deflateIdleMonitors() after the internal waitOnObject is no longer contended or waited on. That fix reduced the frequency of the failures by about half, but it didn't solve the test bug entirely. So I had to make the fix in SerialExecutionDebuggee instead. test/hotspot/jtreg/ProblemList.txt is modified to re-enable the mix002 test. The remaining nine files are also wrapper style stress tests that execute the debugger and debuggee parts of a specific list of tests serially *in the same VM*. Because these tests also use SerialExecutionDebuggee, they also need the boilerplate changes so that WhiteBox.deflateIdleMonitors() can be called.
RFR: 8227337: javax/management/remote/mandatory/connection/ReconnectTest.java NoSuchObjectException no such object in table
Please review the change that fixes an intermittent tests failure. The tests javax/management/remote/mandatory/connection/ReconnectTest.java and javax/management/remote/mandatory/connection/MultiThreadDeadLockTest.java use specific settings for server timeout that in some cases (e.g. when the test is run with -Xcomp) result in JMX server connection timeout thread unexports the remote object while the client connection is still in the progress. Below is an example of a such stacktrace: java.rmi.NoSuchObjectException: no such object in table at java.rmi/sun.rmi.transport.StreamRemoteCall.exceptionReceivedFromServer(StreamRemoteCall.java:303) at java.rmi/sun.rmi.transport.StreamRemoteCall.executeCall(StreamRemoteCall.java:279) at java.rmi/sun.rmi.server.UnicastRef.invoke(UnicastRef.java:164) at jdk.remoteref/jdk.jmx.remote.internal.rmi.PRef.invoke(Unknown Source) at java.management.rmi/javax.management.remote.rmi.RMIConnectionImpl_Stub.getConnectionId(RMIConnectionImpl_Stub.java:318) at java.management.rmi/javax.management.remote.rmi.RMIConnector.getConnectionId(RMIConnector.java:385) at java.management.rmi/javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java:347) at java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:270) at MultiThreadDeadLockTest.main(MultiThreadDeadLockTest.java:87) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:564) at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) at java.base/java.lang.Thread.run(Thread.java:832) The fix adjusts the server timeout the tests use for "test.timeout.factor" system property. Testing: Mach5 tests are in the progress. [1] https://cr.openjdk.java.net/~dtitov/8227337/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8227337 Thanks, Daniil
Re: RFR(S): 8246493: JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support
Hi Dan, Something is wrong with ProblemList.txt. It doesn't show any changes, but I also don't see mixed002 in the file anymore. Otherwise the changes look good. thanks, Chris On 6/29/20 12:21 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following bug: JDK-8246493 JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support https://bugs.openjdk.java.net/browse/JDK-8246493 Here's the webrev URL: http://cr.openjdk.java.net/~dcubed/8246493-webrev/0_for_jdk16/ The test bug that's being fixed: vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java fails intermittently with the following message: nsk.share.TestBug: There are more than one(2) instance of 'nsk.share.jpda.StateTestThread in debuggee Summary of the fix: Use WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This fix has been tested with a Mach5 Tier5 test run that executes all of the JDI tests (along with JDWP, JVM/TI and other Serviceability tests). I also did five 100 iteration runs of the failing mix002 test. Each Mach5 job set ran the test 100 times on Linux-X64, macOSX, and Win-X64 for a total of (5 * 100 * 3) iterations of nsk/jdi/stress/serial/mixed002. There were no failures. Thanks, in advance, for any comments, questions or suggestions. Dan Gory details: The primary focus of the fix is in the first three files in the webrev: test/hotspot/jtreg/vmTestbase/nsk/share/jdi/SerialExecutionDebuggee.java test/hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java test/hotspot/jtreg/ProblemList.txt nsk.share.jdi.SerialExecutionDebuggee is the class that used to serially execute the debuggee portion of a specific list of tests. After this class is done executing a debuggee class, it needs to deflate idle monitors in order to prevent a StateTestThread object created by one debuggee class from confusing the next debuggee class. Each of the debuggee classes that use StateTestThread expect there to be only one of these objects. However, since we are running multiple debuggee classes serially *in the same VM*, the StateTestThread object created in one debuggee can still be around when the next debuggee runs. The COMMAND_CLEAR_DEBUGGEE implementation clears the currentDebuggee variable which permits the debuggee to be GC'ed and is modified by this fix to call WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This takes care of any pinned StateTestThread objects (and any other inflated ObjectMonitors). vmTestbase/nsk/jdi/stress/serial/mixed002 is a wrapper style stress test that executes the debugger and debuggee parts of a specific list of tests serially *in the same VM*. Several of the tests executed by mixed002 make use of the StateTestThread class. The failure is intermittent because the order of test execution is shuffled automatically and sometimes the ServiceThread manages to execute deflation at the right time to prevent more than one StateTestThread object from existing at the same time. The additions to vmTestbase/nsk/jdi/stress/serial/mixed002 are the standard boilerplate needed to call WhiteBox functions from test code. The actual call to WhiteBox.deflateIdleMonitors() is made in SerialExecutionDebuggee. I did attempt a fix where I modified the StateTestThread class to make the call to WhiteBox.deflateIdleMonitors() after the internal waitOnObject is no longer contended or waited on. That fix reduced the frequency of the failures by about half, but it didn't solve the test bug entirely. So I had to make the fix in SerialExecutionDebuggee instead. test/hotspot/jtreg/ProblemList.txt is modified to re-enable the mix002 test. The remaining nine files are also wrapper style stress tests that execute the debugger and debuggee parts of a specific list of tests serially *in the same VM*. Because these tests also use SerialExecutionDebuggee, they also need the boilerplate changes so that WhiteBox.deflateIdleMonitors() can be called.
RFR(S): 8246493: JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support
Greetings, I have a fix for the following bug: JDK-8246493 JDI stress/serial/mixed002 needs to use WhiteBox.deflateIdleMonitors support https://bugs.openjdk.java.net/browse/JDK-8246493 Here's the webrev URL: http://cr.openjdk.java.net/~dcubed/8246493-webrev/0_for_jdk16/ The test bug that's being fixed: vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java fails intermittently with the following message: nsk.share.TestBug: There are more than one(2) instance of 'nsk.share.jpda.StateTestThread in debuggee Summary of the fix: Use WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This fix has been tested with a Mach5 Tier5 test run that executes all of the JDI tests (along with JDWP, JVM/TI and other Serviceability tests). I also did five 100 iteration runs of the failing mix002 test. Each Mach5 job set ran the test 100 times on Linux-X64, macOSX, and Win-X64 for a total of (5 * 100 * 3) iterations of nsk/jdi/stress/serial/mixed002. There were no failures. Thanks, in advance, for any comments, questions or suggestions. Dan Gory details: The primary focus of the fix is in the first three files in the webrev: test/hotspot/jtreg/vmTestbase/nsk/share/jdi/SerialExecutionDebuggee.java test/hotspot/jtreg/vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java test/hotspot/jtreg/ProblemList.txt nsk.share.jdi.SerialExecutionDebuggee is the class that used to serially execute the debuggee portion of a specific list of tests. After this class is done executing a debuggee class, it needs to deflate idle monitors in order to prevent a StateTestThread object created by one debuggee class from confusing the next debuggee class. Each of the debuggee classes that use StateTestThread expect there to be only one of these objects. However, since we are running multiple debuggee classes serially *in the same VM*, the StateTestThread object created in one debuggee can still be around when the next debuggee runs. The COMMAND_CLEAR_DEBUGGEE implementation clears the currentDebuggee variable which permits the debuggee to be GC'ed and is modified by this fix to call WhiteBox.deflateIdleMonitors() to make sure that all inflated ObjectMonitors are deflated after each debuggee has been run. This takes care of any pinned StateTestThread objects (and any other inflated ObjectMonitors). vmTestbase/nsk/jdi/stress/serial/mixed002 is a wrapper style stress test that executes the debugger and debuggee parts of a specific list of tests serially *in the same VM*. Several of the tests executed by mixed002 make use of the StateTestThread class. The failure is intermittent because the order of test execution is shuffled automatically and sometimes the ServiceThread manages to execute deflation at the right time to prevent more than one StateTestThread object from existing at the same time. The additions to vmTestbase/nsk/jdi/stress/serial/mixed002 are the standard boilerplate needed to call WhiteBox functions from test code. The actual call to WhiteBox.deflateIdleMonitors() is made in SerialExecutionDebuggee. I did attempt a fix where I modified the StateTestThread class to make the call to WhiteBox.deflateIdleMonitors() after the internal waitOnObject is no longer contended or waited on. That fix reduced the frequency of the failures by about half, but it didn't solve the test bug entirely. So I had to make the fix in SerialExecutionDebuggee instead. test/hotspot/jtreg/ProblemList.txt is modified to re-enable the mix002 test. The remaining nine files are also wrapper style stress tests that execute the debugger and debuggee parts of a specific list of tests serially *in the same VM*. Because these tests also use SerialExecutionDebuggee, they also need the boilerplate changes so that WhiteBox.deflateIdleMonitors() can be called.
Re: [15] RFR(XXS): 7107012: sun.jvm.hostspot.code.CompressedReadStream readDouble() conversion to long mishandled
Thanks Serguei! Can I get one more reviewer please? The change is very simple. thanks, Chris On 6/26/20 5:51 PM, serguei.spit...@oracle.com wrote: Hi Chris, The fix looks good. I would most likely overlook such a bug with my eyes. :) Thanks, Serguei On 6/26/20 16:03, Chris Plummer wrote: Hello, Please help review the following: http://cr.openjdk.java.net/~cjplummer/7107012/webrev.00/index.html https://bugs.openjdk.java.net/browse/JDK-7107012 This bug is filed as confidential, although the issue is trivial. In the following line of code: return Double.longBitsToDouble((h << 32) | ((long)l & 0xL)); Since h is an int, it's subject to the following: https://docs.oracle.com/javase/specs/jls/se14/html/jls-15.html#jls-15.19 "If the promoted type of the left-hand operand is int, then only the five lowest-order bits of the right-hand operand are used as the shift distance. It is as if the right-hand operand were subjected to a bitwise logical AND operator & (§15.22.1) with the mask value 0x1f (0b1). The shift distance actually used is therefore always in the range 0 to 31, inclusive." So (h << 32) is the same as (h << 0), which is not what was intended. The spec also calls out another issue: "The type of the shift expression is the promoted type of the left-hand operand." So even if it did left shift 32 bits, the result would have been truncated to an int, meaning the result would always be 0. The fix is to first cast h to a long. Doing this addresses both these problems, allowing a full 32 bit left shift to be done, and leaving the result as an untruncated long. I was unable to trigger use of this code in SA. It seems to be used to pull locals out of a CompiledVFrame. I don't see any clhsdb paths to this code. It appears the GUI hsdb uses it via a complex call path I could not fully decipher, but I could not trigger its use from hsdb. In any case, the fix is straight forward and trivial, so I'd rather not have to spend more time digging deeper into its use and providing a test case. thanks, Chris
Re: 15 RFR(XS): 8165276: Spec states that invoke the premain method in an agent class if it's public but implementation differs
Thanks a lot for review, Mandy! I also, asked Leonid to look if the test changes can be simplified Thanks, Serguei On 6/29/20 09:46, Mandy Chung wrote: On 6/27/20 12:23 AM, Alan Bateman wrote: On 27/06/2020 01:40, serguei.spit...@oracle.com wrote: The implementation has this order of lookup: // The agent class must have a premain or agentmain method that // has 1 or 2 arguments. We check in the following order: // // 1) declared with a signature of (String, Instrumentation) // 2) declared with a signature of (String) // 3) inherited with a signature of (String, Instrumentation) // 4) inherited with a signature of (String) The declared methods are gotten with the getDeclaredMethod and inherited with the getMethod. This works for both 1-arg and 2-arg premain methods, so I'm not sure what is inconsistent. Or you have a concern there can be a non-nice NoSuchMethodException? In fact, I don't understand why there is a need to use the getDeclaredMethod. As I see, the getMethod should return a declared method first, and only if it is absent then it checks for a inherited one. The JPLIS agent used getMethod when it was originally created in JDK 5 so it would only find public methods. I haven't studied the intervening history too closely but I assume JDK-6289149 (in JDK 7) created the inconsistency between the spec and implementation when it explored the scenario of premain declared in a super class with different arity and/or modifiers to the premain in the sub-class. I assume the tests that you've been forced to change are related to this same issue. Thanks for digging up the history. So given where we are, and given the statement "The JVM first attempts to invoke the following method on the agent class" in the spec then I guess it's okay to keep the getDeclaredMethod to deal with "whacky" case where a super class of the agent class has a public premain method. I also think it's okay to get a different exception message in this case. Serguie - I reviewed this version. It looks okay. New wevrev version is: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/instr-setAccessible.2/ Mandy
Re: 15 RFR(XS): 8165276: Spec states that invoke the premain method in an agent class if it's public but implementation differs
Hi David, Thank you a lot for review and tweaking the bug title. I've re-targeted this to 16 as was suggested by Joe. Thanks, Serguei On 6/28/20 19:37, David Holmes wrote: Hi Serguei, These changes look good to me. Note that I tweaked the bug synopsis to make it slightly more grammatically correct: that invoke -> to invoke Thanks, David On 26/06/2020 2:17 am, serguei.spit...@oracle.com wrote: New wevrev version is: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/instr-setAccessible.2/ Now the InstrumentationImpl.java has this new check to throw IAE with the meaningful error message: + // reject non-public premain or agentmain method + if (!m.canAccess(null)) { + String msg = "method " + classname + "." + methodname + " must be declared public"; + throw new IllegalAccessException(msg); + } It also includes a new negative test for non-public premain method: test/jdk/java/lang/instrument/NonPublicPremainAgent.java I've tested the non-public agentmain as well with one of the hacked JVMTI aod tests. But I gave up to make it a stand alone test as this testing framework is tricky to use for negative testing. The implementation is common for premain and agentmain cases, so probably, one test Also, I had to fix all impacted java/lang/instrument tests to make the Agent classes public. The following tests required a refactoring: || test/jdk/java/lang/instrument/PremainClass/InheritAgent0100.java test/jdk/java/lang/instrument/PremainClass/InheritAgent1000.java test/jdk/java/lang/instrument/PremainClass/InheritAgent1100.java They define an agent as extending an agent super class which has the premain methods defined: 37 class InheritAgent0101 extends InheritAgent0101Super { 38 39 // 40 // This agent has a single argument premain() method which 41 // is the one that should be called. 42 // 43 public static void premain (String agentArgs) { 44 System.out.println("Hello from Single-Arg InheritAgent0101!"); 45 } 46 47 // This agent does NOT have a double argument premain() method. 48 } 49 50 class InheritAgent0101Super { 51 52 // 53 // This agent has a single argument premain() method which 54 // is NOT the one that should be called. 55 // 56 public static void premain (String agentArgs) { 57 System.out.println("Hello from Single-Arg InheritAgent0101Super!"); 58 throw new Error("ERROR: THIS AGENT SHOULD NOT HAVE BEEN CALLED."); 59 } 60 61 // This agent does NOT have a double argument premain() method. 62 } Above, just one class can be made public. But the InheritAgent0101Super has to be public as well as has the premain method defined. These agent super classes are separated to their own files. To make this refactoring to work new || customized script is introduced: test/jdk/java/lang/instrument/PremainClass/MakeJAR.sh The java/lang/instrument tests are passed locally. I'll submit a mach5 test jobs to make sure nothing is broken. Thanks, Serguei On 6/24/20 13:07, serguei.spit...@oracle.com wrote: On 6/24/20 12:44, Mandy Chung wrote: On 6/24/20 12:26 PM, serguei.spit...@oracle.com wrote: On 6/24/20 05:25, David Holmes wrote: Ah! The test class SimpleAgent is what is not public. That seems a bug in the test. There are many such tests. We can break some of the existing agents by rejecting non-public agent classes. I'm inclined to continue using the setAccessible and just add an extra check for non-public premain/agentmain methods. There is only one non-public SimpleAgent which is shared by j.l.instrument tests. test/jdk/java/lang/instrument/SimpleAgent.java There are many such tests: test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TestLambdaFormRetransformation.java:class Agent implements ClassFileTransformer { test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java:class NativeMethodPrefixAgent { test/jdk/java/lang/instrument/PremainClass/NoPremainAgent.java:class NoPremainAgent { test/jdk/java/lang/instrument/SimpleAgent.java:class SimpleAgent { test/jdk/java/lang/instrument/RetransformAgent.java:class RetransformAgent { test/jdk/java/lang/instrument/PremainClass/InheritAgent0001.java:class InheritAgent0001 extends InheritAgent0001Super { test/jdk/java/lang/instrument/PremainClass/InheritAgent0001.java:class InheritAgent0001Super { test/jdk/java/lang/instrument/PremainClass/InheritAgent0010.java:class InheritAgent0010 extends InheritAgent0010Super { test/jdk/java/lang/instrument/PremainClass/InheritAgent0010.java:class InheritAgent0010Super { test/jdk/java/lang/instrument/PremainClass/InheritAgent0011.java:class InheritAgent0011 extends InheritAgent0011Super { test/jdk/java/lang/instrument/PremainClass/InheritAgent0011.java:class InheritAgent0011Super { test/jdk/java/lang/instrument/PremainClass/InheritAgent0100.java:class InheritAgent0100 extends InheritAgent0100S
Re: 15 RFR(XS): 8165276: Spec states that invoke the premain method in an agent class if it's public but implementation differs
On 6/27/20 00:23, Alan Bateman wrote: On 27/06/2020 01:40, serguei.spit...@oracle.com wrote: The implementation has this order of lookup: // The agent class must have a premain or agentmain method that // has 1 or 2 arguments. We check in the following order: // // 1) declared with a signature of (String, Instrumentation) // 2) declared with a signature of (String) // 3) inherited with a signature of (String, Instrumentation) // 4) inherited with a signature of (String) The declared methods are gotten with the getDeclaredMethod and inherited with the getMethod. This works for both 1-arg and 2-arg premain methods, so I'm not sure what is inconsistent. Or you have a concern there can be a non-nice NoSuchMethodException? In fact, I don't understand why there is a need to use the getDeclaredMethod. As I see, the getMethod should return a declared method first, and only if it is absent then it checks for a inherited one. The JPLIS agent used getMethod when it was originally created in JDK 5 so it would only find public methods. I haven't studied the intervening history too closely but I assume JDK-6289149 (in JDK 7) created the inconsistency between the spec and implementation when it explored the scenario of premain declared in a super class with different arity and/or modifiers to the premain in the sub-class. I assume the tests that you've been forced to change are related to this same issue. So given where we are, and given the statement "The JVM first attempts to invoke the following method on the agent class" in the spec then I guess it's okay to keep the getDeclaredMethod to deal with "whacky" case where a super class of the agent class has a public premain method. Thank you for clarification, Alan. Thanks, Serguei -Alan.
Re: 15 RFR(XS): 8165276: Spec states that invoke the premain method in an agent class if it's public but implementation differs
On 6/27/20 12:23 AM, Alan Bateman wrote: On 27/06/2020 01:40, serguei.spit...@oracle.com wrote: The implementation has this order of lookup: // The agent class must have a premain or agentmain method that // has 1 or 2 arguments. We check in the following order: // // 1) declared with a signature of (String, Instrumentation) // 2) declared with a signature of (String) // 3) inherited with a signature of (String, Instrumentation) // 4) inherited with a signature of (String) The declared methods are gotten with the getDeclaredMethod and inherited with the getMethod. This works for both 1-arg and 2-arg premain methods, so I'm not sure what is inconsistent. Or you have a concern there can be a non-nice NoSuchMethodException? In fact, I don't understand why there is a need to use the getDeclaredMethod. As I see, the getMethod should return a declared method first, and only if it is absent then it checks for a inherited one. The JPLIS agent used getMethod when it was originally created in JDK 5 so it would only find public methods. I haven't studied the intervening history too closely but I assume JDK-6289149 (in JDK 7) created the inconsistency between the spec and implementation when it explored the scenario of premain declared in a super class with different arity and/or modifiers to the premain in the sub-class. I assume the tests that you've been forced to change are related to this same issue. Thanks for digging up the history. So given where we are, and given the statement "The JVM first attempts to invoke the following method on the agent class" in the spec then I guess it's okay to keep the getDeclaredMethod to deal with "whacky" case where a super class of the agent class has a public premain method. I also think it's okay to get a different exception message in this case. Serguie - I reviewed this version. It looks okay. New wevrev version is: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/instr-setAccessible.2/ Mandy
RFR(s): 8247863: Unreachable code in OperatingSystemImpl.getTotalSwapSpaceSize()
Hi, Could I please get a review of this dead-code removal? During review of JDK-8244500 it was discovered that with the new cgroups implementation supporting v1 and v2 Metrics.getMemoryAndSwapLimit() will never return 0 when relevant cgroup files are missing. E.g. on a system where the kernel doesn't support swap limit capabilities. Therefore this code introduced with JDK-8236617 can no longer be reached and should get removed. Bug: https://bugs.openjdk.java.net/browse/JDK-8247863 webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8247863/01/webrev/ Testing: Matthias tested this on the affected system and it did pass for him. Docker tests on cgroup v1 and cgroup v2. Thanks, Severin