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