Project proposal: Remove the Permanent Generation

[Coleen Phillimore]

Dave,
Thank you for looking at the PermGen elimination code.   Some comments 
inline:

On 3/21/2012 1:55 AM, David Schlosnagle wrote:
> On Tue, Mar 20, 2012 at 10:40 AM, Jon Masamitsu
> <jon.masamitsu at oracle.com>  wrote:
>> You may have noticed that this project proposal never got
>> any farther then this mail.  Somewhat due to indecision on
>> our part, this work is not going to become its own project
>> but will be integrated through the hsx/hotspot-gc/hotspot
>> repository.  We wanted to provide a preview of the work
>> so prepared this webrev from an recent merge of the perm gen
>> removal work with hotspot-gc.  We're still working on it
>> but thought this intermediate webrev would be of interest.
>>
>> http://cr.openjdk.java.net/~coleenp/metadata2
> Jon,
>
> I am very glad to see progress on the removal of perm gen from HotSpot
> and the move to dynamic allocation of these metadata objects. I will
> be very happy once I can remove the XX:MaxPermSize from all of our
> deployment configurations, without switching everything to JRockit or
> J9 :)

Yes, we'll be happy about this also!
> I took a look through some, but not all of the changes in this webrev,
> and I had a few comments and questions.
>
>
> agent/src/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
>    Minor spacing nit on line 416:
>      int length = ( innerClassList == null)? 0 : (int) innerClassList.length();
>                    ^                       ^
>
>    There are a few places (lines 542-543, 572-574, 587-588, 679-681)
> that could use the getAllFieldsCount() method instead of separate
> calls to getFields() and length(), for example replacing:
>      ShortArray fields = getFields();
>      int length = (int) fields.length();
>    with:
>      int length = getAllFieldsCount();
>
>    On line 589, the variable cp is never referenced, is this needed?
>      ConstantPool cp = getConstants();
>
>    On line 886, the variable fields is never referenced, is this needed?
>      ShortArray fields = getFields();
>
> agent/src/share/classes/sun/jvm/hotspot/tools/jcore/ClassWriter.java
>    On lines 383-384, it could use InstanceKlass.getAllFieldsCount()
>
> agent/src/share/classes/sun/jvm/hotspot/tools/soql/SOQL.java
>    On lines 153-154, it could use InstanceKlass.getAllFieldsCount()
>
> agent/src/share/classes/sun/jvm/hotspot/ui/classbrowser/HTMLGenerator.java
>    On lines 1120-1121 and 1819-1820, it could use
> InstanceKlass.getAllFieldsCount()

We should have said that the serviceability agent work has only just 
begun so these are preliminary preliminary changes.   Tom Rodriguez is 
working on it now and will address these comments in the code (if he 
hasn't rewritten it completely).
> src/cpu/sparc/vm/assembler_sparc.cpp
>    The following comment seems odd, should this be "The array of super
> classes is compressed in permgen."?
>      3185   // The array of super classes is compressed it is in permgen.

The comments in this function (and some of the code) referred to the 
super class array as a compressed oop array.  In the permgen elimination 
work, it is now pointer sized.   Metadata is never compressed.  We'd 
left some comments in and some code under UseCompressedHeaders.   The 
latter will not change the size of these arrays.   I cleaned up the code.
> src/cpu/x86/vm/c1_CodeStubs_x86.cpp
>    Is it worthwhile to try to keep the various architectures
> implementations in sync? For example c1_CodeStubs_sparc.cpp lines
> 268-270 and 283-285 has address start = __ pc(); in an #ifdef ASSERT,
> while the c1_CodeStubs_x86.cpp lines 287 and 300 always includes
> address start = __ pc();

Yes, it is worthwhile.  Putting this line in #ifdef ASSERT is a bit 
messy but I did it to match the sparc version.
> src/cpu/sparc/vm/frame_sparc.cpp
> src/cpu/x86/vm/frame_x86.cpp
>    There seem to be some slight differences in the changes to
> frame::is_interpreted_frame_valid that I wonder if they were
> intentional, or if these should really be more similar/identical,
> maybe something along the lines of:
>      constantPoolCacheOop cp = *interpreter_frame_cache_addr();
>      if (cp == NULL ||
>          !Space::is_aligned(cp) ||
>          !cp->is_metadata()) return false;
>      assert(!Universe::heap()->is_in_reserved(cp),
>        "Should not be in heap");

Interesting.  I think Space::is_aligned() implies that the constant pool 
pointer is in Java heap (which it was when it was permgen).  The 
alignment requirement was double word.   I don't think we need to impose 
that alignment on the constant pool.   In any case, testing for it here 
doesn't seem right.  I'm going to change the test in both to match the 
simpler sparc version (plus removing spurious blank lines).
> src/cpu/sparc/vm/frame_sparc.cpp lines 669-673:
>   669   constantPoolCacheOop cp = *interpreter_frame_cache_addr();
>   670
>   671   if (cp == NULL ||
>   672       !cp->is_metadata()) return false;
>   673

Yes, this version.
> src/cpu/x86/vm/frame_x86.cpp lines 556-561:
>   556   constantPoolCacheOop cp = *interpreter_frame_cache_addr();
>   557
>   558   if (cp == NULL ||
>   559       !Space::is_aligned(cp)) return false;
>   560   assert(!Universe::heap()->is_in_reserved(cp),
>   561     "Should not be in heap");
>
> src/share/vm/classfile/classFileParser.cpp
>    On line 1225, will the old fields Array only be freed when the
> classloader is unloaded? Why not free the old fields Array immediately
> before reassigning the new_fields?
> 1222       // This loses the original array for "fields" but since it is
> 1223       // in the class loader arena, it will be freed.  Question is
> 1224       // if this is avoidable waste.
>

In the class loader arenas (or metaspace), we don't want to delete 
metadata individually, rather it's all deleted in one big chunk when 
it's class loader is unloaded.  Redefine classes breaks this, and causes 
individual classes to be deleted within a metaspace.  But that is the 
only case where we want to do that.  So this code you found is a leak, 
that we should clean up.

Thank you for the comments and questions and for looking at the code.  
It's a lot!   At some point, we'd like to make it easier for people to 
understand the changes.   I think if people took a patch and looked 
through their areas of interest, it might help.

See changes: http://cr.openjdk.java.net/~coleenp/metadata3/

Thanks,
Coleen

> Thanks,
> Dave