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

[Vladimir Kozlov]

Good.

Thanks,
Vladimir

On 1/17/13 10:30 AM, harold seigel wrote:
> 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