Here is new webrev:
http://cr.openjdk.java.net/~lmesnik/8240698/webrev.03/ 
<http://cr.openjdk.java.net/~lmesnik/8240698/webrev.03/>

The only difference is updated startApp() method and it's comments:
http://cr.openjdk.java.net/~lmesnik/8240698/webrev.03/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
 
<http://cr.openjdk.java.net/~lmesnik/8240698/webrev.03/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html>

Leonid

> On Mar 31, 2020, at 1:32 PM, Chris Plummer <chris.plum...@oracle.com> wrote:
> 
> On 3/31/20 12:09 PM, Leonid Mesnik wrote:
>> Hi
>> 
>> On 3/30/20 9:43 PM, Chris Plummer wrote:
>>> Hi Leonid,
>>> 
>>> On 3/30/20 5:42 PM, Leonid Mesnik wrote:
>>>> Hi
>>>> 
>>>> See my comments inline. I will update webrev after go through all your 
>>>> comments.
>>>> 
>>>> 
>>>> On 3/30/20 11:39 AM, Chris Plummer wrote:
>>>>> 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?
>>>> 
>>>> Which "default test options" do you mean? We have 2 properties to set JVM 
>>>> options. The idea is to pass test.vm.opts to ALL java processes and 
>>>> test.java.opts  to only tested processes if applicable. Usually, for 
>>>> example we don't want to run jcmd with -Xcomp. test.vm.opts was used (a 
>>>> long time ago) for options like '-d32/-d64' on Solaris where JVM don't 
>>>> start without choosing correct version. Also, it is used to reduce maximum 
>>>> heap for all JVM instances when tests are running concurrently.
>>>> 
>>>> So, probably test.vm.opts (or test.vm.tools.opts) should be added by 
>>>> JDKToolLauncher but not test.java.opts. It is separate topic, there are a 
>>>> lot of launchers which ignore test.vm.opts now.
>>> I always get confused about which set of options these properties 
>>> represent, but basically I'm suggesting that if for example we are doing a 
>>> -Xcomp run in mach5, JDKToolLauncher (at least in some cases) should be 
>>> launched with this option. I think this is what you get from 
>>> Utils.getTestJavaOpts(),.
>>> 
>>> For example the SA tests use JDKToolLauncher.createUsingTestJDK("jhsdb"). 
>>> jhsdb is what is really being tested here, and it should be launched with 
>>> the test vm options. Currently we launch the target process with these 
>>> options, which is probably also a good idea.  Also we aren't too concerned 
>>> with the options that the test itself is run with, although I'm guessing 
>>> they also get run with the test java opts. So we have 3 processes here:
>>>  - jhsdb, which should be getting test java opts but is not
>>>  - the target process, which should be getting test java opts and currently 
>>> is
>>>  - the test itself, where options don't really matter, but is getting 
>>> passed test java opts
>>> 
>>> However, you could argue that tests like jinfo, jstack, and jcmd, all of 
>>> which use the Attach API and the bulk of the work is done on the target 
>>> process, are not that concerned with the options passed to the command, but 
>>> do want the options passed to the target process.
>> 
>> Well, it is a good question if we want to run jhsdb tool itself with 
>> additional slow options like Xcomp. Does it help us to improve coverage? 
>> IIRC the original idea of adding test.java/vm.opts was to don't waste time 
>> executing javac and debuggers in slow mode on SPARC.
>> 
>> Anyway, it is a separate question which is out of scope of this change. We 
>> might want to review all debugger/debugee tests to find better way to deal 
>> with this.
> Might be good to get an RFE filed for this.
>> 
>>>> 
>>>>> 
>>>>> 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.
>>>> 
>>>> I think, that these chages are related. The startApp(...) was updated so 
>>>> some test combinations become invalid or redundant.
>>>> 
>>>> ClhsdbFindPC and ClhsdbJstack were always run twice. Now, when test 
>>>> options passed in test it is not needed to run it twice when Xcomp is 
>>>> already set by user.
>>>> 
>>> Ok. I see now that the second test run, which is the non -Xcomp run, adds 
>>> '@requires vm.compMode != "Xcomp"'. But this also is strange. The first 
>>> test run, which does not have the @requires and is the one that makes 
>>> LingeredApp launch with -Xcomp, will always run whether or not it is an 
>>> -Xcomp test run. So it will run as part of the a regular test run and as 
>>> part of a -Xcomp test run. The only difference between the two is the 
>>> -Xcomp run will also run the test with -Xcomp, but that's not really needed 
>>> (I think it will also end up passing -Xcomp to the target processs twice). 
>>> Perhaps '@requires vm.compMode == "Xcomp"' should be used for the first 
>>> test run, but that means it no longer gets run until later tiers when we 
>>> use -Xcomp. Why not revert it back to a single test, but also add 
>>> '@requires vm.compMode != "Xcomp"'. Then it gets run both ways in an early 
>>> tier and not run during the -Xcomp run, which isn't really needed.
>> 
>> There several flag which are executed with Xcomp only: 
>> "-XX:-DoEscapeAnalysis",  "-XX:-UseBiasedLocking", "-XX:+DeoptimizeALot" 
>> where this test is going to be skipped. So we never run test with these 
>> options.
>> 
>> The original idea is to run test with given options and with added Xcomp.  I 
>> left logic the same and only skip run with "Xcomp" when it is set already by 
>> user. I agree that we have some duplication here and it could be improved, 
>> but it could be done separately. If you are ok with this let me file 
>> separate RFE for this.
> Ok.
>> 
>>> 
>>>> ClhsdbScanOops is fixed to don't allow to run incompatible GC combination.
>>> Ok
>>>> 
>>>> So I should update these tests by splitting them or change them to  
>>>> startAppExactJvmOpts() if we wan't continue to ignore user-given test 
>>>> options.
>>> I don't think I was suggesting removing user-given test options. I don't 
>>> see why you would.
>> 
>> I just wanted to say that these tests are affected by my changes and should 
>> be fixed anyway.
> Ok.
> 
> So I think the one change you agreed to make is have the default be to append 
> test vm opts rather than prepend them. Let me know when you have a new webrev.
> 
> thanks,
> 
> Chris
>> 
>> Leonid
>> 
>>>> 
>>>> It seems that #idN are required by jtreg now, otherwise it just run test.
>>> Ok.
>>>> 
>>>>> 
>>>>>  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().
>>>>> 
>>>> I've chosen the most popular usage, which was Utils.appendTestJavaOpts. 
>>>> But I agree, that it would be better to change it to prepend. Thanks for 
>>>> pointing to this.
>>>> 
>>>> I don't want to add startAppAppendJvmArgs()/startAppPrependJvmArgs() to 
>>>> don't complicate all things. I think that startApp() should be used in the 
>>>> cases when test vm options really shouldn't interfere with user-provided 
>>>> options or overwrite them. So basically the behavior is the same as for 
>>>> ProcessTools.createJavaProcessBuilder(true, ...) and jtreg itself.
>>>> 
>>> Ok.
>>>> 
>>>>> 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.
>>>> 
>>>> The idea is to don't mix Xcomp and Xmixed/Xint using requires filter. 
>>>> However ClhsdbFindPC might override Xint with Xmixed if it is set 
>>>> explicitly. Switching to prepending will fix it.
>>> Yes, that's what I was thinking and one reason I thought that should be 
>>> default behavior.
>>> 
>>> thanks,
>>> 
>>> Chris
>>>> 
>>>> Leonid
>>>> 
>>>>> 
>>>>> 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.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/
>>>>>>>> 
>>>>>>>> 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> 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

Reply via email to