Thank you for review. Filed https://bugs.openjdk.java.net/browse/JDK-8242009 <https://bugs.openjdk.java.net/browse/JDK-8242009> for issue with passing arguments into debugger/tools processes.
Leonid > On Apr 1, 2020, at 11:21 AM, Chris Plummer <chris.plum...@oracle.com> wrote: > > Hi Leonid, > > Looks good. Thanks for cleaning this up! > > Chris > > On 3/31/20 4:12 PM, Leonid Mesnik wrote: >> 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 >>> <mailto: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 >>>>>>>> <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> >>>>>>>>>> <mailto: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 >> >
