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

[harold seigel]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130724/25896059/attachment-0001.html