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

[Chris Plummer]

Hi Leonid,

I haven't gone through all the tests yet.  I've accumulated enough 
questions that I'd like to see them answered or addressed before I 
continue on.

This isn't directly related to your changes, but I noticed that users of 
JDKToolLauncher do nothing to make sure that default test options are 
used. This means we are never running these tools with the test options 
being specified with the jtreg run. Is that a bug or intentional?

In the problem lists, is it necessary to list the test multiple times 
with #id0, #id1, etc, or could you list it just once and leave that part 
off. It seems very error prone. Also, changing tests like ClhsdbFindPC, 
ClhsdbJstack, and ClhsdbScanOops to split out the testing in this manner 
seems completely unrelated to this CR, especially when the tests do not 
even contain any changes related to the CR.

  426     public static LingeredApp startApp(String... 
additionalJvmOpts) throws IOException {

The default test opts are appended to additionalJvmOpts, and if you want 
prepended you need to call Utils.prependTestJavaOpts(). I would have 
thought the opposite would be more desirable and expected default 
behavior. Why did you choose this way? I also find it somewhat confusing 
that there is even a default mode for where the additionalJvmOpts go. 
Maybe it would be best to have startAppAppendJvmArgs() and 
startAppPrependJvmArgs() just to make it explicit. This would also be in 
line with the existing startAppExactJvmOpts().

Is ClhsdbFindPC correct. It used to use just use -Xcomp or -Xint, 
ignoring any default test opts. You've fixed it to include the default 
test opts, but the are appended, possibly overriding the -Xcomp or 
-Xint. Don't we want the default test opts prepended? Same for ClhsdbJstack.

thanks,

Chris

On 3/25/20 2:31 PM, Leonid Mesnik 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
>>>>>>
>>>>
>>