RFR: 8242009: Review setting test.java/vm.opts in jcmd/jhsdb and debugger in serviceability tests

[Chris Plummer]

Hi Daniil,

Looks good.

thanks,

Chris

On 5/11/20 9:43 AM, Daniil Titov wrote:
> Hi Chris,
>
> Please review a new version of the fix[1] that also updates jdk/sun/tools  tests.
>
> Testing: Mach5 tier1-tier7 tests successfully passed.
>
>
> [1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.03/
> [2] ] https://bugs.openjdk.java.net/browse/JDK-8242009
>
> Thank you,
> Daniil
>
> On 5/8/20, 12:21 AM, "Chris Plummer" <chris.plummer at oracle.com> wrote:
>
>      Hi Daniil,
>
>      I just noticed you didn't update the tests in jdk/sun/tools/jhsdb. Do
>      you think these should be done also?
>
>      Chris
>
>      On 5/7/20 11:44 PM, Chris Plummer wrote:
>      > Hi Daniil,
>      >
>      > The changes look good.
>      >
>      > thanks,
>      >
>      > Chris
>      >
>      > On 5/4/20 1:05 PM, Daniil Titov wrote:
>      >> Hi Chris,
>      >>
>      >> Please review a new version of webrev [1] that addresses your comments.
>      >>
>      >> For the following 3 tests that showed the increase of the execution
>      >> time with -Xcomp
>      >> more than 5 minutes this version of the change  strips -Xcomp option
>      >> when
>      >> forwarding VM  arguments to  j*-tools  :
>      >>
>      >>     -- serviceability/sa/sadebugd/SADebugDTest.java,
>      >>     -- serviceability/sa/sadebugd/DebugdConnectTest.java,
>      >>     -- serviceability/sa/ClhsdbJstackXcompStress.java
>      >>
>      >> The execution time for the rest of the tests when running with -Xcomp
>      >> was increased
>      >> within 1 and half minute.
>      >>
>      >>
>      >> [1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.02/
>      >> [2] https://bugs.openjdk.java.net/browse/JDK-8242009
>      >>
>      >> Thank you,
>      >>   Daniil
>      >>
>      >>
>      >> On 4/27/20, 12:55 PM, "Chris Plummer" <chris.plummer at oracle.com> wrote:
>      >>
>      >>      Hi Daniil,
>      >>
>      >>      Overall it looks good. A few comments below.
>      >>
>      >>      Can you add a comment to TestSysProps.java indicating the reason
>      >> for
>      >>      checking if the line starts with "[".
>      >>
>      >>      In JDKToolLauncher you have an extra empty line:
>      >>
>      >>        112      * Any platform specific arguments required for
>      >> running the
>      >>      tool are
>      >>        113      * automatically added.
>      >>        114      *
>      >>        115      *
>      >>        116      * @param args
>      >>
>      >>      In OutputAnalyzer.java, I wonder if all these matching APIs you
>      >> updated
>      >>      should by default just include the version output in their
>      >> filtering.
>      >>
>      >>      As for the added time to execute, I would suggest possibly
>      >> stripping
>      >>      -Xcomp from the few outliers, and I would mostly focus on how much
>      >>      longer it takes, not how many times longer. For example,
>      >> increasing from
>      >>      10 seconds to 40 seconds is not a big deal. Increasing from 10
>      >> minutes
>      >>      to 20 minutes is.
>      >>
>      >>      SADebugDTest creates 8 tool processes so, that's probably the
>      >> reason for
>      >>      the big increase, although I'm not sure why it is kind of slow
>      >> in the
>      >>      first place. It does nothing more on each iteration than launch
>      >> "jhsdb
>      >>      debugd", which will connect to the debuggee, and then is killed
>      >> off.
>      >>      Maybe there is something slow with connecting, especial with RMI.
>      >>
>      >>      thanks,
>      >>
>      >>      Chris
>      >>
>      >>      On 4/27/20 12:07 PM, Daniil Titov wrote:
>      >>      > Please review the change [1] that ensures that VM and test
>      >> options are forwarded to
>      >>      > j*-tools when they are launched from serviceability/sa tests.
>      >>      >
>      >>      > The tests that expect an empty output  were corrected to
>      >> ignore the product version printed
>      >>      > in the error stream since in some  tiers the tests are run
>      >> with ' -showversion' VM option (tier3).
>      >>      >
>      >>      > In test serviceability/sa/TestSysProps.java the code that
>      >> counts the system properties  was  corrected
>      >>      > to ignore the debug output when the test is run with "
>      >> -Xlog:cds=debug" option (tier4).
>      >>      >
>      >>      > Testing:  Mach5 tests for tier1 - tier7 passed.
>      >>      >
>      >>      > I also run the test with -XComp at Mach5 linux-x64-debug
>      >> builds before and after the changes
>      >>      > and for  the most of the tests the  overhead is about 2 times
>      >> although for
>      >>      > serviceability/sa/sadebugd/SADebugDTest.java it spikes up to 5
>      >> times.  Probably at least for some tests
>      >>      > it makes to filter out some properties (e.g. -Xcomp) before
>      >> forwarding them to j*-tools.
>      >>      >
>      >>      > serviceability/sa/sadebugd/SADebugDTest.java, before : 2m 23s
>      >> ,  after:11m 07s
>      >>      > serviceability/sa/sadebugd/TestJmapCore.java,  before : 42s ,
>      >> after:1m 09s
>      >>      > serviceability/sa/TestSysProps.java, before : 36s , after: 1m 27s
>      >>      >
>      >>      >
>      >>      > [1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.01
>      >>      > [2] https://bugs.openjdk.java.net/browse/JDK-8242009
>      >>      >
>      >>      > Thank you,
>      >>      > Daniil
>      >>      >
>      >>      >
>      >>
>      >>
>      >>
>      >>
>      >
>      >
>
>
>