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
