RFR (M): JDK-8038587: [TESTBUG] Create CDS tests to exercise region sizes and classlist
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Thu May 22 22:32:29 UTC 2014
Thank you,
I will update the tests, and re-post a webrev.
Misha
On 5/22/2014 3:59 PM, Ioi Lam wrote:
> Looks good to me. Just minor comments:
>
> SpaceUtilizationCheck.java:
>
> filterRegionsOfInteres -> filterRegionsOfInterest
>
> The function could be simplified with String.split:
> 76 private static ArrayList<String> splitLines(String input) {
> 77 ArrayList<String> result = new ArrayList<String>();
> 78 Matcher matcher = Pattern.compile(".*\r?\n").matcher(input);
> 79 while (matcher.find()) {
> 80 result.add(matcher.group());
> 81 }
> 82 return result;
> 83 }
> See
> http://docs.oracle.com/javase/6/docs/api/java/lang/String.html#split(java.lang.String)
>
> Thanks
>
> - Ioi
>
>
> On 5/22/14, 10:35 AM, Mikhailo Seledtsov wrote:
>> Here is the updated webrev, please review:
>> http://cr.openjdk.java.net/~mseledtsov/8038587/webrev.02/
>>
>> Testing: running these tests via JPRT (jprt-sfbay submit -stree .
>> -rtests '*-*-*-runtime/SharedArchiveFile/' -otests '.*runtime.*'
>> -noqa -excludetargets 'linux_ppc.*' -excludetargets 'linux_arm.*'
>> -email mikhailo.seledtsov at oracle.com)
>>
>> Misha
>>
>> On 5/20/2014 5:56 PM, Ioi Lam wrote:
>>> 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
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list