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

[Leonid Mesnik]

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/b2feff5d/attachment.htm>