RFR: 8170297: runtime/SharedArchiveFile/LargeSharedSpace.java didn't run out of memory

[Ioi Lam]

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
>>