RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Fri Aug 4 19:35:12 UTC 2017
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.
>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: misha-rev03-white-box-api.hotspot.diff
URL: <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20170804/f77aa21f/misha-rev03-white-box-api.hotspot-0001.diff>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: misha-rev03-white-box-api.top.diff
URL: <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20170804/f77aa21f/misha-rev03-white-box-api.top-0001.diff>
More information about the hotspot-runtime-dev
mailing list