RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
Ioi Lam
ioi.lam at oracle.com
Mon Aug 7 17:44:14 UTC 2017
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