RFR: 8282420: JFR: Remove event handlers
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 ------------- Commit messages: - Test cleanup - Test fixes - Fix whitespaces - Use EventWriterFactory - Initial Changes: https://git.openjdk.java.net/jdk/pull/8383/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8383&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282420 Stats: 4016 lines in 79 files changed: 2412 ins; 1010 del; 594 mod Patch: https://git.openjdk.java.net/jdk/pull/8383.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8383/head:pull/8383 PR: https://git.openjdk.java.net/jdk/pull/8383
On Mon, 25 Apr 2022 16:49:42 GMT, Erik Gahlin <egahlin@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
On Thu, 5 May 2022 07:05:07 GMT, Jaroslav Bachorik <jbachorik@openjdk.org> wrote:
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision:
Cleanups
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)?
Renamed the methods to on_runtime_resolution, on_c1_resolution and on_c2_resolution to make it clearer. I hope this is sufficient.
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.
Turns out this method is not needed, so I removed it. I also changed the classes FieldInfo and SettingInfo into record classes and while at it, I also made EventInstrumentation (shallow) immutable so guardEventConfiguration is now passed in the constructor. ------------- PR: https://git.openjdk.java.net/jdk/pull/8383
On Thu, 5 May 2022 09:36:27 GMT, Jaroslav Bachorik <jbachorik@openjdk.org> wrote:
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision:
Cleanups
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?
Previously the stack looked like this: UserEvent::commit() { UserEventHandler::write(startTime, duration ...); EventWriter::puStackTrace(); JVM::getStackTraceId(4); Now it looks like this: UserEvent::commit() { EventWriter::puStackTrace(); JVM::getStackTraceId(3); The branches isUsingConfiguration() and isExceptionEvent() are not affected because they used the EventHandler directly previously. ------------- PR: https://git.openjdk.java.net/jdk/pull/8383
On Thu, 5 May 2022 09:20:09 GMT, Jaroslav Bachorik <jbachorik@openjdk.org> wrote:
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision:
Cleanups
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.
Changed from Type to String in some places to reduce allocation. More could be done with the Method descriptor, but I didn't want to go overboard. If needed, I rather handle it separately. ------------- PR: https://git.openjdk.java.net/jdk/pull/8383
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 as a 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
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Cleanups ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/8383/files - new: https://git.openjdk.java.net/jdk/pull/8383/files/28de553d..3e4d9068 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8383&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8383&range=00-01 Stats: 124 lines in 12 files changed: 11 ins; 50 del; 63 mod Patch: https://git.openjdk.java.net/jdk/pull/8383.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8383/head:pull/8383 PR: https://git.openjdk.java.net/jdk/pull/8383
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 as a 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
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Typos in cleanups ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/8383/files - new: https://git.openjdk.java.net/jdk/pull/8383/files/3e4d9068..81989dd6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8383&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8383&range=01-02 Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8383.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8383/head:pull/8383 PR: https://git.openjdk.java.net/jdk/pull/8383
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 as a 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
Erik Gahlin has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge - Typos in cleanups - Cleanups - Test cleanup - Test fixes - Fix whitespaces - Use EventWriterFactory - Initial ------------- Changes: https://git.openjdk.java.net/jdk/pull/8383/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8383&range=03 Stats: 4076 lines in 82 files changed: 2415 ins; 1050 del; 611 mod Patch: https://git.openjdk.java.net/jdk/pull/8383.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8383/head:pull/8383 PR: https://git.openjdk.java.net/jdk/pull/8383
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 as a 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
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Reviewer feedback ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/8383/files - new: https://git.openjdk.java.net/jdk/pull/8383/files/2bf6ed15..2a9081d1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8383&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8383&range=03-04 Stats: 25 lines in 10 files changed: 7 ins; 0 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/8383.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8383/head:pull/8383 PR: https://git.openjdk.java.net/jdk/pull/8383
On Tue, 10 May 2022 13:56:42 GMT, Erik Gahlin <egahlin@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 as a 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
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision:
Reviewer feedback
Marked as reviewed by mgronlun (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/8383
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 as a 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
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Minor fixes ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/8383/files - new: https://git.openjdk.java.net/jdk/pull/8383/files/2a9081d1..193c9d80 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8383&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8383&range=04-05 Stats: 5 lines in 3 files changed: 0 ins; 2 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8383.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8383/head:pull/8383 PR: https://git.openjdk.java.net/jdk/pull/8383
On Tue, 10 May 2022 14:42:58 GMT, Erik Gahlin <egahlin@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 as a 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
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision:
Minor fixes
src/hotspot/share/jfr/instrumentation/jfrResolution.hpp line 40:
38: public: 39: static void on_runtime_resolution(const CallInfo & info, TRAPS); 40: static void on_c1_resolution(const GraphBuilder * builder, const ciKlass * holder, const ciMethod * target);
Should the declarations of `on_c1_resolution` and `on_c2_resolution` be guarded with `COMPILER2_PRESENT` and `COMPILER1_PRESENT` since the method bodies are? ------------- PR: https://git.openjdk.org/jdk/pull/8383
On Wed, 6 Jul 2022 16:50:42 GMT, Doug Simon <dnsimon@openjdk.org> wrote:
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision:
Minor fixes
src/hotspot/share/jfr/instrumentation/jfrResolution.hpp line 40:
38: public: 39: static void on_runtime_resolution(const CallInfo & info, TRAPS); 40: static void on_c1_resolution(const GraphBuilder * builder, const ciKlass * holder, const ciMethod * target);
Should the declarations of `on_c1_resolution` and `on_c2_resolution` be guarded with `COMPILER2_PRESENT` and `COMPILER1_PRESENT` since the method bodies are?
Yes. It's been fixed. See: https://github.com/openjdk/jdk/pull/8680 ------------- PR: https://git.openjdk.org/jdk/pull/8383
On Wed, 6 Jul 2022 19:49:58 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
src/hotspot/share/jfr/instrumentation/jfrResolution.hpp line 40:
38: public: 39: static void on_runtime_resolution(const CallInfo & info, TRAPS); 40: static void on_c1_resolution(const GraphBuilder * builder, const ciKlass * holder, const ciMethod * target);
Should the declarations of `on_c1_resolution` and `on_c2_resolution` be guarded with `COMPILER2_PRESENT` and `COMPILER1_PRESENT` since the method bodies are?
Yes. It's been fixed. See: https://github.com/openjdk/jdk/pull/8680
Yes, I see the bodies are guarded. I was referring to the declarations here in `jfrResolution.hpp`. Shouldn't it be something like: #ifdef COMPILER1 static void on_c1_resolution(const GraphBuilder * builder, const ciKlass * holder, const ciMethod * target); #endif #ifdef COMPILER2 static void on_c2_resolution(const Parse * parse, const ciKlass * holder, const ciMethod * target); #endif and likewise in `jfr.hpp`: #ifdef COMPILER2 static void on_resolution(const Parse* parse, const ciKlass* holder, const ciMethod* target); #endif #ifdef COMPILER1 static void on_resolution(const GraphBuilder* builder, const ciKlass* holder, const ciMethod* target); #endif ------------- PR: https://git.openjdk.org/jdk/pull/8383
On Wed, 6 Jul 2022 20:25:57 GMT, Doug Simon <dnsimon@openjdk.org> wrote:
Yes. It's been fixed. See: https://github.com/openjdk/jdk/pull/8680
Yes, I see the bodies are guarded. I was referring to the declarations here in `jfrResolution.hpp`. Shouldn't it be something like:
#ifdef COMPILER1 static void on_c1_resolution(const GraphBuilder * builder, const ciKlass * holder, const ciMethod * target); #endif #ifdef COMPILER2 static void on_c2_resolution(const Parse * parse, const ciKlass * holder, const ciMethod * target); #endif
and likewise in `jfr.hpp`:
#ifdef COMPILER2 static void on_resolution(const Parse* parse, const ciKlass* holder, const ciMethod* target); #endif #ifdef COMPILER1 static void on_resolution(const GraphBuilder* builder, const ciKlass* holder, const ciMethod* target); #endif
In general, we would like to reduce the amount of conditionalization because it makes the code harder to read and follow. Compilers should not require a definition until a definition is actually needed. Are you seeing compilation errors? ------------- PR: https://git.openjdk.org/jdk/pull/8383
On Thu, 7 Jul 2022 13:12:16 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:
Yes, I see the bodies are guarded. I was referring to the declarations here in `jfrResolution.hpp`. Shouldn't it be something like:
#ifdef COMPILER1 static void on_c1_resolution(const GraphBuilder * builder, const ciKlass * holder, const ciMethod * target); #endif #ifdef COMPILER2 static void on_c2_resolution(const Parse * parse, const ciKlass * holder, const ciMethod * target); #endif
and likewise in `jfr.hpp`:
#ifdef COMPILER2 static void on_resolution(const Parse* parse, const ciKlass* holder, const ciMethod* target); #endif #ifdef COMPILER1 static void on_resolution(const GraphBuilder* builder, const ciKlass* holder, const ciMethod* target); #endif
In general, we would like to reduce the amount of conditionalization because it makes the code harder to read and follow. Compilers should not require a definition until a definition is actually needed. Are you seeing compilation errors?
No I'm not which surprised me. I agree though, if the compilers don't complain, fewer conditional macros is better. ------------- PR: https://git.openjdk.org/jdk/pull/8383
On Thu, 7 Jul 2022 13:38:50 GMT, Doug Simon <dnsimon@openjdk.org> wrote:
In general, we would like to reduce the amount of conditionalization because it makes the code harder to read and follow. Compilers should not require a definition until a definition is actually needed. Are you seeing compilation errors?
No I'm not which surprised me. I agree though, if the compilers don't complain, fewer conditional macros is better.
It is because the bodies in the call chain have been conditionalized out. There is no linking because there are no uses. ------------- PR: https://git.openjdk.org/jdk/pull/8383
On Tue, 10 May 2022 14:42:58 GMT, Erik Gahlin <egahlin@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 as a 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
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision:
Minor fixes
test/jdk/jdk/jfr/jvm/TestGetEventWriter.java line 114:
112: Event e = newEventObject("RegisteredTrueEvent"); 113: try { 114: e.commit(); // throws
When I modify this test to print the IllegalAccessError, I get: java.lang.IllegalAccessError: class jdk.jfr.jvm.RegisteredTrueEvent (in unnamed module @0x4ed05077) cannot access class jdk.jfr.internal.event.EventWriterFactory (in module jdk.jfr) because module jdk.jfr does not export jdk.jfr.internal.event to unnamed module @0x4ed05077 at jdk.jfr.jvm.RegisteredTrueEvent.commit(RegisteredTrueEvent.java:31) at jdk.jfr.jvm.TestGetEventWriter.testRegisteredTrueEvent(TestGetEventWriter.java:104) I was assuming this test is attempting to instead trigger the IAE thrown when linking the commit call. This is achieved by adding `-vmoptions:--add-exports=jdk.jfr/jdk.jfr.internal.event=ALL-UNNAMED` to the jtreg command line: java.lang.IllegalAccessError: illegal access linking method 'jdk.jfr.internal.event.EventWriterFactory.getEventWriter(long)' at jdk.jfr.jvm.RegisteredTrueEvent.commit(RegisteredTrueEvent.java:31) at jdk.jfr.jvm.TestGetEventWriter.testRegisteredTrueEvent(TestGetEventWriter.java:104) Maybe this should be added to the `@run` directives? ------------- PR: https://git.openjdk.org/jdk/pull/8383
On Mon, 25 Apr 2022 16:49:42 GMT, Erik Gahlin <egahlin@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 as a 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
This pull request has now been integrated. Changeset: 0f377363 Author: Erik Gahlin <egahlin@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/0f3773635d3f20866472b505ac390bac10ed... Stats: 4083 lines in 82 files changed: 2420 ins; 1050 del; 613 mod 8282420: JFR: Remove event handlers Reviewed-by: mgronlun ------------- PR: https://git.openjdk.java.net/jdk/pull/8383
participants (6)
-
Doug Simon
-
Erik Gahlin
-
Erik Gahlin
-
Jaroslav Bachorik
-
Markus Grönlund
-
Markus Grönlund