RFR (M): JDK-8038587: [TESTBUG] Create CDS tests to exercise region sizes and classlist
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Fri May 23 16:08:36 UTC 2014
Hi,
I have updated the code following Ioi's recommendations, thank you.
Please review at http://cr.openjdk.java.net/~mseledtsov/8038587/webrev.03/
Yesterday's JPRT run with these tests has discovered a new bug, see
JDK-8043896 <https://bugs.openjdk.java.net/browse/JDK-8043896>
I have added the "@ignore JDK-8043896" to the LimitSharedSizes.java
test. I would like to keep this test in the source control, with @ignore
tag, this way it will not be lost; once the associated bugs are fixed,
the @ignore will be removed.
Thank you,
Misha
On 5/22/2014 6:32 PM, Mikhailo Seledtsov wrote:
> 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