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 04:43:13 UTC 2020
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.
>
>>
>> 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.
> 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.
>
> 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