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
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
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
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
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
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
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
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
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
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
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
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
