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

Mikael Gerdin mikael.gerdin at oracle.com
Fri Jul 19 07:17:45 PDT 2013


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


More information about the hotspot-runtime-dev mailing list