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

[Coleen Phillimore]

Thanks Stefan,  I have 2 small comments.

On 05/21/2013 10:38 AM, Stefan Karlsson wrote:
> 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?

The hprof format and jvmtiTagMap spec assumes they are dumped and the 
callbacks are there.
>
> I still see no reason to keep the setters.

Oh, yes, I deleted them.  Thanks.
>
>>> 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?

Yes, or a separate change for runtime.

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

Thanks!   I've made some changes so am retesting.   I also changed the 
SA code, so I have to send out another review.

Coleen

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