Request for review: 8000968: NPG UseCompressedKlassPointers asserts withObjectAlignmentInBytes for > 32G Compressed Oops

[Vladimir Kozlov]

Looks good.

Please, test different combinations of class_metaspace and heap sizes to 
confirm that it works as new code expects.

Thanks,
Vladimir

On 1/23/13 7:44 AM, harold seigel wrote:
> Hi Vladimir,
> Thanks for the explanation.
>
> I added the HeapBaseMinAddress check condition to the 'if' statement in
> this new webrev: http://cr.openjdk.java.net/~hseigel/bug_8000968_5/
> <http://cr.openjdk.java.net/%7Ehseigel/bug_8000968_5/>
>
> Could you take one more look when you have a chance?
>
> Thanks, Harold
>
> On 1/22/2013 5:56 PM, Vladimir Kozlov wrote:
>> On 1/22/13 2:38 PM, harold seigel wrote:
>>> Hi Vladimir,
>>>
>>> Can you explain why the HeapBaseMinAddress check is needed, if the heap
>>> is going to be based at (KlassEncodingMetaspaceMax -
>>> Universe::class_metaspace_size()) ?
>>>
>>> Is it because the CDS region is based at HeapBaseMinAddress?
>>
>> HeapBaseMinAddress is usually set by default to the address below
>> which you can't reserve memory. For example, on sparc 64bit VM can't
>> reserve below 4Gb address and HeapBaseMinAddress=4g there.
>>
>> We have this check in the first condition:
>>
>> (total_size <= OopEncodingHeapMax) which is (heap_size +
>> HeapBaseMinAddress <= OopEncodingHeapMax)
>>
>> We need similar check if we want the base to be in
>> KlassEncodingMetaspaceMax.
>>
>> Vladimir
>>
>>>
>>> Thanks, Harold
>>>
>>> On 1/22/2013 3:04 PM, Vladimir Kozlov wrote:
>>>> Harold,
>>>>
>>>> New condition is not accurate since it does not take into account
>>>> HeapBaseMinAddress. You need additional condition:
>>>>
>>>> +     } else if (UseCompressedKlassPointers && (mode !=
>>>> HeapBasedNarrowOop) &&
>>>> +         (Universe::class_metaspace_size() + HeapBaseMinAddress <=
>>>> KlassEncodingMetaspaceMax) &&
>>>> +         (KlassEncodingMetaspaceMax + heap_size -
>>>> Universe::class_metaspace_size() <= OopEncodingHeapMax)) {
>>>>
>>>> Vladimir
>>>>
>>>> On 1/22/13 10:22 AM, harold seigel wrote:
>>>>> HI Roland,
>>>>>
>>>>> Thanks again for your comments.  I incorporated them in this new
>>>>> webrev:
>>>>> http://cr.openjdk.java.net/~hseigel/bug_8000968_4/
>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8000968_4/>
>>>>>
>>>>> The only changes from the previous webrev are to module universe.cpp.
>>>>>
>>>>> Could you take another look when you have a chance?
>>>>>
>>>>> Thanks, Harold
>>>>>
>>>>> On 1/18/2013 6:44 AM, Roland Westrelin wrote:
>>>>>> Hi Harold,
>>>>>>
>>>>>>> I updated the webrev at
>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8000968_3/<http://cr.openjdk.java.net/%7Ehseigel/bug_8000968_3/>
>>>>>>>
>>>>>>>
>>>>>>> with this change.
>>>>>>   696     } else if ((total_size<= OopEncodingHeapMax)&&  (mode !=
>>>>>> HeapBasedNarrowOop)&&
>>>>>>   697         (!UseCompressedKlassPointers ||
>>>>>>   698           (((OopEncodingHeapMax - heap_size) +
>>>>>> Universe::class_metaspace_size())<= KlassEncodingMetaspaceMax))) {
>>>>>>
>>>>>> heap_size<  OopEncodingHeapMax - KlassEncodingMetaspaceMax is
>>>>>> possible, right? Then compressed klass pointers are off with this
>>>>>> code. So wouldn't you also want to check for:
>>>>>>
>>>>>> KlassEncodingMetaspaceMax + heap_size -
>>>>>> Universe::class_metaspace_size()<= OopEncodingHeapMax
>>>>>>
>>>>>> ?
>>>>>>
>>>>>> and then use KlassEncodingMetaspaceMax -
>>>>>> Universe::class_metaspace_size() as base.
>>>>>>
>>>>>> Roland.