> On Mar 25, 2020, at 3:36 PM, Leonid Mesnik <leonid.mes...@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.mes...@oracle.com
>>> <mailto: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
>>>>>>>>
>>>>>>
>>>>
>>
