RFR: 8309203: C2: remove copy-by-value of GrowableArray for InterfaceSet [v2]

[Tobias Hartmann]

On Mon, 23 Oct 2023 14:01:50 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The `InterfaceSet` component of types is immutable (once it is fully
>> constructed). In that way, it's similar to other type components. In
>> this change I propose making `InterfaceSet` a subtype of `Type` (and
>> renaming it `TypeInterfaces`). It's then feasible to `hashcons`
>> `TypeInterfaces` instances the way it's done for other `Type`
>> instance.  A single copy of a `TypeInterfaces` for a particular set of
>> interfaces is live during a compilation (and for the entire length of
>> the compilation) and pointer equality can be used to check two sets of
>> interfaces for equality. I think that change makes interface support
>> fit better with the type system implementation and has the added
>> benefits that there's no copy by value of the GrowableArray that holds
>> the set of interfaces anymore.
>> 
>> A couple of required virtual methods for `Type` are not implemented
>> for `TypeInterfaces`: `xmeet` and `singleton` (their body now calls
>> `ShouldNotReachHere`). They would need a way to tell whether
>> `TypeInterfaces` instances are above the center line or not. I thought
>> about (and tried) having a root class `Type` that would contain the
>> bare minimum for type that can be `hashcons`'ed so this method
>> wouldn't need to be implemented by `TypeInterfaces`. That proved to be
>> quite complicated and I gave up.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   build fix

Very nice cleanup. I just added a few style comments.

src/hotspot/share/opto/type.cpp line 66:

> 64:   { Bad,             T_ARRAY,      "array:",        false, Node::NotAMachineReg, relocInfo::none          },  // Array
> 65: 
> 66:   { Bad,             T_ARRAY,      "interfaces:",  false, Node::NotAMachineReg, relocInfo::none          },  // Interfaces

Indentation is wrong.

src/hotspot/share/opto/type.cpp line 3274:

> 3272: 
> 3273: const TypeInterfaces* TypeInterfaces::make(GrowableArray<ciInstanceKlass*>* interfaces) {
> 3274:   TypeInterfaces* result = interfaces == nullptr ? new TypeInterfaces() : new TypeInterfaces(interfaces);

Suggestion:

  TypeInterfaces* result = (interfaces == nullptr) ? new TypeInterfaces() : new TypeInterfaces(interfaces);

src/hotspot/share/opto/type.cpp line 3336:

> 3334: }
> 3335: 
> 3336: const Type *TypeInterfaces::xdual() const {

Suggestion:

const Type* TypeInterfaces::xdual() const {

src/hotspot/share/opto/type.cpp line 3392:

> 3390:            (j >= other->_list.length() ||
> 3391:             compare(_list.at(i), other->_list.at(j)) < 0)) {
> 3392:       result_list.push((ciInstanceKlass*)_list.at(i));

I think you should use `as_instance_klass()` here and in other code.

src/hotspot/share/opto/type.hpp line 97:

> 95:     Array,                      // Array types
> 96: 
> 97:     Interfaces,

Please add a comment

src/hotspot/share/opto/type.hpp line 877:

> 875: };
> 876: 
> 877: class TypeInterfaces : public Type {

Please add a comment

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16309#pullrequestreview-1696558619
PR Review Comment: https://git.openjdk.org/jdk/pull/16309#discussion_r1371275555
PR Review Comment: https://git.openjdk.org/jdk/pull/16309#discussion_r1371276144
PR Review Comment: https://git.openjdk.org/jdk/pull/16309#discussion_r1371276816
PR Review Comment: https://git.openjdk.org/jdk/pull/16309#discussion_r1371278433
PR Review Comment: https://git.openjdk.org/jdk/pull/16309#discussion_r1371288833
PR Review Comment: https://git.openjdk.org/jdk/pull/16309#discussion_r1371289228