RFR (M): JDK-8038587: [TESTBUG] Create CDS tests to exercise region sizes and classlist
Ioi Lam
ioi.lam at oracle.com
Thu May 22 19:59:52 UTC 2014
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