RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Tue Aug 8 09:00:20 UTC 2017
Hi Mikhailo,
yes, I please need a sponsor.
Thanks for the help with working on this change!
I added Ioi as reviewer in the webrev, so the patches
can be pushed as is.
Thanks,
Goetz.
> -----Original Message-----
> From: Mikhailo Seledtsov [mailto:mikhailo.seledtsov at oracle.com]
> Sent: Dienstag, 8. August 2017 00:01
> To: Ioi Lam <ioi.lam at oracle.com>
> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Igor Ignatyev
> <igor.ignatyev at oracle.com>; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
>
> Hi Goetz,
>
> Please let me know if you need a sponsor for this change.
>
> Mikhailo
>
> On 8/7/17, 10:44 AM, Ioi Lam wrote:
> > Looks good to me, too. Reviewed.
> >
> > Thanks
> >
> > - Ioi
> >
> >
> >
> > On 8/7/17 9:46 AM, Mikhailo Seledtsov wrote:
> >> The change looks good to me,
> >>
> >> Thank you,
> >> Mikhailo
> >>
> >> On 8/7/17, 1:02 AM, Lindenmaier, Goetz wrote:
> >>> 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