RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Mon Aug 7 08:02:59 UTC 2017
Hi,
webrev with Whitebox:
http://cr.openjdk.java.net/~goetz/wr17/8185436-cdsKey/webrev.04/
http://cr.openjdk.java.net/~goetz/wr17/8185436-cdsKey/webrev.04-hs/
I don't see so much of a difference to throwing an exception, if
Whitebox is not properly implemented you get one, anyways:
Exception in thread "main" java.lang.UnsatisfiedLinkError: sun.hotspot.WhiteBox.isCDSIncludedInVmBuild()Z
at sun.hotspot.WhiteBox.isCDSIncludedInVmBuild(Native Method)
Maybe it's a bit less likely to break, though.
I'm fine with this, too.
Best regards,
Goetz.,
> -----Original Message-----
> From: Mikhailo Seledtsov [mailto:mikhailo.seledtsov at oracle.com]
> Sent: Freitag, 4. August 2017 21:35
> To: Ioi Lam <ioi.lam at oracle.com>; Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com>; Igor Ignatyev <igor.ignatyev at oracle.com>
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
>
> Hi,
>
> I have an alternative solution that is IMO rather simple, reliable
> and will
> solve some issues we discussed (e.g. no need to throw exceptions, no
> need to handle failure to map an archive).
> The proposed solution uses White Box test API to determine whether VM
> is compiled with INCLUDE_CDS on or off.
> I implemented and tested it today, it works for me.
>
> The patch is attached. Please let me know what you think.
>
> Thank you,
> Mikhailo
>
> On 8/3/17, 11:39 PM, Ioi Lam wrote:
> > Hi Goetz,
> >
> > Instead of testing -Xshare:on, I think you should test with
> > -Xshare:auto, which sets the flags
> >
> > UseSharedSpaces = true
> > RequireSharedSpaces = false
> >
> > and will reliably print "Shared spaces are not supported in this VM"
> > if-and-only-if INCLUDE_CDS is disabled (see arguments.cpp):
> >
> >
> > #if !INCLUDE_CDS
> > if (DumpSharedSpaces || RequireSharedSpaces) {
> > jio_fprintf(defaultStream::error_stream(),
> > "Shared spaces are not supported in this VM\n");
> > return JNI_ERR;
> > }
> > if ((UseSharedSpaces && FLAG_IS_CMDLINE(UseSharedSpaces)) ||
> > log_is_enabled(Info, cds)) {
> > warning("Shared spaces are not supported in this VM");
> > FLAG_SET_DEFAULT(UseSharedSpaces, false);
> > LogConfiguration::configure_stdout(LogLevel::Off, true,
> > LOG_TAGS(cds));
> > }
> > no_shared_spaces("CDS Disabled");
> > #endif // INCLUDE_CDS
> >
> >
> > That way, you don't need to test any other output message or exit
> > conditions(such as mapping error).
> >
> >
> > E.g.:
> >
> > ioilinux /jdk/iter/build/linux-x64$ ./images/jdk/bin/java -Xshare:auto
> > -version
> > java version "10-internal"
> > Java(TM) SE Runtime Environment (build
> > 10-internal+0-2017-08-04-0614567.iklam.iter)
> > Java HotSpot(TM) 64-Bit Server VM (build
> > 10-internal+0-2017-08-04-0614567.iklam.iter, mixed mode)
> >
> >
> > ioilinux /jdk/iter/build/linux-x64$ ./images/jdk/bin/java
> > -XXaltjvm=minimal -Xshare:auto -version
> > Java HotSpot(TM) 64-Bit Minimal VM warning: Shared spaces are not
> > supported in this VM
> > java version "10-internal"
> > Java(TM) SE Runtime Environment (build
> > 10-internal+0-2017-08-04-0614567.iklam.iter)
> > Java HotSpot(TM) 64-Bit Minimal VM (build
> > 10-internal+0-2017-08-04-0614567.iklam.iter, mixed mode)
> >
> >
> >
> > Thanks
> > - Ioi
> >
> > On 8/3/17 10:58 PM, Lindenmaier, Goetz wrote:
> >> 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