Skip to content
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

Jump to bottom

Add JFR monitor enter event. #21179

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add JFR monitor enter event. #21179

wants to merge 1 commit into from
+169 −2

Conversation

adpopescu
Copy link
Contributor

Adding monitor blocked/entered event.

@tajila tajila requested a review from theresa-m February 24, 2025 20:49
theresa-m
theresa-m suggested changes Feb 25, 2025
View reviewed changes
runtime/oti/j9vm.hdf Outdated
@@ -229,6 +229,10 @@ typedef UDATA (* lookupNativeAddressCallback)(struct J9VMThread *currentThread,
<struct>J9VMMonitorContendedEnteredEvent</struct>
<data type="struct J9VMThread*" name="currentThread" description="current thread" />
<data type="omrthread_monitor_t" name="monitor" description="the contended monitor" />
<data type="I_64" name="startTicks" description="current ticks when wait began" />
<data type="UDATA" name="monitorAddress" description="object monitor address" />
<data type="struct J9Class*" name="monitorClass" description="monitor class" />
Copy link
Contributor

Choose a reason for hiding this comment

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

description="object monitor class"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

runtime/vm/JFRChunkWriter.hpp

/* write event type */
_bufferWriter->writeLEB128(MonitorEnterID);

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this extra line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

runtime/oti/j9nonbuilder.h Outdated
UDATA monitorAddress;
} J9JFRMonitorEntered;

#define J9JFRMONITORENTERED_STACKTRACE(jfrEvent) ((UDATA*)(((J9JFRMonitorEntered*)(jfrEvent)) + 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

J9JFRMONITORENTERED_STACKTRACE -> J9JFRMONITORENTER_STACKTRACE

Copy link
Contributor

Choose a reason for hiding this comment

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

The existing name (J9JFRMONITORENTERED_STACKTRACE) is consistent with #21179 (comment).

Please add a space before each * in pointer types:

#define J9JFRMONITORENTERED_STACKTRACE(jfrEvent) ((UDATA *)(((J9JFRMonitorEntered *)(jfrEvent)) + 1))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added spaces.

@adpopescu adpopescu force-pushed the jfr-monitor branch 2 times, most recently from e440cc9 to 2ee0035 Compare February 25, 2025 21:36
@keithc-ca
Copy link
Contributor

Please update the commit message and the description; they should describe what this will do if merged.
That might be "Add support for JFR monitor enter event" (no period, not "Adding...").
See https://github.com/eclipse-omr/omr/blob/master/CONTRIBUTING.md#commit-guidelines.

keithc-ca
keithc-ca requested changes Feb 25, 2025
View reviewed changes
runtime/vm/ObjectMonitor.cpp Outdated
bool frameBuilt = saveBlockingEnterObject(currentThread);
ALWAYS_TRIGGER_J9HOOK_VM_MONITOR_CONTENDED_ENTERED(vm->hookInterface, currentThread, monitor);
ALWAYS_TRIGGER_J9HOOK_VM_MONITOR_CONTENDED_ENTERED(vm->hookInterface, currentThread, monitor, startTicks, (UDATA) monitor, ramClass, ownerThread);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't repeat monitor, removing the duplicate field declaration in j9vm.hdf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the duplicate

runtime/vm/jfr.cpp Outdated
@@ -93,6 +93,9 @@ jfrEventSize(J9JFREvent *jfrEvent)
case J9JFR_EVENT_TYPE_OBJECT_WAIT:
size = sizeof(J9JFRMonitorWaited) + (((J9JFRMonitorWaited*)jfrEvent)->stackTraceSize * sizeof(UDATA));
break;
case J9JFR_EVENT_TYPE_MONITOR_ENTER:
size = sizeof(J9JFRMonitorEntered) + (((J9JFRMonitorEntered*)jfrEvent)->stackTraceSize * sizeof(UDATA));
Copy link
Contributor

Choose a reason for hiding this comment

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

Space before *, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

runtime/vm/jfr.cpp Outdated
static void
jfrVMMonitorEntered(J9HookInterface **hook, UDATA eventNum, void *eventData, void *userData)
{
J9VMMonitorContendedEnteredEvent *event = (J9VMMonitorContendedEnteredEvent *) eventData;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please be consistent with casts; there's no space after ) in other casts in this function - there shouldn't be one here either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

runtime/vm/jfr.cpp Outdated
@@ -867,6 +901,7 @@ tearDownJFR(J9JavaVM *vm)
/* Unregister it anyway even it wasn't registered for initializeJFR(vm, TRUE). */
(*vmHooks)->J9HookUnregister(vmHooks, J9HOOK_VM_INITIALIZED, jfrVMInitialized, NULL);
(*vmHooks)->J9HookUnregister(vmHooks, J9HOOK_VM_MONITOR_WAITED, jfrVMMonitorWaited, NULL);
(*vmHooks)->J9HookUnregister(vmHooks, J9HOOK_VM_MONITOR_CONTENDED_ENTERED, jfrVMMonitorEntered, NULL );
Copy link
Contributor

Choose a reason for hiding this comment

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

No space before ), please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@adpopescu
Add JFR monitor enter event
6496380 
This change adds support for the JFR monitor enter event.

Signed-off-by: Adrian Popescu <[email protected]>
@adpopescu adpopescu changed the title Adding JFR monitor enter event. Add JFR monitor enter event. Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithc-ca keithc-ca keithc-ca requested changes

@theresa-m theresa-m theresa-m approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adpopescu @keithc-ca @theresa-m