RFR: 8282420: JFR: Remove event handlers

[Jaroslav Bachorik]

On Mon, 25 Apr 2022 16:49:42 GMT, Erik Gahlin <egahlin at openjdk.org> wrote:

> Hi,
> 
> Could I have a review of a fix that removes event handler classes for JFR. Bytecode for event instrumentation is now only added to the event class. Benefits are:
> 
> - No class memory leak in the boot class loader.
> - Reduce overhead from class loading during startup, which is important with additional JDK events that are coming (VirtualThreadStart etc.)
> - One less frame to traverse when recording a Java stack trace.
> 
> Future benefits are:
> 
> - Simplify creating instrumentation at build step. See https://bugs.openjdk.java.net/browse/JDK-8279354
> - Simplify implementation of Event Metrics. See https://bugs.openjdk.java.net/browse/JDK-8224749
> 
> When the Security Manager is removed, much of the code being added for security reasons can be deleted.
> 
> There are few JFR hooks when code is being linked. Plan is to also use these for other events later. 
> 
> Testing: tier 1-4, jdk/jdk/jfr
> 
> Thanks
> Erik

src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp line 1451:

> 1449: 
> 1450: // If for a subklass, on initial class load, an implementation exist for any of the final methods declared in Event,
> 1451: // then constaints are considered breached.

Suggestion:

// then constraints are considered breached.

src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp line 1468:

> 1466:   if (invalid_preconditions_for_subklass_on_initial_load(ik)) {
> 1467:     // Remove the tag denoting this as a jdk.jfr.Event subklass. No instrumentation, hence no events can be written.
> 1468:     // The class is allow to load as-is, but it is classified as outside of the jfr system.

Suggestion:

    // The class is allowed to load as-is, but it is classified as outside of the jfr system.

src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp line 1560:

> 1558:     (JvmtiCachedClassFileData*)NEW_C_HEAP_ARRAY_RETURN_NULL(u1, offset_of(JvmtiCachedClassFileData, data) + stream_len, mtInternal);
> 1559:   if (p == NULL) {
> 1560:     log_error(jfr, system)("Allocation using C_HEAP_ARRAY for " SIZE_FORMAT " bytes failed in JfrEventClassTransformer::on_klass_creation",

Is the log text referencing a wrong method?

Suggestion:

    log_error(jfr, system)("Allocation using C_HEAP_ARRAY for " SIZE_FORMAT " bytes failed in JfrEventClassTransformer::cache_class_file_data",

src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp line 190:

> 188:   /* Set callbacks */
> 189:   memset(&callbacks, 0, sizeof(callbacks));
> 190:   // callbacks.ClassFileLoadHook = jfr_on_class_file_load_hook;

Perhaps you want to remove this commented out code

src/hotspot/share/jfr/instrumentation/jfrResolution.hpp line 41:

> 39:   static void on_resolution(const CallInfo & info, TRAPS);
> 40:   static void on_resolution(const Parse * parse, const ciKlass * holder, const ciMethod * target);
> 41:   static void on_resolution(const GraphBuilder * builder, const ciKlass * holder, const ciMethod * target);

Could you add brief comments as when will each callback be called?
Eg. the second form is called from C1 and the third form is called from C2 (assuming based on what I see in the cpp file)?

src/jdk.jfr/share/classes/jdk/jfr/internal/EventClassBuilder.java line 112:

> 110:         methodVisitor.visitMethodInsn(Opcodes.INVOKESPECIAL, className, m.getName(), m.getDescriptor(), false);
> 111:     }
> 112: 

I wonder if it makes sense to extract this method - it is virtually replacing one method call by another.

src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java line 92:

> 90: 
> 91:         public boolean isString() {
> 92:             return ASMToolkit.TYPE_STRING.getDescriptor().equals(fieldDescriptor);

If this is to be called often it might be beneficial to cache the descriptor instead of recreating it each time.

src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java line 139:

> 137:         this.settingInfos = buildSettingInfos(superClass, classNode);
> 138:         this.fieldInfos = buildFieldInfos(superClass, classNode);
> 139:         String n = annotationValue(classNode, ANNOTATION_TYPE_NAME.getDescriptor(), String.class);

As well - the descriptor could be cached to avoid calling substring operations repeatedly.

src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformEventType.java line 109:

> 107:             }
> 108:         }
> 109:         return 3;

Why is the default stack trace offset changed?

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

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