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.
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.
ClhsdbScanOops is fixed to don't allow to run incompatible GC combination.
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.
It seems that #idN are required by jtreg now, otherwise it just run test.
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.
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.
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
