RFR: 6312651: Compiler should only use verified interface types for optimization [v2]

[Roland Westrelin]

On Fri, 28 Oct 2022 17:01:06 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   build fix
>
> src/hotspot/share/ci/ciInstanceKlass.cpp line 735:
> 
>> 733:   GrowableArray<ciInstanceKlass*>* result = NULL;
>> 734:   GUARDED_VM_ENTRY(
>> 735:     InstanceKlass* ik = get_instanceKlass();
> 
> Does it make sense to cache the result on `ciInstanceKlass` instance?

Maybe but it's not as simple as it seems (for the reason discussed below for vm classes). Some classes are allocated in a special arena because they are shared between compilations. A _transitive_interfaces array would need to be allocated in that same arena (otherwise if lazily allocated during a particular compilation it would be in the thread's resource are and  its content would be wiped out when the compilation is over but still reachable). That's not straightforward with the current implementation as the arena a ciInstanceKlass is allocated in is not available to the ciInstanceKlass so that would require extra changes and I'm not sure the extra complexity is worth it.

> src/hotspot/share/opto/type.cpp line 4840:
> 
>> 4838:       }
>> 4839:       interfaces = this_interfaces.intersection_with(tp_interfaces);
>> 4840:       return TypeInstPtr::make(ptr, ciEnv::current()->Object_klass(), interfaces, false, NULL,offset, instance_id, speculative, depth);
> 
>> NULL,offset
> 
> missing space

Ok.

> src/hotspot/share/opto/type.cpp line 5737:
> 
>> 5735:       // below the centerline when the superclass is exact. We need to
>> 5736:       // do the same here.
>> 5737:       if (klass()->equals(ciEnv::current()->Object_klass()) && this_interfaces.intersection_with(tp_interfaces).eq(this_interfaces) && !klass_is_exact()) {
> 
>> this_interfaces.intersection_with(tp_interfaces).eq(this_interfaces)
> 
> Maybe a case for a helper method `InterfaceSet::contains(InterfaceSet)`?

Indeed, there are several uses of this pattern. I will make that change.

> src/hotspot/share/opto/type.cpp line 5861:
> 
>> 5859:     bool klass_is_exact = ik->is_final();
>> 5860:     if (!klass_is_exact &&
>> 5861:         deps != NULL && UseUniqueSubclasses) {
> 
> Please, put `UseUniqueSubclasses` guard at the top of the method.

Ok.

> src/hotspot/share/opto/type.hpp line 1154:
> 
>> 1152:   // Respects UseUniqueSubclasses.
>> 1153:   // If the klass is final, the resulting type will be exact.
>> 1154:   static const TypeOopPtr* make_from_klass(ciKlass* klass, bool trust_interface = false) {
> 
> I'd suggest to use an enum (`trust_interfaces`/`ignore_interfaces`) instead of a `bool`, so the intention is clear at call sites.

Good suggestion. I will make that change.

-------------

PR: https://git.openjdk.org/jdk/pull/10901