RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri Aug 4 05:58:36 UTC 2017
Hi Mikhailo,
I put in your version of vmCDS() into this new webrev.
I also had to update the list of tests marked in hotspot,
as tests were removed and added in between, and resolved
it against the aot change:
http://cr.openjdk.java.net/~goetz/wr17/8185436-cdsKey/webrev.03/
http://cr.openjdk.java.net/~goetz/wr17/8185436-cdsKey/webrev.03-hs/
I don't think it's a good idea to swallow the exception silently
as you propose.
In our test setup, the tests would just be switched off if something
breaks, and no one will see that. If they fail though, it's an easy
and quick fix. I would at least switch them on, then one sees the
failing tests in case switching them on was the wrong guess.
Also, below, the method dump() throws an exception.
Best regards,
Goetz
> -----Original Message-----
> From: mikhailo [mailto:mikhailo.seledtsov at oracle.com]
> Sent: Tuesday, August 01, 2017 11:49 PM
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
>
> Hi Goetz,
>
> I have reviewed your updated changes, and they overall look good to me.
>
> However, I have some comments + concerns regarding VMProps.vmCDS():
>
>
> 1. Throwing exceptions from within the vmCDS() method.
>
> The VMProps properties are evaluated at the start of each run. If
> the exception is thrown here the whole test run will fail (not just the
> test that uses '@requires vm.cds'). The failure will occur shortly after
> the start of jtreg test run with a message:
> "java.lang.RuntimeException: Can not start VM to test to
> find out it's features. Switching off class data sharing (CDS)."
>
> Your method has 2 throw statements: "new RuntimeException("Can not
> start VM..." and "java.lang.RuntimeException: Can not start VM to test
> to...". I would recommend a more graceful way to fail, e.g. to print the
> message and to return "false" instead. This way the rest of the test run
> will continue, but the tests requiring vm.cds will be skipped with
> qualification of "not selected".
>
> 2. The check for "An error has occurred while processing the shared
> archive file." assumes that archive was not created prior to the
> execution of this evaluation code. However, there are test modes where
> archive is created prior to test run. We use such mode on regular basis.
> In such cases the code will fail.
> I recommend to run "-Xshare:on -version", and check the
> following match that would result in return of "true":
> "Java HotSpot.*sharing"
>
> 3. On occasion the mapping of shared archive region to a specified
> address will fail (due to system configuration, space already occupied,
> ASLR, etc.)
>
> Hence I recommend checking for such conditions as well:
>
> if (output.firstMatch("Unable to map") != null) {
> System.out.println("VMProps.vmCDS() encountered an archive
> mapping failure, still proceeding with vm.cds=true");
> return "true";
> }
> I am returning true here because seeing this output means that CDS
> feature is supported, however in this particular instance archive failed
> to map.
>
>
> The rest of the changes looks good to me.
>
> See for my version of VMProps.vmCDS() below. Let me know what you think.
>
>
> Thank you,
>
> Mikhailo
>
>
> ================== my update of VMProps.vmCDS()
>
> protected String vmCDS() {
> System.setProperty("test.jdk", System.getProperty("java.home"));
> ProcessBuilder pb =
> ProcessTools.createJavaProcessBuilder("-Xshare:on", "-version");
> OutputAnalyzer output;
>
> try {
> output = new OutputAnalyzer(pb.start());
> } catch (IOException e) {
> System.err.println( "Can not start VM to test to find out
> it's features. " +
> "Switching off class data
> sharing (CDS)." + e);
> return "false";
> }
> if (output.firstMatch("Shared spaces are not supported in this
> VM") != null) {
> return "false";
> }
> if (output.firstMatch("An error has occurred while processing
> the shared archive file.") != null) {
> return "true";
> }
> if (output.firstMatch("Java HotSpot.*sharing") != null) {
> return "true";
> }
> if (output.firstMatch("Unable to map") != null) {
> System.out.println("VMProps.vmCDS() encountered an archive
> mapping failure, still proceeding with vm.cds=true");
> return "true";
> }
>
> return "false";
> }
> ==================
>
>
>
> On 08/01/2017 07:20 AM, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > I made new webrevs implementing the change with @requires:
> > http://cr.openjdk.java.net/~goetz/wr17/8185436-cdsKey/webrev.02/
> > http://cr.openjdk.java.net/~goetz/wr17/8185436-cdsKey/webrev.02-hs/
> >
> > I also changed the bug description and synopsis.
> >
> > For the jtreg runner I would propose to set the property test.jdk
> > so that it is available in VMProps. Igor also ran into this issue.
> >
> > Best regards,
> > Goetz.
> >
> >
> >> -----Original Message-----
> >> From: Mikhailo Seledtsov [mailto:mikhailo.seledtsov at oracle.com]
> >> Sent: Montag, 31. Juli 2017 22:19
> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> >> Cc: hotspot-runtime-dev at openjdk.java.net
> >> Subject: Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds
> tests
> >>
> >> Hi Goetz,
> >>
> >> I have an idea on how to address your second use case.
> >> The idea is to define a special test property (e.g.
> >> test.cds.disable.cds.support) which will override logic inside the
> >> VMProps.vmCDSSupported(). If this property is defined to "true" in test
> >> invocation command then vmCDSSupported() returns false (CDS is
> disabled,
> >> not supported), and all tests marked with "@requires vm.cds.supported"
> >> will be skipped.
> >>
> >> How to use it:
> >> jtreg -Dtest.cds.disable.cds.support=true <tests-to-run>
> >> E.g.: jtreg -Dtest.cds.disable.cds.support=true
> >> hs/hotspot/test/runtime/SharedArchiveFile/ArchiveDoesNotExist.java
> >>
> >> I prototyped this approach, it works for me. I have attached the diff.
> >> Let me know whether this works for your use case, or if you have any
> >> questions.
> >>
> >>
> >> Thank you,
> >> Mikhailo
> >>
> >>
> >> On 7/31/17, 1:45 AM, Lindenmaier, Goetz wrote:
> >>> Hi Mikhailo,
> >>>
> >>> Basically I'm fine with using the @requires property.
> >>> But is there a way to overrule the outcome of the method
> >>> implemented In VMProps.java computing the property?
> >>> I have two use cases for the key I want to introduce.
> >>>
> >>> First, our internal VM (we are Oracle licensees) is compiled without
> >>> CDS support. Thus we don't want to run the CDS tests. Currently
> >>> we have them all listed in the ProblemList, but that's not nice, especially
> >>> because we have to adapt it whenever a new test is added.
> >>> As I understand, the @requires property works fine, here.
> >>>
> >>> Second, we also test the two ports we contributed (ppc and s390). These
> >> contain
> >>> rudimentary cds support and so far passed all tests. Unfortunately it
> broke
> >>> lately in jdk10. Instead of fixing it (our people are working on finishing
> our
> >>> internal Java 9 port) I would like to switch off all cds tests.
> >>> As I can set the key on the command line of jtreg, I easily can do that.
> >>> Is there a way to do similar with the @requires property?
> >>>
> >>> Best regards,
> >>> Goetz.
> >>>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Mikhailo Seledtsov [mailto:mikhailo.seledtsov at oracle.com]
> >>>> Sent: Freitag, 28. Juli 2017 23:53
> >>>> To: Lindenmaier, Goetz<goetz.lindenmaier at sap.com>
> >>>> Cc: hotspot-runtime-dev at openjdk.java.net
> >>>> Subject: Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds
> >> tests
> >>>> Hi Goetz,
> >>>>
> >>>> I am a HotSpot SQE Engineer at Oracle. I have discussed your
> proposed
> >>>> fix with Igor Ignatyev (also VM SQE Engineer), and we have the
> following
> >>>> feedback on this change.
> >>>>
> >>>> 1. As part of streamlining and simplifying SQE process and the use of
> >>>> test tools we have narrowed down the test selection mechanisms.
> >>>>
> >>>> 2. Our preferred test selection mechanism is use of "@requires" and a
> >>>> corresponding test/jtreg-ext/requires/VMProps.java. Even though
> JTREG
> >>>> supports use of "@key", we prefer the use of "@requires" as a first
> >> choice.
> >>>> 3. If it is not possible to use "@requires" for a given situation then
> >>>> use "@key" mechanism. We would ask you if you could explore the
> >>>> possibility of implementing this change via @requires first.
> >>>>
> >>>>
> >>>> Here are several hints that may help:
> >>>>
> >>>> 1. Take a look at<top>/test/jtreg-ext/requires/VMProps.java. The value
> >>>> of a given "requires property" is evaluated inside this file and placed
> >>>> into a map (see public call() method). Add your evaluation code here,
> >>>> and then follow the pattern used for other properties. Create a
> property
> >>>> (e.g. vm.cds.supported, with values of true/false). Create a method
> that
> >>>> evaluates the property value (e.g. isCDSSupported() or similar).
> >>>>
> >>>> 2. The method could use several options to evaluate whether CDS is
> >>>> supported.
> >>>> A. WhiteBox API. Create a new WB test API method which can
> return
> >>>> true if CDS_ compiler flag is defined, otherwise false.
> >>>> Call WB API from VMProps.java. See
> >>>> WB.getBooleanVMFlag("EnableJVMCI") as an example. Or create your
> >> own
> >>>> WB.isCDSSupported()
> >>>> WhiteBox.java resides in test/lib/sun/hotspot/WhiteBox.java
> >>>>
> >>>> B. Another options is to evaluate by running VM with sharing on and
> >>>> checking the return (may be not as reliable as option A)
> >>>> C. Other ideas welcome.
> >>>>
> >>>> 3. Include "@requres vm.cds.supported == true" to the appropriate
> tests.
> >>>>
> >>>> Let me know if you have any questions.
> >>>>
> >>>>
> >>>> Best regards,
> >>>> Mikhailo
> >>>>
> >>>>
> >>>> On 7/28/17, 12:58 AM, Lindenmaier, Goetz wrote:
> >>>>> Hi
> >>>>>
> >>>>> we compile the VM without CDS support. Thus the CDS tests
> >>>>> fail. This change introduces a keyword 'cds' and marks
> >>>>> the tests accordingly.
> >>>>> This change also fixes the keywords specified in
> >>>> gc/g1/TestSharedArchiveWithPreTouch.java.
> >>>>> There may only be one @key keyword in the test specification.
> >>>>> In runtime/CompressedOops/CompressedClassPointers.java only one
> >> test
> >>>>> case required CDS. I changed this sub case to succeed if CDS is not
> >>>>> available.
> >>>>>
> >>>>> Please review this change. I please need a sponsor.
> >>>>> http://cr.openjdk.java.net/~goetz/wr17/8185436-cdsKey/webrev.01/
> >>>>>
> >>>>> Best regards,
> >>>>> Goetz.
More information about the hotspot-runtime-dev
mailing list