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

[Vladimir Kozlov]

Harold,

Could you also not put "else" on different line in arguments.cpp?:

1441       }
1442       else if (FLAG_IS_DEFAULT(ClassMetaspaceSize)) {

It is not Hotspot coding style. We use " } else {".

Thanks,
Vladimir

On 1/17/13 8:08 AM, harold seigel wrote:
> Hi Vladimir,
>
> Thanks again for your comments!
>
> I incorporated them, fixed the copyright date, and posted an updated
> webrev: http://cr.openjdk.java.net/~hseigel/bug_8000968_3/
> <http://cr.openjdk.java.net/%7Ehseigel/bug_8000968_3/>
>
> Now, I just need another reviewer.
>
> Thanks, Harold
>
> On 1/16/2013 2:54 PM, Vladimir Kozlov wrote:
>> On 1/16/13 11:16 AM, harold seigel wrote:
>>> Hi Vladimir,
>>>
>>> Thanks for all your useful comments.  I incorporated them in a new
>>> webrev at
>>>
>>> http://cr.openjdk.java.net/~hseigel/bug_8000968_2/
>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8000968_2/>
>>
>> Could you move (mode != HeapBasedNarrowOop) back to line 696 as it was
>> before (line 698 is big and you have to scroll to see && at its end) ?:
>>
>>  696     } else if ((total_size <= OopEncodingHeapMax) &&
>>  697         (!UseCompressedKlassPointers ||
>>  698           (((OopEncodingHeapMax - heap_size) +
>> Universe::class_metaspace_size()) <= KlassEncodingMetaspaceMax)) &&
>>  699         (mode != HeapBasedNarrowOop)) {
>>
>> Don't add empty line before "} else {", code is small enough.
>>
>> Otherwise looks good.
>>
>>>
>>> Details about the changes in this webrev are inlined below.
>>>
>>> Thanks! Harold
>>>
>>> On 1/11/2013 3:51 PM, Vladimir Kozlov wrote:
>>>> Harold,
>>>>
>>>> First, we are missing check in arguments.cpp that ClassMetaspaceSize <
>>>> KlassPtrEncodingHeapMax. We can switch off compressed class pointers
>>>> even with compressed oops because it worked before Roland pushed his
>>>> fix for compressed class pointers.
>>> I added this check to arguments.cpp.
>>>>
>>>> Second, the right way to fix your problem, I think, is set
>>>> Universe::_narrow_klass._base separately (it requires changing logic
>>>> for loading base into register and other checks that the register has
>>>> base). It is a lot more changes then this but right one.
>>> We discussed this at our weekly runtime meeting and agree that this is a
>>> better long term fix for this problem.  But, we would like to fix the
>>> bug in the short term, using a common base, and then enter a new bug
>>> proposing the long term fix using separate bases.  Does this sound
>>> okay?  How large an effort would it be to use separate bases?  Also,
>>> what would be the cost benefit of using separate bases?
>>
>> Yes, this sound good. I think it may take 2-3 months to do that,
>> Roland started to work in that direction before we decided to use
>> short cut (common base) for now. I think we have to do it regardless
>> the cost if we want to have compressed headers with big (TB) java
>> heaps where we can't use compressed oops (we can't increase
>> ObjAlignment indefinitely to not wast heap space). The same if we go
>> to klass table implementation. The cost is we may have to sacrifice an
>> other register to keep this base, but on other hand we can reload
>> value for each decode since, I think, we decode/encode klasses much
>> less frequently then oops. We need performance testing to find best
>> solution.
>>
>> Thanks,
>> Vladimir
>>
>>>>
>>>> You can hardcode KlassPtrEncodingHeapMax value in
>>>> globalDefinitions.hpp since LogKlassAlignmentInBytes = 3 always:
>>>>
>>>>   const int KlassAlignment           = KlassAlignmentInBytes /
>>>> HeapWordSize;
>>>> + const int KlassPtrEncodingHeapMax = (uint64_t(max_juint) + 1) <<
>>>> LogKlassAlignmentInBytes;
>>> I found an existing constant in globalDefinitions.hpp called
>>> KlassEncodingMetaspaceMax, and am using that.
>>>>
>>>> We don't use file static variables, pass aligned_metaspace_size to
>>>> preferred_heap_base() as argument or make it Universe's field.
>>>>
>>>> Instead of aligned_metaspace_size, I would call it
>>>> class_metaspace_size.
>>> I added a field, _class_metaspace_size, to class Universe and added
>>> setter and getter methods for it.
>>>>
>>>> The next check is incorrect:
>>>>
>>>> class_metaspace_size + HeapBaseMinAddress <= KlassPtrEncodingHeapMax
>>>>
>>>> should be (and add parenthesis):
>>>>
>>>> ((OopEncodingHeapMax - heap_size + class_metaspace_size) <=
>>>> KlassPtrEncodingHeapMax)
>>>>
>>>> because base = (OopEncodingHeapMax - heap_size)
>>> Fixed.
>>>>
>>>> Printing (verbose output) KlassPtrEncodingHeapMax is useless since it
>>>> is always the same value. I would print klass metaspace start address
>>>> instead.
>>> I removed the print statement.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 1/11/13 11:13 AM, harold seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Please review the following change to fix bug 8000968.
>>>>>
>>>>> Summary:
>>>>> The cause of this problem is that the compression mode for Oops and
>>>>> KlassPointers compression is determined using OopEncodingHeapMax,
>>>>> which
>>>>> is based on the alignment and shifting of CompressedOops.  When
>>>>> ObjectAlignmentInBytes=32, Oops pointers can be shifted by 5 bits.
>>>>> However, KlassPointers are still 8 byte aligned and can only be
>>>>> shifted
>>>>> by 3 bits.  Hence, a common compression mode that is calculated
>>>>> based on
>>>>> CompressedOop's 5 bit shift does not work for CompressedKlassPointers.
>>>>>
>>>>> This fix adds a new variable, KlassPtrEncodingHeapMax, which is
>>>>> based on
>>>>> the alignment and shifting of CompressedKlassPointers.  It then uses
>>>>> KlassPtrEncodingHeapMax, along with OopEncodingHeapMax, to determine
>>>>> which compression mode to use.  This means that a compression mode is
>>>>> selected only if it works for both Oops and KlassPointers. Previously,
>>>>> only Oops were looked at.
>>>>>
>>>>> This was tested with JPRT, JCK vm and lang tests,  ute
>>>>> vm.quck.testlist,
>>>>> and hand testing.
>>>>>
>>>>> Openwebrev at http://cr.openjdk.java.net/~hseigel/bug_8000968/
>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8000968/>
>>>>>
>>>>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=8000968
>>>>>
>>>>> Thanks!  Harold