RFR (M): JDK-8038587: [TESTBUG] Create CDS tests to exercise region sizes and classlist
Coleen Phillimore
coleen.phillimore at oracle.com
Fri May 23 21:31:38 UTC 2014
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