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
