On Fri, 4 Aug 2023 21:11:05 GMT, Dean Long <dl...@openjdk.org> wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Dean's suggested changes.
>
> src/hotspot/share/prims/jni.cpp line 209:
>
>> 207:
>> 208:
>> 209: intptr_t jfieldIDWorkaround::encode_klass_hash(Klass* k, intptr_t
>> offset) {
>
> The caller always passes an int, so why not change the parameter to int?
> Suggestion:
>
> intptr_t jfieldIDWorkaround::encode_klass_hash(Klass* k, int offset) {
Ok. That's a good suggestion.
> src/hotspot/share/prims/jni.cpp line 1906:
>
>> 1904: o = JvmtiExport::jni_SetField_probe(thread, obj, o, k, fieldID,
>> false, SigType, (jvalue *)&field_value); \
>> 1905: } \
>> 1906: if (SigType == JVM_SIGNATURE_BOOLEAN) { value =
>> (Argument)(((jboolean)value) & 1); } \
>
> This is already done in bool_field_put(), so it is redundant here. I think
> it can be safely removed.
It is, you're right. I'm going to rerun the tests because this is an unrelated
change.
> src/hotspot/share/prims/jvm.cpp line 612:
>
>> 610: // as implemented in the classic virtual machine; return 0 if object
>> is null
>> 611: return handle == nullptr ? 0 :
>> 612: checked_cast<jint>(ObjectSynchronizer::FastHashCode (THREAD,
>> JNIHandles::resolve_non_null(handle)));
>
> What about making FastHashCode return jint?
I briefly had a look at that but that's a bit more fan-out from code in
synchronizer.cpp and other callers.
> src/hotspot/share/prims/methodHandles.cpp line 910:
>
>> 908: InstanceKlass* defc =
>> InstanceKlass::cast(java_lang_Class::as_Klass(clazz));
>> 909: DEBUG_ONLY(clazz = nullptr); // safety
>> 910: intptr_t vmindex = java_lang_invoke_MemberName::vmindex(mname());
>
> Why not do the checked_cast<int> here instead of below?
> Ideally, the vmindex field would be changed to int (field offsets,
> itable/vtable indexes are all int), but that's more work.
Why not? It's the same and casts the int where it's needed to be an int. It
would be a bigger change to make this an int. Not that big because it's an
injected field but too much extra for this change.
I had a look at this and it affects platform code non-trivially.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15160#discussion_r1284901764
PR Review Comment: https://git.openjdk.org/jdk/pull/15160#discussion_r1284903411
PR Review Comment: https://git.openjdk.org/jdk/pull/15160#discussion_r1284899851
PR Review Comment: https://git.openjdk.org/jdk/pull/15160#discussion_r1284905530
