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
