On 21 maj 2013, at 16:12, Coleen Phillimore <coleen.phillim...@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 >
