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

[harold seigel]

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