RFR: 8240698: LingeredApp does not pass getTestJavaOpts() to the children process if vmArguments is already specified
Chris Plummer
chris.plummer at oracle.com
Tue Mar 31 20:32:34 UTC 2020
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.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
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
More information about the serviceability-dev
mailing list