Skip to content

8348853: Fold layout helper check for objects implementing non-array interfaces #24245

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

8348853: Fold layout helper check for objects implementing non-array interfaces #24245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2bf3f27
Generalize the not-array proof
marc-chevalier Mar 20, 2025
a77c397
Revert now useless fix
marc-chevalier Mar 21, 2025
daaaf9a
not reinventing the wheel
marc-chevalier Mar 31, 2025
0467722
Merge branch 'master' into feat/Fold-layout-helper-check-for-objects-…
marc-chevalier Apr 8, 2025
b1fb82a
Merge branch 'master' into feat/Fold-layout-helper-check-for-objects-…
marc-chevalier Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/hotspot/share/ci/ciMetadata.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ class ciMetadata: public ciBaseObject {

Metadata* constant_encoding() { return _metadata; }

bool equals(ciMetadata* obj) const { return (this == obj); }
bool equals(const ciMetadata* obj) const { return (this == obj); }

uint hash() { return ident() * 31; } // ???

Expand Down
7 changes: 1 addition & 6 deletions src/hotspot/share/opto/library_call.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4305,12 +4305,7 @@ Node* LibraryCallKit::generate_array_guard_common(Node* kls, RegionNode* region,
if (obj != nullptr && is_array_ctrl != nullptr && is_array_ctrl != top()) {
// Keep track of the fact that 'obj' is an array to prevent
// array specific accesses from floating above the guard.
Node* cast = _gvn.transform(new CastPPNode(is_array_ctrl, *obj, TypeAryPtr::BOTTOM));
// Check for top because in rare cases, the type system can determine that
// the object can't be an array but the layout helper check is not folded.
if (!cast->is_top()) {
*obj = cast;
}
*obj = _gvn.transform(new CastPPNode(is_array_ctrl, *obj, TypeAryPtr::BOTTOM));
}
return ctrl;
}
Expand Down
6 changes: 2 additions & 4 deletions src/hotspot/share/opto/memnode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2211,10 +2211,8 @@ const Type* LoadNode::Value(PhaseGVN* phase) const {
// This will help short-circuit some reflective code.
if (tkls->offset() == in_bytes(Klass::layout_helper_offset()) &&
tkls->isa_instklassptr() && // not directly typed as an array
!tkls->is_instklassptr()->instance_klass()->is_java_lang_Object() // not the supertype of all T[] and specifically not Serializable & Cloneable
) {
// Note: When interfaces are reliable, we can narrow the interface
// test to (klass != Serializable && klass != Cloneable).
!tkls->is_instklassptr()->might_be_an_array() // not the supertype of all T[] (java.lang.Object) or has an interface that is not Serializable or Cloneable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do the same by using TypeKlassPtr::maybe_java_subtype_of(TypeAryKlassPtr::BOTTOM) and define a TypeAryKlassPtr::BOTTOM to be a static field for the array_interfaces?

AFAICT, TypeKlassPtr::maybe_java_subtype_of() already covers that case so it would avoid some logic duplication. Also in the test above, maybe you could simplify the test a little but by removing tkls->isa_instklassptr()?

Copy link
Member Author

@marc-chevalier marc-chevalier Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be

TypeAryKlassPtr::BOTTOM->maybe_java_subtype_of(tkls)

rather than

tkls->maybe_java_subtype_of(TypeAryKlassPtr::BOTTOM)

My reasoning: if TypeAryKlassPtr::BOTTOM is java.lang.Object + Cloneable + Serializable any array is a subtype of that. But so is any class implementing these interfaces. As well as as any Object implementing more interfaces. But for these two last cases, we know they cannot be array, which is what we want to know: are we sure it's not an array, or could it be an array?

But if we check if tkls is a supertype of java.lang.Object + Cloneable + Serializable, then it has to be an Object (the most general class) and it implements a subset of Cloneable and Serializable. In this case, it can be an array. If tkls is not a super-type of java.lang.Object + Cloneable + Serializable, there are 2 cases:

  • either it is an array type directly (so, I think, in a way or another, we need to check for is_instklassptr), and so a fortiori it can be an array type.
  • it's an instance type and then cannot be an array since there is nothing between array types and java.lang.Object + Cloneable + Serializable. I.e. there is no type T that is not an array type, that is a super-type of at least one array type and that is not a super-type of java.lang.Object + Cloneable + Serializable (that is that is not java.lang.Object or that implements at least another interface).

In other words, our question is

\exists T: T is an array type /\ T <= tkls

(where A <= B means A is a subtype of B) which is equivalent to

    tkls >= (java.lang.Object + Cloneable + Serializable)
\/ (tkls <= (java.lang.Object + Cloneable + Serializable) /\ tkls is an array type)

We can spare the call to is_instklassptr by using a virtual method instead or probably other mechanisms, that's an implementation detail. But I think we need to distinguish cases: both int[] and MyClass + Cloneable + Serializable + MyInterface are sub-types of java.lang.Object + Cloneable + Serializable but for one, we can conclude it's definitely an array, and the other, it's definitely not. Without distinguishing cases, the only sound approximation would be to that that everything can be an array (both sub and super types of java.lang.Object + Cloneable + Serializable).

Does that makes sense? Did I get something wrong? is the BOTTOM not what you had in mind?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, what I suggested doesn't work indeed.

) {
assert(Opcode() == Op_LoadI, "must load an int from _layout_helper");
jint min_size = Klass::instance_layout_helper(oopDesc::header_size(), false);
// The key property of this type is that it folds up tests
Expand Down
19 changes: 19 additions & 0 deletions src/hotspot/share/opto/type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3685,6 +3685,12 @@ bool TypeInterfaces::singleton(void) const {
return Type::singleton();
}

bool TypeInterfaces::has_non_array_interface() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using TypeAryPtr::_array_interfaces->contains(_interfaces); instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost!

return !TypeAryPtr::_array_interfaces->contains(this);

Contains is about TypeInterfaces, that is set of interfaces. So I just need to check that this is not a sub-set of array interfaces. That should do it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm confused, isn't this what I proposed? I didn't check the exact syntax, I just wondered if the TypeInterfaces::contains method couldn't be used instead of adding a new method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, totally! It's just a detail difference. But there is another question: whether we still want has_non_array_interface has a wrapper for this call with a more explicit name, or if we simply inline your suggestion on the callsite of has_non_array_interface. I tend toward the first, I like explicit names, and I suspect it might be useful in more than one place, but not a strong opinion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I just replaced the implementation of has_non_array_interface. If one feels even keeping the method is premature factorization, I can easily inline it.

assert(TypeAryPtr::_array_interfaces != nullptr, "How come Type::Initialize_shared wasn't called yet?");

return !TypeAryPtr::_array_interfaces->contains(this);
}

//------------------------------TypeOopPtr-------------------------------------
TypeOopPtr::TypeOopPtr(TYPES t, PTR ptr, ciKlass* k, const TypeInterfaces* interfaces, bool xk, ciObject* o, int offset,
int instance_id, const TypePtr* speculative, int inline_depth)
Expand Down Expand Up @@ -6197,6 +6203,19 @@ template <class T1, class T2> bool TypePtr::is_java_subtype_of_helper_for_instan
return this_one->klass()->is_subtype_of(other->klass()) && this_one->_interfaces->contains(other->_interfaces);
}

bool TypeInstKlassPtr::might_be_an_array() const {
if (!instance_klass()->is_java_lang_Object()) {
// TypeInstKlassPtr can be an array only if it is java.lang.Object: the only supertype of array types.
return false;
}
if (interfaces()->has_non_array_interface()) {
// Arrays only implement Cloneable and Serializable. If we see any other interface, [this] cannot be an array.
return false;
}
// Cannot prove it's not an array.
return true;
}

bool TypeInstKlassPtr::is_java_subtype_of_helper(const TypeKlassPtr* other, bool this_exact, bool other_exact) const {
return TypePtr::is_java_subtype_of_helper_for_instance(this, other, this_exact, other_exact);
}
Expand Down
4 changes: 4 additions & 0 deletions src/hotspot/share/opto/type.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,6 +943,7 @@ class TypeInterfaces : public Type {
const Type* xmeet(const Type* t) const;

bool singleton(void) const;
bool has_non_array_interface() const;
};

//------------------------------TypePtr----------------------------------------
Expand Down Expand Up @@ -1420,6 +1421,7 @@ class TypeInstPtr : public TypeOopPtr {
class TypeAryPtr : public TypeOopPtr {
friend class Type;
friend class TypePtr;
friend class TypeInterfaces;

TypeAryPtr( PTR ptr, ciObject* o, const TypeAry *ary, ciKlass* k, bool xk,
int offset, int instance_id, bool is_autobox_cache,
Expand Down Expand Up @@ -1692,6 +1694,8 @@ class TypeInstKlassPtr : public TypeKlassPtr {
return klass()->as_instance_klass();
}

bool might_be_an_array() const;

bool is_same_java_type_as_helper(const TypeKlassPtr* other) const;
bool is_java_subtype_of_helper(const TypeKlassPtr* other, bool this_exact, bool other_exact) const;
bool maybe_java_subtype_of_helper(const TypeKlassPtr* other, bool this_exact, bool other_exact) const;
Expand Down