> Test ClhsdbJstack.java is updated. > now you reduced coverage provided by this test, I actually meant to create a separate jtreg test description in this test and pass "Xcomp" or "true" (or anything) as an argument to ClhsdbJstack, and use the value of this argument to decide if -Xcomp should be added to LingeredApp.startApp or not.
Thanks, -- Igor > On Mar 25, 2020, at 2:31 PM, Leonid Mesnik <leonid.mes...@oracle.com> wrote: > > Igor, Stefan, Ioi > > Thank you for your feedback. > > Filed https://bugs.openjdk.java.net/browse/JDK-8241624 > <https://bugs.openjdk.java.net/browse/JDK-8241624> To change @run main... to > @run driver. > > Test ClhsdbJstack.java is updated. > > Still waiting for review from SVC team. > > webrev: http://cr.openjdk.java.net/~lmesnik/8240698/webrev.02/ > <http://cr.openjdk.java.net/~lmesnik/8240698/webrev.02/> > Leonid > > On 3/25/20 12:46 PM, Igor Ignatyev wrote: >> Hi Leonid, >> >> not related related to your patch (but yet somewhat made more obvious by >> it), it seems all (or at least almost all) the tests which use LingeredApp >> should be run in "driver" mode as they just orchestrate execution of other >> JVMs, so running them w/ main (let alone main/othervm) just wastes time, >> test/hotspot/jtreg/serviceability/sa/ClhsdbJstack.java#id1, for example, >> will now executed w/ Xcomp which will make it very slow for no reasons. >> since you already got your hands dirty w/ these tests, could you please file >> an RFE to sort this out and list all the affected tests there? >> >> re: the patch, could you please update ClhsdbJstack.java test not to be run >> w/ Xcomp and follow the same pattern you used in other tests (e.g. >> ClhsdbScanOops) ? other than that it looks fine to me, I however wouldn't be >> able to tell if all svc tests continue to do that they were supposed to, so >> I'd prefer for someone from svc team to chime in. >> >> Thanks, >> -- Igor >> >>> On Mar 25, 2020, at 12:01 PM, Leonid Mesnik <leonid.mes...@oracle.com >>> <mailto:leonid.mes...@oracle.com>> wrote: >>> >>> Added Ioi, who also proposed new version of startAppVmOpts. >>> >>> Please find new webrev: >>> http://cr.openjdk.java.net/~lmesnik/8240698/webrev.01/ >>> <http://cr.openjdk.java.net/~lmesnik/8240698/webrev.01/> >>> Renamed startAppVmOpts/runAppVmOpts to >>> "startAppExactJvmOpts/runAppExactJvmOpts" is used. It should make very >>> clear that this method doesn't use any of test.java.opts, test.vm.opts. >>> >>> Also, I fixed test/hotspot/jtreg/serviceability/sa/ClhsdbFlags.java >>> metnioned by Igor, and removed null pointer check as Ioi suggested in >>> startApp method. >>> >>> + public static void startApp(LingeredApp theApp, String... >>> additionalJvmOpts) throws IOException { >>> + startAppExactJvmOpts(theApp, >>> Utils.appendTestJavaOpts(additionalJvmOpts)); >>> + } >>> >>> Leonid >>> >>> On 3/25/20 10:14 AM, Stefan Karlsson wrote: >>>> On 2020-03-25 17:40, Igor Ignatyev wrote: >>>>> Hi Leonid, >>>>> >>>>> I have briefly looked at the patch, a few comments so far: >>>>> >>>>> test/hotspot/jtreg/serviceability/sa/ClhsdbFlags.java: >>>>> - at L#114, could you please call static method using class name (as >>>>> the opposite of using instance)? or was it meant to be >>>>> theApp.runAppVmOpts(vmArgs) ? >>>>> >>>>> test/lib/jdk/test/lib/apps/LingeredApp.java: >>>>> - it seems that code indent of startApp(LingeredApp, String[]) isn't >>>>> correct >>>>> - I don't like startAppVmOpts name, but unfortunately don't have a better >>>>> suggestion (yet) >>>> >>>> I was going to say the same. Jtreg has the concept of "java options" and >>>> "vm options". We have had a fair share of bugs and wasted time when tests >>>> have been using the "vm options" part (VM_OPTIONS, test.vm.options, etc), >>>> and we've been moving away from using that way to pass options. I recently >>>> cleaned up some of this with: >>>> >>>> 8237111: LingeredApp should be started with getTestJavaOpts >>>> >>>> Because of this, I would prefer if we used a name that doesn't include >>>> "VmOpts", because it's too alike the other concept. Some suggestions: >>>> startAppJavaOptions >>>> startAppUsingJavaOptions >>>> startAppWithJavaOptions >>>> startAppExactJavaOptions >>>> startAppJvmOptions >>>> >>>> Thanks, >>>> StefanK >>>> >>>>> Thanks, >>>>> -- Igor >>>>> >>>>>> On Mar 25, 2020, at 8:55 AM, Leonid Mesnik <leonid.mes...@oracle.com> >>>>>> <mailto:leonid.mes...@oracle.com> wrote: >>>>>> >>>>>> Hi >>>>>> >>>>>> Could you please review following fix which change LingeredApp to >>>>>> prepend vm options to java/vm.test.opts when startApp is used and >>>>>> provide startAppVmOpts to override options completely. >>>>>> >>>>>> The intention is to avoid issue like in this bug where test/jtreg >>>>>> options were ignored by tests. Also I fixed some tests where intention >>>>>> was to append vm options rather than to override them. >>>>>> >>>>>> webrev: http://cr.openjdk.java.net/~lmesnik/8240698/webrev.00/ >>>>>> <http://cr.openjdk.java.net/~lmesnik/8240698/webrev.00/> >>>>>> >>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8240698 >>>>>> <https://bugs.openjdk.java.net/browse/JDK-8240698> >>>>>> >>>>>> Leonid >>>>>> >>>> >>
