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