RFR: 8265129: Add intrinsic support for JVM.getClassId [v6]

[Denghui Dong]

On Thu, 20 May 2021 19:14:07 GMT, Markus Grönlund <mgronlun at openjdk.org> wrote:

>> Denghui Dong has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fix crash problem
>
> src/hotspot/share/c1/c1_Compiler.cpp line 225:
> 
>> 223:   case vmIntrinsics::_counterTime:
>> 224:   case vmIntrinsics::_getEventWriter:
>> 225:   // TODO: temporarily not implement getClassId in c1
> 
> I think we need not worry about an intrinsic for C1, so please remove.

removed.

> src/hotspot/share/jfr/jni/jfrJniMethodRegistration.cpp line 48:
> 
>> 46:       (char*)"getAllEventClasses", (char*)"()Ljava/util/List;", (void*)jfr_get_all_event_classes,
>> 47:       (char*)"getClassId", (char*)"(Ljava/lang/Class;)J", (void*)jfr_class_id,
>> 48:       (char*)"getClassIdNonIntrinsic", (char*)"(Ljava/lang/Class;)J", (void*)jfr_class_id,
> 
> Please remove the getClassIdNonIntrinsic entry, thanks. In addition, can you also remove the now abandoned entry point on the Java side, jdk.jfr.internal.JVM.getClassIdNonIntrinsic. And the entry point in jfr/jni/jniMethod.hpp | .cpp. Thanks.

removed.

> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdLoadBarrier.hpp line 71:
> 
>> 69: class JfrTraceIdLoadBarrier : AllStatic {
>> 70:   friend class JfrCheckpointManager;
>> 71:   friend class SharedRuntime;
> 
> Don't think we need to involve SharedRuntime.

removed.

> src/hotspot/share/opto/library_call.cpp line 61:
> 
>> 59: 
>> 60: #ifdef JFR_HAVE_INTRINSICS
>> 61: #include "jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.hpp"
> 
> Can you move all entry points you need to expose to "jfr/jfr.hpp" and include that instead? I prefer to avoid exposing a lot of the internal impl. details if possible. You can then move the SharedRuntime::trace_id_load_barrier() routines, into jfr/jfr.cpp and avoid touching the runtime/sharedRuntime.hpp | .cpp. Thanks.

removed.

> src/hotspot/share/opto/library_call.cpp line 2775:
> 
>> 2773:  */
>> 2774: bool LibraryCallKit::inline_native_classID() {
>> 2775:   Node* cls = null_check(argument(0), T_OBJECT);
> 
> We can remove the null check, The mirror passed is null checked at the callsite (EventWriter).

good catch! Fixed.

> src/hotspot/share/opto/library_call.cpp line 2784:
> 
>> 2782:                                                  TypeRawPtr::BOTTOM, TypeKlassPtr::OBJECT_OR_NULL));
>> 2783: 
>> 2784:   Node* signaled_flag_address = makecon(TypeRawPtr::make(JfrTraceIdEpoch::signal_address()));
> 
> Can you move this expression to become dependent if you actually need it? It will not be needed in the majority of cases (the InstanceKlass will already be tagged). Thanks.

fixed.

> src/hotspot/share/opto/runtime.cpp line 1502:
> 
>> 1500: }
>> 1501: 
>> 1502: const TypeFunc *OptoRuntime::trace_id_load_barrier_Type() {
> 
> Perhaps inside #if INCLUDE_JFR

fixed

> src/hotspot/share/opto/runtime.hpp line 307:
> 
>> 305:   static const TypeFunc* register_finalizer_Type();
>> 306: 
>> 307:   static const TypeFunc* trace_id_load_barrier_Type();
> 
> JFR_ONLY(static const TypeFunc* trace_id_load_barrier_Type();)

fixed

> src/hotspot/share/runtime/sharedRuntime.cpp line 1884:
> 
>> 1882: 
>> 1883: #ifdef JFR_HAVE_INTRINSICS
>> 1884: JRT_LEAF(void, SharedRuntime::trace_id_load_barrier(Klass * klass))
> 
> This moves to "jfr/jfr.cpp" instead, thanks.

fixed.

> src/hotspot/share/runtime/sharedRuntime.hpp line 526:
> 
>> 524: 
>> 525: #ifdef JFR_HAVE_INTRINSICS
>> 526:   static void trace_id_load_barrier(Klass* klass);
> 
> Move to "jfr/jfr.hpp", thanks.

fixed.

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

PR: https://git.openjdk.java.net/jdk/pull/3470