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

Tue May 20 21:56:57 UTC 2014

Misha, sounds good to me.

Thanks
- Ioi

On 5/20/14, 2:50 PM, Mikhailo Seledtsov wrote:
> 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
>>>>>>>
>>>>>
>>>>
>>>
>>
>