RFR (M): JDK-8038587: [TESTBUG] Create CDS tests to exercise region sizes and classlist

Tue May 20 21:50:47 UTC 2014

Hi Ioi,

  SpaceUtilizationCheck:
   I have just discussed this with Coleen. She recommends the following:
1. Only check the space utilization for RO and RW regions, since they 
account for most of the reserved space
2. She thinks that 50% minimum utilization is reasonable, for the 
bootstrap CDS, for both 32-bit and 64-bit platforms. I agree
3. On 64-bit platforms, better utilization will reduce a chance of 
reservation denial due to ASLR. In other words, the smaller space the 
CDS is asking for to reserve, the smaller chance of collision with an 
already-mapped region.

I am going to update the code accordingly. Please let me know if you 
have any objections to this approach.

Misha

On 5/20/2014 3:19 PM, Mikhailo Seledtsov wrote:
> Hi Ioi,
>
>  Thank you for reviewing the tests. Please see my comments inline, let 
> me know if you disagree.
> I will re-work the code based on these comments, and post the updated 
> webrev.
>
> Misha
>
> On 5/19/2014 4:45 PM, Ioi Lam wrote:
>> Hi Misha,
>>
>> SharedBaseAddress.java:
>>
>>    During dumping, the VM will first try to map at the address
>>    specified by -XX:SharedBaseAddress. However, if this fails (another
>>    mapping already exists there, the VM will simply map a random
>>    address (as selected by the OS).
>>
>>    I am not sure if there's anything you need to check here, but just 
>> FYI.
>>
> The main goal of this test is too make sure that JVM does not crash or 
> throw exception, no additional checks.
>> SpaceUtilizationCheck:
>>
>>    It will probably never happen, but in the rare case you may have
>>    100% utilization. So for robustness, you may want to handle this in
>>
>>        result.add(input.substring(m.start() + 1, m.start() + 3 ));
> Good catch. Thank you. I will add this case to the matcher logic, as 
> well as a single-digit utilization case (e.g. 3.3%)
>>
>>    Also, 75% may be a good value for 32-bit (where available address
>>    space is scarce), but for 64-bit, I think it's OK to have a low
>>    utilization (i.e., a large default size) so that it's easier for the
>>    user to try different class list without having to worry about the
>>    region sizes.
>>
> I agree, thank you for pointing this out. I will add a check to have 
> different thresholds for 32-bit vs 64-bit. I still think that 64-bit 
> should have some reasonable minimum value for utilization.
> How about 30%?
>> Thanks
>>
>> - Ioi
>>
>>
>>
>>
>> On 5/15/14, 4:15 PM, Mikhailo Seledtsov wrote:
>>> Hi David, team,
>>>
>>>  After  more discussions on the usefulness and stability of the 
>>> ClassListExerciser test with the team, we have decided that this 
>>> test is not that useful. Thank you David for your comments again.
>>> I have kept two other tests, and added a new test: 
>>> SharedBaseAddress.java, which was in the plans and is intended to 
>>> exercise various values for the SharedBaseAddress CL flag.
>>>
>>> The updated webrev can be found at: 
>>> http://cr.openjdk.java.net/~mseledtsov/8038587/webrev.01/
>>> The bug name has been changed to: [TESTBUG] Create CDS tests to 
>>> exercise region sizes and base address
>>>
>>> Thank you,
>>> Misha
>>>
>>> On 4/2/2014 7:55 PM, Mikhailo Seledtsov wrote:
>>>> David,
>>>>
>>>>  Thank you. I will rework ClassListExerciser test to take your 
>>>> comments into consideration, and will submit a new webrev.
>>>>
>>>> Misha
>>>>
>>>> On 4/1/2014 9:52 PM, David Holmes wrote:
>>>>> On 2/04/2014 7:06 AM, Mikhailo Seledtsov wrote:
>>>>>> Hi David,
>>>>>>
>>>>>>   Thank you for review and your feedback.
>>>>>>
>>>>>> The intent of this test is sanity check of basic functionality, 
>>>>>> making
>>>>>> sure the shared classes are loaded w/o crashes or errors. Even 
>>>>>> though
>>>>>> creating a shared archive with -Xshare:dump does exercise loading 
>>>>>> of the
>>>>>> classes from the classlist, I believe SQE should verify it, by
>>>>>> explicitly performing this operation. In my experience I have 
>>>>>> found that
>>>>>> basic tests often find interesting bugs.
>>>>>>
>>>>>> I did drop the attempt to instantiate classes, because the amount of
>>>>>> classes in the class list that have default constructors and 
>>>>>> instantiate
>>>>>> successfully is quite small, and not worth the trouble. Many classes
>>>>>> fail instantiation due to the absence of UI, or other valid reasons.
>>>>>
>>>>> Okay. Dropping that seems to alleviate most of my concerns.
>>>>>
>>>>>> What I have found, however, as part of this exercise, is that the
>>>>>> default SE classlist is optimized for the client, not the server.
>>>>>>
>>>>>> As for classes that are part of the classlist, but are really 
>>>>>> missing
>>>>>> from rt.jar: will you consider this to be a bug?
>>>>>
>>>>> No. The default classlist, as you note is defined for a particular 
>>>>> scenario - at the moment "client" apps. But many of those classes 
>>>>> are not present in Compact Profiles. So unless/until we have 
>>>>> customized default classlists for Compact Profiles, missing 
>>>>> classes can be expected. I don't see this as an issue that 
>>>>> warrants such customized classlists.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>>
>>>>>> Thank you,
>>>>>> Misha
>>>>>>
>>>>>>
>>>>>> On 4/1/2014 1:46 AM, David Holmes wrote:
>>>>>>> Hi Misha,
>>>>>>>
>>>>>>> On 28/03/2014 5:34 AM, Mikhailo Seledtsov wrote:
>>>>>>>> Please review these 3 new CDS tests, an ongoing effort in 
>>>>>>>> implementation
>>>>>>>> of the CDS test specification.
>>>>>>>>
>>>>>>>>      JBS: https://bugs.openjdk.java.net/browse/JDK-8038587
>>>>>>>>      Webrev: 
>>>>>>>> http://cr.openjdk.java.net/~mseledtsov/8038587/webrev.00/
>>>>>>>>      Testing:
>>>>>>>>          Local testing on multiple platforms
>>>>>>>>          JPRT to exercise the added tests:
>>>>>>>> 2014-03-27-184953.mseledtsov.cds (PASS)
>>>>>>>>          These tests found 2 bugs, and one potential issue
>>>>>>>
>>>>>>> I don't quite get the point of the ClassListExerciser test. The
>>>>>>> classlist may well contain classes that do not exist, or that 
>>>>>>> can not
>>>>>>> be instantiated in the test context, even if they have a no-arg
>>>>>>> constructor. Simply creating an archive "exercises" the 
>>>>>>> classlist, so
>>>>>>> I'm really not sure what this test is intending to test.
>>>>>>>
>>>>>>> Also this test won't work with SE Embedded as we have a customized
>>>>>>> default classlist for the Embedded stack.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>> Thank you,
>>>>>>>> Misha
>>>>>>
>>>>
>>>
>>
>