RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Lindenmaier, Goetz
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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Jiangli Zhou
Hi Lindenmaier,

This looks reasonable. I can sponsor you after one additional review approval for the change.

Thanks,
Jiangli

> On Jul 28, 2017, at 12:58 AM, Lindenmaier, Goetz <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Mikhailo Seledtsov
In reply to this post by Lindenmaier, Goetz
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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Lindenmaier, Goetz
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:[hidden email]]
> Sent: Freitag, 28. Juli 2017 23:53
> To: Lindenmaier, Goetz <[hidden email]>
> Cc: [hidden email]
> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Mikhailo Seledtsov
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:[hidden email]]
>> Sent: Freitag, 28. Juli 2017 23:53
>> To: Lindenmaier, Goetz<[hidden email]>
>> Cc: [hidden email]
>> 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.

CDS-testing-at-requires-01.diff (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Lindenmaier, Goetz
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:[hidden email]]
> Sent: Montag, 31. Juli 2017 22:19
> To: Lindenmaier, Goetz <[hidden email]>
> Cc: [hidden email]
> 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:[hidden email]]
> >> Sent: Freitag, 28. Juli 2017 23:53
> >> To: Lindenmaier, Goetz<[hidden email]>
> >> Cc: [hidden email]
> >> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Mikhailo Seledtsov
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:[hidden email]]
>> Sent: Montag, 31. Juli 2017 22:19
>> To: Lindenmaier, Goetz <[hidden email]>
>> Cc: [hidden email]
>> 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:[hidden email]]
>>>> Sent: Freitag, 28. Juli 2017 23:53
>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>> Cc: [hidden email]
>>>> 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Lindenmaier, Goetz
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:[hidden email]]
> Sent: Tuesday, August 01, 2017 11:49 PM
> To: Lindenmaier, Goetz <[hidden email]>
> Cc: [hidden email]
> 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:[hidden email]]
> >> Sent: Montag, 31. Juli 2017 22:19
> >> To: Lindenmaier, Goetz <[hidden email]>
> >> Cc: [hidden email]
> >> 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:[hidden email]]
> >>>> Sent: Freitag, 28. Juli 2017 23:53
> >>>> To: Lindenmaier, Goetz<[hidden email]>
> >>>> Cc: [hidden email]
> >>>> 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Ioi Lam
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:[hidden email]]
>> Sent: Tuesday, August 01, 2017 11:49 PM
>> To: Lindenmaier, Goetz <[hidden email]>
>> Cc: [hidden email]
>> 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:[hidden email]]
>>>> Sent: Montag, 31. Juli 2017 22:19
>>>> To: Lindenmaier, Goetz <[hidden email]>
>>>> Cc: [hidden email]
>>>> 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:[hidden email]]
>>>>>> Sent: Freitag, 28. Juli 2017 23:53
>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>> Cc: [hidden email]
>>>>>> 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Mikhailo Seledtsov
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:[hidden email]]
>>> Sent: Tuesday, August 01, 2017 11:49 PM
>>> To: Lindenmaier, Goetz <[hidden email]>
>>> Cc: [hidden email]
>>> 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:[hidden email]]
>>>>> Sent: Montag, 31. Juli 2017 22:19
>>>>> To: Lindenmaier, Goetz <[hidden email]>
>>>>> Cc: [hidden email]
>>>>> 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:[hidden email]]
>>>>>>> Sent: Freitag, 28. Juli 2017 23:53
>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>> Cc: [hidden email]
>>>>>>> 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.
>

misha-rev03-white-box-api.hotspot.diff (2K) Download Attachment
misha-rev03-white-box-api.top.diff (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Lindenmaier, Goetz
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:[hidden email]]
> Sent: Freitag, 4. August 2017 21:35
> To: Ioi Lam <[hidden email]>; Lindenmaier, Goetz
> <[hidden email]>; Igor Ignatyev <[hidden email]>
> Cc: [hidden email]
> 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:[hidden email]]
> >>> Sent: Tuesday, August 01, 2017 11:49 PM
> >>> To: Lindenmaier, Goetz <[hidden email]>
> >>> Cc: [hidden email]
> >>> 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:[hidden email]]
> >>>>> Sent: Montag, 31. Juli 2017 22:19
> >>>>> To: Lindenmaier, Goetz <[hidden email]>
> >>>>> Cc: [hidden email]
> >>>>> 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:[hidden email]]
> >>>>>>> Sent: Freitag, 28. Juli 2017 23:53
> >>>>>>> To: Lindenmaier, Goetz<[hidden email]>
> >>>>>>> Cc: [hidden email]
> >>>>>>> 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.
> >
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Mikhailo Seledtsov
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:[hidden email]]
>> Sent: Freitag, 4. August 2017 21:35
>> To: Ioi Lam<[hidden email]>; Lindenmaier, Goetz
>> <[hidden email]>; Igor Ignatyev<[hidden email]>
>> Cc: [hidden email]
>> 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:[hidden email]]
>>>>> Sent: Tuesday, August 01, 2017 11:49 PM
>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>> Cc: [hidden email]
>>>>> 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:[hidden email]]
>>>>>>> Sent: Montag, 31. Juli 2017 22:19
>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>> Cc: [hidden email]
>>>>>>> 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:[hidden email]]
>>>>>>>>> Sent: Freitag, 28. Juli 2017 23:53
>>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>>>> Cc: [hidden email]
>>>>>>>>> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Ioi Lam
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:[hidden email]]
>>> Sent: Freitag, 4. August 2017 21:35
>>> To: Ioi Lam<[hidden email]>; Lindenmaier, Goetz
>>> <[hidden email]>; Igor Ignatyev<[hidden email]>
>>> Cc: [hidden email]
>>> 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:[hidden email]]
>>>>>> Sent: Tuesday, August 01, 2017 11:49 PM
>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>> Cc: [hidden email]
>>>>>> 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:[hidden email]]
>>>>>>>> Sent: Montag, 31. Juli 2017 22:19
>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>>> Cc: [hidden email]
>>>>>>>> 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:[hidden email]]
>>>>>>>>>> Sent: Freitag, 28. Juli 2017 23:53
>>>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>>>>> Cc: [hidden email]
>>>>>>>>>> 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Mikhailo Seledtsov
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:[hidden email]]
>>>> Sent: Freitag, 4. August 2017 21:35
>>>> To: Ioi Lam<[hidden email]>; Lindenmaier, Goetz
>>>> <[hidden email]>; Igor Ignatyev<[hidden email]>
>>>> Cc: [hidden email]
>>>> 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:[hidden email]]
>>>>>>> Sent: Tuesday, August 01, 2017 11:49 PM
>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>> Cc: [hidden email]
>>>>>>> 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:[hidden email]]
>>>>>>>>> Sent: Montag, 31. Juli 2017 22:19
>>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>>>> Cc: [hidden email]
>>>>>>>>> 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:[hidden email]]
>>>>>>>>>>> Sent: Freitag, 28. Juli 2017 23:53
>>>>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>>>>>> Cc: [hidden email]
>>>>>>>>>>> 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.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Lindenmaier, Goetz
Hi Mikhailo,

yes, I please need a sponsor.
Thanks for the help with working on this change!
I added Ioi as reviewer in the webrev, so the patches
can be pushed as is.

Thanks,
  Goetz.

> -----Original Message-----
> From: Mikhailo Seledtsov [mailto:[hidden email]]
> Sent: Dienstag, 8. August 2017 00:01
> To: Ioi Lam <[hidden email]>
> Cc: Lindenmaier, Goetz <[hidden email]>; Igor Ignatyev
> <[hidden email]>; [hidden email]
> Subject: Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
>
> 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:[hidden email]]
> >>>> Sent: Freitag, 4. August 2017 21:35
> >>>> To: Ioi Lam<[hidden email]>; Lindenmaier, Goetz
> >>>> <[hidden email]>; Igor Ignatyev<[hidden email]>
> >>>> Cc: [hidden email]
> >>>> 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:[hidden email]]
> >>>>>>> Sent: Tuesday, August 01, 2017 11:49 PM
> >>>>>>> To: Lindenmaier, Goetz<[hidden email]>
> >>>>>>> Cc: [hidden email]
> >>>>>>> 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:[hidden email]]
> >>>>>>>>> Sent: Montag, 31. Juli 2017 22:19
> >>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
> >>>>>>>>> Cc: [hidden email]
> >>>>>>>>> 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:[hidden email]]
> >>>>>>>>>>> Sent: Freitag, 28. Juli 2017 23:53
> >>>>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
> >>>>>>>>>>> Cc: [hidden email]
> >>>>>>>>>>> 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.
> >
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Mikhailo Seledtsov
Hi Goetz,

   I will test your patch and then will do a sponsor push.


Thank you,

Mikhailo


On 08/08/2017 02:00 AM, Lindenmaier, Goetz wrote:

> Hi Mikhailo,
>
> yes, I please need a sponsor.
> Thanks for the help with working on this change!
> I added Ioi as reviewer in the webrev, so the patches
> can be pushed as is.
>
> Thanks,
>    Goetz.
>
>> -----Original Message-----
>> From: Mikhailo Seledtsov [mailto:[hidden email]]
>> Sent: Dienstag, 8. August 2017 00:01
>> To: Ioi Lam <[hidden email]>
>> Cc: Lindenmaier, Goetz <[hidden email]>; Igor Ignatyev
>> <[hidden email]>; [hidden email]
>> Subject: Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
>>
>> 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:[hidden email]]
>>>>>> Sent: Freitag, 4. August 2017 21:35
>>>>>> To: Ioi Lam<[hidden email]>; Lindenmaier, Goetz
>>>>>> <[hidden email]>; Igor Ignatyev<[hidden email]>
>>>>>> Cc: [hidden email]
>>>>>> 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:[hidden email]]
>>>>>>>>> Sent: Tuesday, August 01, 2017 11:49 PM
>>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>>>> Cc: [hidden email]
>>>>>>>>> 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:[hidden email]]
>>>>>>>>>>> Sent: Montag, 31. Juli 2017 22:19
>>>>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>>>>>> Cc: [hidden email]
>>>>>>>>>>> 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:[hidden email]]
>>>>>>>>>>>>> Sent: Freitag, 28. Juli 2017 23:53
>>>>>>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>>>>>>>> Cc: [hidden email]
>>>>>>>>>>>>> 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests

Lindenmaier, Goetz
Hi mikhailo,

Thanks for sponsoring!

Best regards, götz


> Am 08.08.2017 um 20:05 schrieb mikhailo <[hidden email]>:
>
> Hi Goetz,
>
>  I will test your patch and then will do a sponsor push.
>
>
> Thank you,
>
> Mikhailo
>
>
>> On 08/08/2017 02:00 AM, Lindenmaier, Goetz wrote:
>> Hi Mikhailo,
>>
>> yes, I please need a sponsor.
>> Thanks for the help with working on this change!
>> I added Ioi as reviewer in the webrev, so the patches
>> can be pushed as is.
>>
>> Thanks,
>>   Goetz.
>>
>>> -----Original Message-----
>>> From: Mikhailo Seledtsov [mailto:[hidden email]]
>>> Sent: Dienstag, 8. August 2017 00:01
>>> To: Ioi Lam <[hidden email]>
>>> Cc: Lindenmaier, Goetz <[hidden email]>; Igor Ignatyev
>>> <[hidden email]>; [hidden email]
>>> Subject: Re: RFR(M): 8185436: jtreg: introduce keyword to disable cds tests
>>>
>>> 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:[hidden email]]
>>>>>>> Sent: Freitag, 4. August 2017 21:35
>>>>>>> To: Ioi Lam<[hidden email]>; Lindenmaier, Goetz
>>>>>>> <[hidden email]>; Igor Ignatyev<[hidden email]>
>>>>>>> Cc: [hidden email]
>>>>>>> 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:[hidden email]]
>>>>>>>>>> Sent: Tuesday, August 01, 2017 11:49 PM
>>>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>>>>> Cc: [hidden email]
>>>>>>>>>> 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:[hidden email]]
>>>>>>>>>>>> Sent: Montag, 31. Juli 2017 22:19
>>>>>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>>>>>>> Cc: [hidden email]
>>>>>>>>>>>> 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:[hidden email]]
>>>>>>>>>>>>>> Sent: Freitag, 28. Juli 2017 23:53
>>>>>>>>>>>>>> To: Lindenmaier, Goetz<[hidden email]>
>>>>>>>>>>>>>> Cc: [hidden email]
>>>>>>>>>>>>>> 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.
>
Loading...