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

[harold seigel]

Hi Vladimir,

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.

Thanks, Harold

On 1/17/2013 11:29 AM, Vladimir Kozlov wrote:
> 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