RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
Ioi Lam
ioi.lam at oracle.com
Fri Aug 4 06:39:32 UTC 2017
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