RFR: 8240698: LingeredApp does not pass getTestJavaOpts() to the children process if vmArguments is already specified

[Igor Ignatyev]

> On Mar 25, 2020, at 3:36 PM, Leonid Mesnik <leonid.mesnik at oracle.com> wrote:
> 
> 
> 
> On 3/25/20 2:58 PM, Igor Ignatyev wrote:
>>> 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.
> Seems I misinterpret you words. Do you mean to change it to this? Basically the same as my original but faster and better prepared for "@run driver".
> 
> http://cr.openjdk.java.net/~lmesnik/8240698/webrev.02/test/hotspot/jtreg/serviceability/sa/ClhsdbJstack.java.udiff.html <http://cr.openjdk.java.net/~lmesnik/8240698/webrev.02/test/hotspot/jtreg/serviceability/sa/ClhsdbJstack.java.udiff.html>
> 
yeap.
> 
> Leonid
> 
>> 
>> Thanks,
>> -- Igor
>> 
>> 
>>> On Mar 25, 2020, at 2:31 PM, Leonid Mesnik <leonid.mesnik at oracle.com <mailto:leonid.mesnik at 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.mesnik at oracle.com <mailto:leonid.mesnik at 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.mesnik at oracle.com> <mailto:leonid.mesnik at 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 
>>>>>>>> 
>>>>>> 
>>>> 
>> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200325/ea8ba2d8/attachment-0001.htm>