Request for review: 8003424 - Enable Class Data Sharing for CompressedOops

[Vladimir Kozlov]

Hi Harold,

 > Usually the narrow_klass_base will be 32G and the
 > LogKlassAlignmentInBytes will be 3 (8 bytes alignment) which means that
 > narrow_klass_base >> 3 will exceed maxint. So, we will need R12, unless
 > we do multiple addq(r, narrow_klass_base_shifted()).  Does that make 
sense?

My bad, I miscalculated. But we have "perfect value" 0x800000000 and I 
am tempted to use may be bts (bit set) to avoid R12 usage (assuming or 
with check that class metaspace size < 32Gb).

 > Is there one prefix byte per instruction, or just for certain 
instructions?

"Not all instructions require a REX prefix in 64-bit mode. A prefix is 
necessary only if an instruction references one of the extended 
registers or uses a 64-bit operand."

I want you also run tests with -XX:ObjectAlignmentInBytes=16 and =32 and 
big java heap. Recently we got several bugs which indicated that we did 
not separate cleanly oops and klass pointers en/decoding.

Thanks,
Vladimir

On 7/24/13 11:35 AM, harold seigel wrote:
> Hi Vladimir,
>
> Thank you for the review!  Please see inline comments.
>
> Thanks, Harold
>
> On 7/19/2013 7:56 PM, Vladimir Kozlov wrote:
>> On 7/18/13 3:03 PM, Vladimir Kozlov wrote:
>>> Nice clean up, Harold
>>>
>>> Could you add the size of class metaspace in output with
>>> PrintCompressedOopsMode (and with verbose). I don't want to remember an
>>> other very long flag name: TraceMetavirtualspaceAllocation.
> Will be done in next webrev.
>>>
>>> Also it would be nice to print these info line (and compressed oops info
>>> line) in hs_err files.
> I filed an enhancement bug for this: 8021264
> <https://jbs.oracle.com/bugs/browse/JDK-8021264>.
>>>
>>> About "effect(KILL cr);   /// ???? is this KILL needed?" in x86_64.ad.
>>> encode_klass_not_null() and decode_klass_not_null() use arithmetic
>>> instructions which modify flags: subq, addq, shrq, xorptr. Also note
>>> that code in .ad file does not check the encoding mode for narrow
>>> klass/oop so it should be conservative and assume that arithmetic
>>> instructions are used.
> OK. Thanks.
>>>
>>> If Universe::narrow_klass_shift() == 0 you have klass base below 4Gb so
>>> the value is 32 bit. You can use it directly in narrow klass
>>> encoding/decoding without destroying R12.
>>
>> Correction. You can do it only if base < 2Gb max positive int (sign
>> bit is extended so you will get wrong result with address > 2gb).
> But the base will normally be 32Gb.  So this case won't happen very often.
>>
>> You definitely should not use R12 in decode_klass_not_null(Register
>> dst, Register src) method where you have free 'dst' register.
> Will be fixed in next webrev.
>>
>> About CompressedKlassPointersBase 0x800000000 (32*G). Actually it
>> should depend on ObjectAlignmentInBytes. For =16 it should be 64Gb.
>> The idea was to avoid using R12 by using shifted base:
>>
>> decode:
>> if (Universe::narrow_klass_base_shifted() != 0) {
>>   if (Universe::set_narrow_klass_shift() == 0) {
>>     shrq(r, LogKlassAlignmentInBytes);
>>   }
>>   addq(r, Universe::narrow_klass_base_shifted());
>>   shlq(r, LogKlassAlignmentInBytes);
>> }
>>
>> Universe::narrow_klass_base_shifted() is set only if
>> (Universe::narrow_klass_base >> LogKlassAlignmentInBytes) <= maxint
>> (max positive int).
> Usually the narrow_klass_base will be 32G and the
> LogKlassAlignmentInBytes will be 3 (8 bytes alignment) which means that
> narrow_klass_base >> 3 will exceed maxint. So, we will need R12, unless
> we do multiple addq(r, narrow_klass_base_shifted()).  Does that make sense?
>>
>> And you have general memory reservation problem. At least on Solaris,
>> as I remember, OS will always try to keep protected 4Kb pages between
>> different requested memory regions. That is why in jdk7 we first
>> reserve one huge space and then cut it on java heap, perm gen and
>> protection page below heap to catch implicit NULL exceptions with
>> compressed oops.
>>
>> So, please, verify that you are getting 'cds_address + cds_total'
>> address or CompressedKlassPointersBase without CDS and with compressed
>> oops and with Xmx20Gb.
> I verifed that we get either cds_address+cds_total as the address, or
> CompressedKlassPointersBase as the address without CDS on Linux, Windows
> 7, Mac OS, and Solaris 5.11.  This is true with default heap sizes and
> -Xmx32G.
>
> On Solaris 5.10, I don't get CompressedKlassPointersBase or the desired
> CDS address for class metaspace regardless of heap size.
>>
>>>
>>> instr_size_for_decode_klass_not_null() is ugly but unfortunately we
>>> can't do other way. I assume you use max size because depending on
>>> registers used in instructions you will or will not get prefix byte
>>> (r8-r15).
> Is there one prefix byte per instruction, or just for certain instructions?
>>>
>>> I will look on changes in more details may be late.
>>
>> What 'vsm' stands for in using_class_vsm()? Don't use abbreviations
>> which are not obvious.
> I changed using_class_vsm() to using_class_space().
>>
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 7/18/13 10:54 AM, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this fix for bug 8003424.
>>>>
>>>> Open webrev at http://cr.openjdk.java.net/~hseigel/bug_8003424/
>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8003424/>
>>>> Open bug links at:
>>>>
>>>> http://bugs.sun.com/view_bug.do?bug_id=8003424
>>>> http://bugs.sun.com/view_bug.do?bug_id=8016729
>>>> http://bugs.sun.com/view_bug.do?bug_id=8011610
>>>>
>>>> JBS Bug links at
>>>>
>>>> https://jbs.oracle.com/bugs/browse/JDK-8003424
>>>> https://jbs.oracle.com/bugs/browse/JDK-8016729
>>>> https://jbs.oracle.com/bugs/browse/JDK-8011610
>>>>
>>>>
>>>> This fix provides support for using compressed klass pointers with CDS.
>>>> It also changes the class metaspace allocation on 64-bit platforms when
>>>> UseCompressedKlassPointers is true.  Instead of allocating the class
>>>> metaspace as part of the Java Heap allocation and locating it at the
>>>> base of that allocation, the metaspace will now be allocated
>>>> separately,
>>>> above the Java heap.  This will enable future expansion of the
>>>> metaspace
>>>> because it won't be backed up against the Java heap.  If CDS is being
>>>> used, then the CDS region will be allocated between the Java heap and
>>>> the metaspace.
>>>>
>>>> The new class metaspace allocation code tries to allocate memory at
>>>> 32G,
>>>> or above the CDS region, if it is present.  Because of this, encoding
>>>> and decoding of compressed klass pointers will always require use of a
>>>> base register.  So, encoding and decoding of compressed klass pointers
>>>> will differ from compressed oops.
>>>>
>>>> There are no class metaspace allocation changes if
>>>> UseCompressedKlassPointers is turned off or if running on a 32-bit
>>>> platform.
>>>>
>>>> The code changes also include some cleanup and will fix bugs 8016729,
>>>> 8011610, and 8003424.
>>>>
>>>>
>>>> Many of the modules in this webrev contain a lot of changes. Modules
>>>> macroAssembler_sparc.cpp and macroAssembler_x86.cpp were changed to
>>>> support the new encoding and decoding of compressed klass pointers.
>>>>
>>>> Module metaspace.cpp was changed significantly to support the new
>>>> allocation for metaspace and the CDS region and to determine compressed
>>>> klass pointer encoding base and shifting values.
>>>>
>>>> Most of the changes to module universe.cpp were to remove code related
>>>> to allocating metaspace and remove code that considered compressed
>>>> klass
>>>> pointers when determining the compressed oops compression mechanism.
>>>>
>>>> Modules klass.inline.hpp and oop.inline.hpp were changed as part of a
>>>> cleanup requested by Coleen.
>>>>
>>>>
>>>> These changes were tested with JCK Lang and VM tests, JTReg tests,
>>>> JPRT,
>>>> GCBasher, refworkload, ute vm.quick.testlist and vm.mlvm.testlist
>>>> tests.  Most of the test were run with -Xshare:on and -Xshare:off (with
>>>> and without CDS), and were run on Solaris Sparc, 32-Bit Linux and
>>>> 64-Bit
>>>> Linux.  Jtreg tests were run on Windows 7 and Mac OS.  JCK tests were
>>>> run on Solaris x86.
>>>>
>>>> The performance impact of these changes is TBD.
>>>>
>>>> Thanks, Harold
>>>>
>>>>
>