RFR: 8138659: Speed up InstanceKlass subclass discrimination (original) (raw)

Coleen Phillimore coleen.phillimore at oracle.com
Mon Oct 5 19:54:14 UTC 2015

On 10/5/15 3:28 PM, Kim Barrett wrote:

On Oct 5, 2015, at 4:03 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:

On 2015-10-03 20:26, Kim Barrett wrote:

Please review this change to speed up discrimination among the classes rooted at InstanceKlass.

[…] Changed calls to Klass::oopisinstanceXXX to instead call Klass::oopisinstance then cast to InstanceKlass and use the corresponding kindisxxx predicate. Removed the no longer used Klass::oopisinstanceXXX functions. […] CR: https://bugs.openjdk.java.net/browse/JDK-8138659 Webrev: http://cr.openjdk.java.net/~kbarrett/8138659/webrev.00/ Did you consider keeping the "kind" infrastructure within the instanceKlass files instead of letting it leak down into oop.inline.hpp and out to the rest of the runtime? - virtual bool oopisinstanceClassLoader() const { return false; } - virtual bool oopisinstanceMirror() const { return false; } - virtual bool oopisinstanceRef() const { return false; } Could be: bool oopisinstanceClassLoader() const { return oopisinstance() && InstanceKlass::cast(k)->oopisclassloader(); } bool oopisinstanceMirror() const { return oopisinstance() && InstanceKlass::cast(k)->oopismirror(); } bool oopisinstanceRef() const { return oopisinstance() && InstanceKlass::cast(k)->oopisreference(); } Thanks for looking at this. I did think about it. It's a bit fishy for Klass to have such knowlege about InstanceKlass, though I'd have held my nose if the dispatch could have been put entirely in Klass. Coleen expressed a similar dislike of the Klass::oopisinstanceXXX functions when we talked about this change.

Yes, this was my opinion. I didn't think Klass needs to know what sort of InstanceKlasses there are. Coleen

There's also the technical difficulty that this introduces a circular dependency between Klass and InstanceKlass. I think it might work today to have klass.inline.hpp #include instanceKlass.hpp, but that would be quite fragile. Or the definitions of the Klass::oopisinstanceXXX functions could be moved to klass.cpp, giving up on inlining them. So even ignoring semantic issues, there are syntactic problems.

Other than that, this looks good to me.

StefanK

Testing: JPRT Aurora ad-hoc defaults + GC Nightly + Runtime Nightly

More information about the hotspot-dev mailing list