RFR: 8138659: Speed up InstanceKlass subclass discrimination

[Coleen Phillimore]

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

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