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

[harold seigel]

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/>

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?
>
> 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