RFR 8003421: NPG: Move oops out of InstanceKlass into mirror

[Coleen Phillimore]

On 05/21/2013 05:11 AM, Stefan Karlsson wrote:
> Hi Coleen,
>
> Good to see all these oops moving to the mirrors. I think the changes 
> look good. I let someone else review the SA changes

Yes, I'm hoping for someone to review the SA changes.
>
> Some comments below:
>
> On 05/21/2013 12:39 AM, Coleen Phillimore wrote:
>> Summary: Inject protection_domain, signers, init_lock into 
>> java_lang_Class
>>
>> Net footprint change is zero except that these fields are in Java 
>> heap rather than metaspace.
>
> There should be some memory saved since we now use compressed oops for 
> the embedded fields.

That's right.
>
>> This helps a little with InstanceKlass size which is in fixed size 
>> space with UseCompressedKlassPointers. Included serviceability 
>> because there were SA changes to code that I don't know is used.
>>
>> Future work is to remove the signers field and the unused 
>> SetProtectionDomain function.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8003421/
>> bug link at http://bugs.sun.com/view_bug.do?bug_id=8003421
>
> http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/instanceKlass.hpp.patch 
>
>
>    // protection domain
> -  oop protection_domain()                  { return 
> _protection_domain; }
> -  void set_protection_domain(oop pd)       { 
> klass_oop_store(&_protection_domain, pd); }
> +  oop protection_domain() const;
> +  void set_protection_domain(Handle pd);
> ...
>    // signers
> -  objArrayOop signers() const              { return _signers; }
> -  void set_signers(objArrayOop s)          { 
> klass_oop_store((oop*)&_signers, s); }
> +  objArrayOop signers() const;
> +  void set_signers(objArrayOop s);
>
> You don't really need the setters on the InstanceKlass anymore. They 
> are only used in jvm.cpp where they take a couple of unnecessary 
> indirections: mirror -> IK -> mirror->set_protection_domain/set_signers.
>

I left these accessor functions in with a comment that JVMTI spec 
defined these fields in InstanceKlass and we have to simulate that they 
are still there for compatibility.

> http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/arrayKlass.cpp.udiff.html 
>
>
> -  java_lang_Class::create_mirror(k, CHECK);
> +  java_lang_Class::create_mirror(k, Handle(NULL), CHECK);
>
> You use NULL here since typeArrays always return a NULL pd, and 
> objArrays always returns the pd of the bottom klass?
>

Yes.
> http://cr.openjdk.java.net/~coleenp/8003421/src/share/vm/oops/instanceKlass.cpp.patch 
>
>
> -void InstanceKlass::oops_do(OopClosure* cl) {
> -  Klass::oops_do(cl);
> -
> -  cl->do_oop(adr_protection_domain());
> -  cl->do_oop(adr_signers());
> -  cl->do_oop(adr_init_lock());
> -
> -  // Don't walk the arrays since they are walked from the 
> ClassLoaderData objects.
> -}
>
> If we could move ArrayKlass::_component_mirror into the j.l.Class, 
> then _java_mirror would be the only oop in the klasses and we could 
> make Klass::oops_do non-virtual ...
>

That would add a field to all mirrors though.  It's a bit harder but it 
would be nice to have smaller mirrors for array klasses vs. instanceKlasses.


> Another thing. If we could direct-allocate the java mirrors in the old 
> gen, then we wouldn't have to walk all the klasses during the young 
> GCs. This would make the GCs a bit less complicated and we could get 
> rid of these fields in Klass:
>
>   // Remembered sets support for the oops in the klasses.
>   jbyte _modified_oops;             // Card Table Equivalent (YC/CMS 
> support)
>   jbyte _accumulated_modified_oops; // Mod Union Equivalent (CMS support)
>

Yes, we want to move in this direction!  Not with this change though.

Coleen
> thanks,
> StefanK
>
>>
>> Tested with vm.quick.testlist, JPRT, jtreg java/security tests and 
>> jck8 tests.
>>
>> Thanks,
>> Coleen
>