RFR: 8138659: Speed up InstanceKlass subclass discrimination

[Kim Barrett]

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::oop_is_instanceXXX to instead call
>> Klass::oop_is_instance then cast to InstanceKlass and use the
>> corresponding kind_is_xxx predicate.  Removed the no longer used
>> Klass::oop_is_instanceXXX 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 oop_is_instanceClassLoader() const { return false; }
> - virtual bool oop_is_instanceMirror() const { return false; }
> - virtual bool oop_is_instanceRef() const { return false; }
> 
> 
> Could be:
> 
> bool oop_is_instanceClassLoader() const { return oop_is_instance() && InstanceKlass::cast(k)->oop_is_class_loader(); }
> bool oop_is_instanceMirror() const           { return oop_is_instance() && InstanceKlass::cast(k)->oop_is_mirror(); }
> bool oop_is_instanceRef() const                { return oop_is_instance() && InstanceKlass::cast(k)->oop_is_reference(); }

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::oop_is_instanceXXX functions when we talked about this change.

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::oop_is_instanceXXX 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