RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Mon Aug 7 16:46:40 UTC 2017
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