RFR (M): JDK-8038587: [TESTBUG] Create CDS tests to exercise region sizes and classlist
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Tue May 27 23:25:12 UTC 2014
Hi Coleen,
Thank you for the review. I am happy to hear that you like these tests.
I have fixed the indentation issue.
Misha
On 5/23/2014 2:31 PM, Coleen Phillimore wrote:
>
> Misha,
>
> in
> http://cr.openjdk.java.net/~mseledtsov/8038587/webrev.03/test/runtime/SharedArchiveFile/SharedBaseAddress.java.html
>
> your indentation on lines 63-65 is off.
>
> I like these tests. The do the most testing with most efficient use
> of lines of code. And they found a bug!
>
> Coleen
>
> On 5/23/14, 12:08 PM, Mikhailo Seledtsov wrote:
>> 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