Skip to content

8329750: Change Universe functions to return more specific Klass* types #18652

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 3 commits into from
Closed

8329750: Change Universe functions to return more specific Klass* types #18652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc2a4a9
8329750: Change Universe functions to return more specific Klass* types
stefank Apr 5, 2024
d36f650
Update src/hotspot/share/classfile/systemDictionary.cpp
stefank Apr 8, 2024
36bef54
Revert "Update src/hotspot/share/classfile/systemDictionary.cpp "
stefank Apr 8, 2024
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/classfile/systemDictionary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ Klass* SystemDictionary::resolve_array_class_or_null(Symbol* class_name,
}
} else {
k = Universe::typeArrayKlass(t);
k = TypeArrayKlass::cast(k)->array_klass(ndims, CHECK_NULL);
k = k->array_klass(ndims, CHECK_NULL);
Comment on lines 370 to +371
Copy link
Member

Choose a reason for hiding this comment

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

I assume the cast was an attempt to de-virtualize the array_klass() call, so it is better not to use Klass* here.

Copy link
Member Author

Choose a reason for hiding this comment

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

My experience is that these type of casts doesn't make the compiler devirtualize the calls. I tried it now and verified that both with and without the cast we still get the virtual call. You typically need to tell the compiler what function it should be using. (I played around a lot with this when writing the devirtualization layer for the oop_iterate/OopIterateClosure code.)

I tested writing the code above as TypeArrayKlass::cast(k)->TypeArrayKlass::array_klass(ndims, CHECK_NULL) and that gets rid of the virtual call. However, the compiler still can't inline the code ArrayKlass::array_klass code because it is inside a .cpp file and not an .inline.hpp, so this results in a direct call instead of inlined code.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I guess the compiler needs to be conservative in case TypeArrayKlass has a subclass.

Copy link
Contributor

Choose a reason for hiding this comment

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

this looks puzzling. Why are there two array_klass calls now? Add short comments to explain.

Copy link
Contributor

Choose a reason for hiding this comment

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

I sort of see now but am getting squint lines. It's not important for performance to eliminate a virtual call here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. Having two array_klass calls were not intentional. I accepted Dean's suggestion in the GitHub UI, but that didn't remove the old array_klass. I think I'll revert that change given that it is not important to devirtualize this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh good because I was going to need a lot more coffee to understand why there was a second call. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Why devirtualize elsehwere but not here? Maybe it's not a big deal.

Copy link
Member

Choose a reason for hiding this comment

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

Or to put it another way, what's the advantage of using TypeArrayKlass* tak in similar situations below?

}
return k;
}
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,7 +636,7 @@ C2V_VMENTRY_NULL(jobject, lookupType, (JNIEnv* env, jobject, jstring jname, ARGU
resolved_klass = resolved_klass->array_klass(ndim, CHECK_NULL);
}
} else {
resolved_klass = TypeArrayKlass::cast(Universe::typeArrayKlass(ss.type()))->array_klass(ndim, CHECK_NULL);
resolved_klass = Universe::typeArrayKlass(ss.type())->array_klass(ndim, CHECK_NULL);
}
} else {
resolved_klass = SystemDictionary::find_instance_klass(THREAD, class_name,
Expand Down
35 changes: 14 additions & 21 deletions src/hotspot/share/memory/oopFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,41 +41,41 @@
#include "utilities/utf8.hpp"

typeArrayOop oopFactory::new_boolArray(int length, TRAPS) {
return TypeArrayKlass::cast(Universe::boolArrayKlass())->allocate(length, THREAD);
return Universe::boolArrayKlass()->allocate(length, THREAD);
}

typeArrayOop oopFactory::new_charArray(int length, TRAPS) {
return TypeArrayKlass::cast(Universe::charArrayKlass())->allocate(length, THREAD);
return Universe::charArrayKlass()->allocate(length, THREAD);
}

typeArrayOop oopFactory::new_floatArray(int length, TRAPS) {
return TypeArrayKlass::cast(Universe::floatArrayKlass())->allocate(length, THREAD);
return Universe::floatArrayKlass()->allocate(length, THREAD);
}

typeArrayOop oopFactory::new_doubleArray(int length, TRAPS) {
return TypeArrayKlass::cast(Universe::doubleArrayKlass())->allocate(length, THREAD);
return Universe::doubleArrayKlass()->allocate(length, THREAD);
}

typeArrayOop oopFactory::new_byteArray(int length, TRAPS) {
return TypeArrayKlass::cast(Universe::byteArrayKlass())->allocate(length, THREAD);
return Universe::byteArrayKlass()->allocate(length, THREAD);
}

typeArrayOop oopFactory::new_shortArray(int length, TRAPS) {
return TypeArrayKlass::cast(Universe::shortArrayKlass())->allocate(length, THREAD);
return Universe::shortArrayKlass()->allocate(length, THREAD);
}

typeArrayOop oopFactory::new_intArray(int length, TRAPS) {
return TypeArrayKlass::cast(Universe::intArrayKlass())->allocate(length, THREAD);
return Universe::intArrayKlass()->allocate(length, THREAD);
}

typeArrayOop oopFactory::new_longArray(int length, TRAPS) {
return TypeArrayKlass::cast(Universe::longArrayKlass())->allocate(length, THREAD);
return Universe::longArrayKlass()->allocate(length, THREAD);
}

// create java.lang.Object[]
objArrayOop oopFactory::new_objectArray(int length, TRAPS) {
assert(Universe::objectArrayKlass() != nullptr, "Too early?");
return ObjArrayKlass::cast(Universe::objectArrayKlass())->allocate(length, THREAD);
return Universe::objectArrayKlass()->allocate(length, THREAD);
}

typeArrayOop oopFactory::new_charArray(const char* utf8_str, TRAPS) {
Expand All @@ -88,10 +88,8 @@ typeArrayOop oopFactory::new_charArray(const char* utf8_str, TRAPS) {
}

typeArrayOop oopFactory::new_typeArray(BasicType type, int length, TRAPS) {
Klass* klass = Universe::typeArrayKlass(type);
TypeArrayKlass* typeArrayKlass = TypeArrayKlass::cast(klass);
typeArrayOop result = typeArrayKlass->allocate(length, THREAD);
return result;
TypeArrayKlass* klass = Universe::typeArrayKlass(type);
return klass->allocate(length, THREAD);
}

// Create a Java array that points to Symbol.
Expand All @@ -100,17 +98,12 @@ typeArrayOop oopFactory::new_typeArray(BasicType type, int length, TRAPS) {
// this. They cast Symbol* into this type.
typeArrayOop oopFactory::new_symbolArray(int length, TRAPS) {
BasicType type = LP64_ONLY(T_LONG) NOT_LP64(T_INT);
Klass* klass = Universe::typeArrayKlass(type);
TypeArrayKlass* typeArrayKlass = TypeArrayKlass::cast(klass);
typeArrayOop result = typeArrayKlass->allocate(length, THREAD);
return result;
return new_typeArray(type, length, THREAD);
}

typeArrayOop oopFactory::new_typeArray_nozero(BasicType type, int length, TRAPS) {
Klass* klass = Universe::typeArrayKlass(type);
TypeArrayKlass* typeArrayKlass = TypeArrayKlass::cast(klass);
typeArrayOop result = typeArrayKlass->allocate_common(length, false, THREAD);
return result;
TypeArrayKlass* klass = Universe::typeArrayKlass(type);
return klass->allocate_common(length, false, THREAD);
}


Expand Down
12 changes: 7 additions & 5 deletions src/hotspot/share/memory/universe.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,9 @@ static LatestMethodCache _throw_no_such_method_error_cache; // Unsafe.throwNoSuc
static LatestMethodCache _do_stack_walk_cache; // AbstractStackWalker.doStackWalk()

// Known objects
Klass* Universe::_typeArrayKlasses[T_LONG+1] = { nullptr /*, nullptr...*/ };
Klass* Universe::_objectArrayKlass = nullptr;
Klass* Universe::_fillerArrayKlass = nullptr;
TypeArrayKlass* Universe::_typeArrayKlasses[T_LONG+1] = { nullptr /*, nullptr...*/ };
ObjArrayKlass* Universe::_objectArrayKlass = nullptr;
Klass* Universe::_fillerArrayKlass = nullptr;
OopHandle Universe::_basic_type_mirrors[T_VOID+1];
#if INCLUDE_CDS_JAVA_HEAP
int Universe::_archived_basic_type_mirror_indices[T_VOID+1];
Expand Down Expand Up @@ -472,8 +472,10 @@ void Universe::genesis(TRAPS) {
// ordinary object arrays, _objectArrayKlass will be loaded when
// SystemDictionary::initialize(CHECK); is run. See the extra check
// for Object_klass_loaded in objArrayKlassKlass::allocate_objArray_klass_impl.
_objectArrayKlass = InstanceKlass::
cast(vmClasses::Object_klass())->array_klass(1, CHECK);
{
Klass* oak = vmClasses::Object_klass()->array_klass(CHECK);
_objectArrayKlass = ObjArrayKlass::cast(oak);
}
// OLD
// Add the class to the class hierarchy manually to make sure that
// its vtable is initialized after core bootstrapping is completed.
Expand Down
24 changes: 12 additions & 12 deletions src/hotspot/share/memory/universe.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ class Universe: AllStatic {

private:
// Known classes in the VM
static Klass* _typeArrayKlasses[T_LONG+1];
static Klass* _objectArrayKlass;
static TypeArrayKlass* _typeArrayKlasses[T_LONG+1];
static ObjArrayKlass* _objectArrayKlass;
// Special int-Array that represents filler objects that are used by GC to overwrite
// dead objects. References to them are generally an error.
static Klass* _fillerArrayKlass;
Expand Down Expand Up @@ -174,20 +174,20 @@ class Universe: AllStatic {
static void set_verify_data(uintptr_t mask, uintptr_t bits) PRODUCT_RETURN;

// Known classes in the VM
static Klass* boolArrayKlass() { return typeArrayKlass(T_BOOLEAN); }
static Klass* byteArrayKlass() { return typeArrayKlass(T_BYTE); }
static Klass* charArrayKlass() { return typeArrayKlass(T_CHAR); }
static Klass* intArrayKlass() { return typeArrayKlass(T_INT); }
static Klass* shortArrayKlass() { return typeArrayKlass(T_SHORT); }
static Klass* longArrayKlass() { return typeArrayKlass(T_LONG); }
static Klass* floatArrayKlass() { return typeArrayKlass(T_FLOAT); }
static Klass* doubleArrayKlass() { return typeArrayKlass(T_DOUBLE); }
static TypeArrayKlass* boolArrayKlass() { return typeArrayKlass(T_BOOLEAN); }
static TypeArrayKlass* byteArrayKlass() { return typeArrayKlass(T_BYTE); }
static TypeArrayKlass* charArrayKlass() { return typeArrayKlass(T_CHAR); }
static TypeArrayKlass* intArrayKlass() { return typeArrayKlass(T_INT); }
static TypeArrayKlass* shortArrayKlass() { return typeArrayKlass(T_SHORT); }
static TypeArrayKlass* longArrayKlass() { return typeArrayKlass(T_LONG); }
static TypeArrayKlass* floatArrayKlass() { return typeArrayKlass(T_FLOAT); }
static TypeArrayKlass* doubleArrayKlass() { return typeArrayKlass(T_DOUBLE); }

static Klass* objectArrayKlass() { return _objectArrayKlass; }
static ObjArrayKlass* objectArrayKlass() { return _objectArrayKlass; }

static Klass* fillerArrayKlass() { return _fillerArrayKlass; }

static Klass* typeArrayKlass(BasicType t) {
static TypeArrayKlass* typeArrayKlass(BasicType t) {
assert((uint)t >= T_BOOLEAN, "range check for type: %s", type2name(t));
assert((uint)t < T_LONG+1, "range check for type: %s", type2name(t));
assert(_typeArrayKlasses[t] != nullptr, "domain check");
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/prims/vectorSupport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ Handle VectorSupport::allocate_vector_payload_helper(InstanceKlass* ik, frame* f
int elem_size = type2aelembytes(elem_bt);

// On-heap vector values are represented as primitive arrays.
TypeArrayKlass* tak = TypeArrayKlass::cast(Universe::typeArrayKlass(elem_bt));
TypeArrayKlass* tak = Universe::typeArrayKlass(elem_bt);

typeArrayOop arr = tak->allocate(num_elem, CHECK_NH); // safepoint

Expand Down