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

[Vladimir Kozlov]

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