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

[Stefan Karlsson]

On 21 maj 2013, at 16:12, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:

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

Where in the spec does it mention InstanceKlasses?

I still see no reason to keep the setters.

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

Unless we reuse the pd field, which isn't used for the arrays. But that's a hack.

>  It's a bit harder but it would be nice to have smaller mirrors for array klasses vs. instanceKlasses.

The size of the mirrors are already of variable size, because of the static fields, so this would probably be a good idea. Something for the Embedded team to pursue maybe?

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

Of course. I'm fine with your current changes as they are.

StefanK

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