[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8170297: runtime/SharedArchiveFile/LargeSharedSpace.java didn't run out of memory
From:       Jiangli Zhou <jiangli.zhou () oracle ! com>
Date:       2016-11-29 5:24:40
Message-ID: E3F5D797-EBD5-471B-AEE8-83B65C9282B4 () oracle ! com
[Download RAW message or body]

Hi David and Ioi,

Thanks for the review!

Thanks,
Jiangli 

> On Nov 28, 2016, at 8:29 PM, David Holmes <david.holmes@oracle.com> wrote:
> 
> Thanks Jiangli! That looks a lot clearer to me.
> 
> David
> 
>> On 29/11/2016 1:00 PM, Jiangli Zhou wrote:
>> Hi David and Ioi,
>> 
>> Here is the updated test:
>> 
>>  http://cr.openjdk.java.net/~jiangli/8170297/webrev.01/
>> 
>> IĄŻve split the test cases and reworked the comments. The original bug
>> (8169870) was found on 32-bit platform, so it is worth to test
>> -XX:SharedMiscCodeSize=1600386047 on 32-bit. Tested on linux-64,
>> linux-32 and aarch64. Also running RBT.
>> 
>> Thanks,
>> Jiangli
>> 
>>> On Nov 28, 2016, at 2:57 PM, Jiangli Zhou <jiangli.zhou@Oracle.COM
>>> <mailto:jiangli.zhou@Oracle.COM>> wrote:
>>> 
>>> Hi David and Ioi,
>>> 
>>> Thank you so much for the comments!
>>> 
>>> For the compressed klass limit, it is determined by "shared space size
>>> + compressed klass space size". I agree, the test case and comments
>>> are not well defined for that. IĄŻll update the test to incorporate
>>> your suggestions.
>>> 
>>> Thanks,
>>> Jiangli
>>> 
>>>> On Nov 28, 2016, at 2:47 PM, Ioi Lam <ioi.lam@oracle.com
>>>> <mailto:ioi.lam@oracle.com>> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On 11/28/16 2:16 PM, David Holmes wrote:
>>>>> Hi Jiangli,
>>>>> 
>>>>>> On 29/11/2016 7:52 AM, Jiangli Zhou wrote:
>>>>>> Please review the following fix for test bug JDK-8170297
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8170297>.
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~jiangli/8170297/webrev.00/
>>>>>> <http://cr.openjdk.java.net/~jiangli/8170297/webrev.00/>
>>>>>> 
>>>>>> On some test platforms, the archive might be dumped successfully
>>>>>> with the specified large size. Added an extra level of checking in
>>>>>> the test. Tested with linux-64, linux-32 and aarch64.
>>>>> 
>>>>> I have a problem with this test. :) The test says "the size is too
>>>>> large, exceeding the limit for compressed klass support" - so how
>>>>> can it pass? Is there a limit or not? If this size is too small to
>>>>> fail should we be using a bigger size? Does it need to be dynamic
>>>>> based on the host machine? Do we need to calculate a size based on
>>>>> available memory?
>>>> 
>>>> Maybe the comments should spell out the exact same size of the
>>>> compress klass
>>>> size limit, in both hex and decimal forms? Otherwise it will be hard
>>>> to know
>>>> what 1066924031 would actually lead to.
>>>> 
>>>> I would also suggest splitting the test into two different portions,
>>>> instead
>>>> of using the loop. It's hard to understand how the 3 output message
>>>> would related
>>>> to the 2 input numbers.I think a test case should be clear about
>>>> exactly what it
>>>> intends to test, and not just be "here's bunch of input parameters and it
>>>> should just work".
>>>> 
>>>> 
>>>> E.g, for the second case (1600386047):
>>>> 
>>>>  "Loading classes to share" should not happen on (64-bit platforms
>>>> && compressed
>>>>  pointers.)
>>>> 
>>>>  In this case (1600386047), since the intention is to test the effect on
>>>>  the compress klass size limit, you should explicitly pass
>>>> -XX:+UseCompressedOops
>>>>  -XX:+UseCompressedClassPointersto the end of your command-line, so that
>>>>  they would override any VM options specified via JTREG.
>>>> 
>>>> Also, LargeSharedSpace.java can be run on 32-bit platforms, but the
>>>> message " larger than compressed klass limit" only happens in LP64 code.
>>>> So I think the second test (1600386047) should be excluded from
>>>> 32-bit platforms
>>>> by using Platform.is32bit().
>>>> 
>>>> #ifdef _LP64
>>>>  if (cds_total + compressed_class_space_size() >
>>>> UnscaledClassSpaceMax) {
>>>>    vm_exit_during_initialization("Unable to dump shared archive.",
>>>>        err_msg("Size of archive (" SIZE_FORMAT ") + compressed class
>>>> space ("
>>>>                SIZE_FORMAT ") == total (" SIZE_FORMAT ") is larger
>>>> than compressed "
>>>>                "klass limit: " UINT64_FORMAT, cds_total,
>>>> compressed_class_space_size(),
>>>>                cds_total + compressed_class_space_size(),
>>>> UnscaledClassSpaceMax));
>>>>  }
>>>> ...
>>>> 
>>>> Thanks
>>>> - Ioi
>>>> 
>>>> 
>>>>> Style nit: you added a few // comments but this test is using /* ...
>>>>> */ comments. The mix looks odd to me especially when comments run
>>>>> into each other.
>>>>> 
>>>>> Thanks,
>>>>> David
>>>>> 
>>>>>> Thanks,
>>>>>> Jiangli
>> 

[prev in list] [next in list] [prev in thread] [next in thread]