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

[harold seigel]

Hi Mikael,

Thank you for your review comments.  I incorporated all of them and they 
will be available in the updated request for review.

Thanks, Harold

On 7/19/2013 10:17 AM, Mikael Gerdin wrote:
> Hi Harold,
>
> On 2013-07-18 19:54, 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/>
>
> First of all I want to say that I like the intention of the change, 
> but I have some comments on the code:
>
> Now that compressed klass pointers use a different base than 
> compressed oops it is extremely confusing to use the typedef 
> "narrowOop" for them.
> I'd like a "narrowKlass" typedef and use that exclusively when 
> handling compressed class pointers.
> We just got rid of the whole untyped everything-is-an-oop mess when we 
> removed the perm gen, let's try to keep it that way.
>
> A lot of the Metaspace narrowKlass initialization code use the 
> Metaspace:: qualifier even though all the code is in the scope of 
> class Metaspace, my opinion is that it's unnecessarily verbose.
>
> Metaspace::initialize_class_space can be made private now that it's 
> called from Metaspace instead of Universe.
>
> You did not update max_heap_for_compressed_oops() in arguments.cpp, 
> currently it still expects ClassMetaspace to be allocated next to the 
> heap.
>
>
>> 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.
>
> So the "future expansion" of class metaspace will come as a separate 
> change? That's fine by me.
>
>>
>> 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.
>
> Slightly unrelated comment about the .ad file changes.
> If we stored the compressed klass pointer for the IC instead of the 
> full Klass* we wouldn't need to decode the narrowKlass at all in the 
> unverified entry points, right?
>
>>
>> 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.
>>
>
> You don't mention the changes in heap.cpp (in class CodeHeap)
> Are they related to the changed initialization order between 
> Metaspace::global_initialize() and codeCache_init()?
>
>
> /Mikael
>
>>
>> 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
>>
>>