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

[Leonid Mesnik]

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

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 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/
>>
>> 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/
>>>>
>>>> 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> 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/
>>>>>>>
>>>>>>> bug: 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/7dd00480/attachment.htm>