Add JFR NativeLibrary event #21629

AditiS11 · 2025-04-11T18:46:27Z

Add NativeLibrary event to JFR
Capture libraryName, base and top addresses for each library

tajila · 2025-04-11T19:01:37Z

runtime/vm/JFRConstantPoolTypes.hpp

+		 PORT_ACCESS_FROM_VMC(currentThread);;
+		 OMRPORT_ACCESS_FROM_J9PORT(PORTLIB);
+		 this->_nativeLibrariesCount = 0;
+		 uintptr_t result = omrsl_get_libraries(&VM_JFRConstantPoolTypes::tempCallback, this);


why not pass processNativeLibrariesCallback directly

I am getting a mismatch error if I pass processNativeLibrariesCallback directly. I need to make the callback static. But if I make the callback static, I won't be able to access the non-static members of the VM_JFRConstantPoolTypes class without creating an object of the class.

Which "non-static members" do you need? you are passing an instance of VM_JFRConstantPoolTypes as a parameter, so you can access non-statics via the getters.

tajila · 2025-04-11T19:01:50Z

runtime/vm/JFRConstantPoolTypes.hpp

@@ -1246,6 +1268,67 @@ class VM_JFRConstantPoolTypes {
 		gcConfiguration->heapAddressBits = J9JAVAVM_REFERENCE_SIZE(vm) * 8;
 	}

+	uintptr_t processNativeLibrariesCallback(const char *libraryName, void *lowAddress, void *highAddress, void *userData)


this should be a static function

tajila · 2025-04-11T19:04:48Z

runtime/vm/JFRConstantPoolTypes.hpp

@@ -1246,6 +1268,67 @@ class VM_JFRConstantPoolTypes {
 		gcConfiguration->heapAddressBits = J9JAVAVM_REFERENCE_SIZE(vm) * 8;
 	}

+	uintptr_t processNativeLibrariesCallback(const char *libraryName, void *lowAddress, void *highAddress, void *userData)


you should add a _nativeLibraryCMDlineStrSizeTotal and += strlen(libraryName) to track the size of the strings, that way you can get a more accurate requiredBufferSize

tajila · 2025-04-11T19:07:39Z

Please add a new test case to jfr_events.xml just to validate the event is emitted. You dont need to validate the actual values since those will always change.

tajila · 2025-04-11T19:25:36Z

runtime/vm/JFRChunkWriter.cpp

+	_bufferWriter->writeLEB128(entry->ticks);
+
+	/* write library name */
+	_writer->writeStringLiteral(entry->name);


you can free and NULL the string after you write it to the chunk

tajila · 2025-04-14T18:18:36Z

runtime/vm/JFRChunkWriter.cpp

+	/* write event size */
+	_writer->writeEventSize(_bufferWriter, dataStart);
+
+	if (NULL != entry->name)


NULL check not needed as free() doesn the NULL check

tajila · 2025-04-14T18:19:13Z

runtime/vm/JFRConstantPoolTypes.hpp

+		PORT_ACCESS_FROM_JAVAVM(constantPoolTypes->_vm);
+		while (NULL != entry)
+		{
+			if (strcmp(entry->name, libraryName) == 0) {


lvalue should be on the left side of the ==

Tobi means rvalue (i.e. 0).

tajila · 2025-04-14T18:23:05Z

runtime/vm/JFRConstantPoolTypes.hpp

+		if (NULL == strstr(libraryName, ".so")) {
+			return 0;
+		}
+		VM_JFRConstantPoolTypes *constantPoolTypes = static_cast<VM_JFRConstantPoolTypes *>(userData);


should be a reinterpret cast

tajila · 2025-04-14T18:23:21Z

runtime/vm/JFRConstantPoolTypes.hpp

+			}
+			entry = entry->next;
+		}
+		NativeLibraryEntry *newEntry = static_cast<NativeLibraryEntry *>(pool_newElement(nativeLibrariesTable));


should be a reinterpret cast

tajila · 2025-04-14T18:23:40Z

runtime/vm/JFRConstantPoolTypes.hpp

+		constantPoolTypes->_nativeLibraryCMDlineStrSizeTotal += strlen(libraryName);
+		if (previousNativeLibraryEntry != NULL) {
+			previousNativeLibraryEntry->next = newEntry;
+		}


formatting } else {

tajila · 2025-04-14T18:23:52Z

runtime/vm/JFRConstantPoolTypes.hpp

+
+	void loadNativeLibraries(J9VMThread *currentThread)
+	{
+		 PORT_ACCESS_FROM_VMC(currentThread);;


double ;;

tajila · 2025-04-14T18:25:37Z

runtime/vm/JFRConstantPoolTypes.hpp

+		 OMRPORT_ACCESS_FROM_J9PORT(PORTLIB);
+		 this->_nativeLibrariesCount = 0;
+		 uintptr_t result = omrsl_get_libraries(processNativeLibrariesCallback, this);
+		 if (0 != result) {


if result fails you need to set _buildResult to one of the failures.

tajila · 2025-04-14T18:26:16Z

runtime/vm/JFRConstantPoolTypes.hpp

+		NativeLibraryEntry *previousNativeLibraryEntry = constantPoolTypes->_previousNativeLibraryEntry;
+		NativeLibraryEntry *entry = firstNativeLibraryEntry;
+		PORT_ACCESS_FROM_JAVAVM(constantPoolTypes->_vm);
+		while (NULL != entry)


formatting while (cond) {

see https://github.com/eclipse-omr/omr/blob/master/doc/CodingStandard.md for coding guidelines

Suggest using a for loop:

for (; NULL != entry; entry = entry->next) { ... }

tajila · 2025-04-14T18:27:28Z

runtime/vm/JFRConstantPoolTypes.hpp

@@ -1439,6 +1529,14 @@ class VM_JFRConstantPoolTypes {
 			goto done;
 		}

+		_nativeLibrariesTable = pool_new(sizeof(NativeLibraryEntry), 0, sizeof(U_64), 0, J9_GET_CALLSITE(), OMRMEM_CATEGORY_VM, POOL_FOR_PORT(privatePortLibrary));
+		if(NULL == _nativeLibrariesTable ) {


missing space if (cond) {

No space before ).

If you could fix lines 1516, 1522 and 1528 as well, that would be appreciated.

tajila · 2025-04-14T18:28:05Z

runtime/vm/JFRConstantPoolTypes.hpp

+		 this->_nativeLibrariesCount = 0;
+		 uintptr_t result = omrsl_get_libraries(processNativeLibrariesCallback, this);
+		 if (0 != result) {
+			 return ;


return is not needed

keithc-ca

Please adjust the commit message: the summary line should not end with a period and I don't think "event" should be capitalized.

keithc-ca · 2025-04-14T20:35:52Z

runtime/vm/JFRChunkWriter.cpp

+VM_JFRChunkWriter::writeNativeLibraryEvent(void *anElement, void *userData)
+{
+	NativeLibraryEntry *entry = (NativeLibraryEntry *)anElement;
+	VM_JFRChunkWriter *_writer = (VM_JFRChunkWriter *)userData;


Please don't use the field naming convention for locals (remove the _ prefix).

keithc-ca · 2025-04-14T20:38:22Z

runtime/vm/JFRChunkWriter.hpp

@@ -100,6 +100,7 @@ enum MetadataTypeID {
 	StackTraceID = 188,
 	FrameTypeID = 189,
 	StackFrameID = 197,
+	NativeLibraryID = 112,


Please keep this list organized by value.

keithc-ca · 2025-04-14T20:40:11Z

runtime/vm/JFRChunkWriter.hpp

@@ -899,6 +904,8 @@ class VM_JFRChunkWriter {

 		requiredBufferSize += _vm->peakThreadCount * THREAD_DUMP_EVENT_SIZE_PER_THREAD;

+		requiredBufferSize += ((_constantPoolTypes.getNativeLibraryCount() * 200) + _constantPoolTypes.getNativeLibraryCMDlineStrSizeTotal());


Please remove the outermost parentheses and split this long line:

requiredBufferSize += (_constantPoolTypes.getNativeLibraryCount() * 200) + _constantPoolTypes.getNativeLibraryCMDlineStrSizeTotal();

I think 200 should be captured in a named constant to clarify the intent.

runtime/vm/JFRConstantPoolTypes.hpp

test/functional/cmdLineTests/jfr/jfrevents.xml

keithc-ca · 2025-04-14T20:56:53Z

runtime/vm/JFRConstantPoolTypes.hpp

+		NativeLibraryEntry *previousNativeLibraryEntry = constantPoolTypes->_previousNativeLibraryEntry;
+		NativeLibraryEntry *entry = firstNativeLibraryEntry;
+		PORT_ACCESS_FROM_JAVAVM(constantPoolTypes->_vm);
+		while (NULL != entry)


Suggest using a for loop:

for (; NULL != entry; entry = entry->next) { ... }

runtime/vm/JFRConstantPoolTypes.hpp

keithc-ca · 2025-04-14T20:59:21Z

runtime/vm/JFRConstantPoolTypes.hpp

@@ -1439,6 +1529,14 @@ class VM_JFRConstantPoolTypes {
 			goto done;
 		}

+		_nativeLibrariesTable = pool_new(sizeof(NativeLibraryEntry), 0, sizeof(U_64), 0, J9_GET_CALLSITE(), OMRMEM_CATEGORY_VM, POOL_FOR_PORT(privatePortLibrary));
+		if(NULL == _nativeLibrariesTable ) {


No space before ).

keithc-ca

Please don't end the commit headline with a period. That line should be no more than 70 characters long.

keithc-ca · 2025-04-15T19:16:18Z

runtime/vm/JFRChunkWriter.cpp

+{
+	NativeLibraryEntry *entry = (NativeLibraryEntry *)anElement;
+	VM_JFRChunkWriter *writer = (VM_JFRChunkWriter *)userData;
+	VM_BufferWriter *_bufferWriter = writer->_bufferWriter;


No leading underscore please for local names:

VM_BufferWriter *bufferWriter = writer->_bufferWriter;

The other functions in the file followed a similar pattern. Hence I maintained the same pattern.

void VM_JFRChunkWriter::writeThreadStatisticsEvent(void *anElement, void *userData) { ThreadStatisticsEntry *entry = (ThreadStatisticsEntry *)anElement; VM_BufferWriter *_bufferWriter = (VM_BufferWriter *)userData;

Pls do let me know if I should still make the change.

I don't think those other functions should have locals whose names start with an underscore either; but that need not be corrected with this change. I'm only asking that you don't make it worse.

runtime/vm/JFRChunkWriter.cpp

keithc-ca · 2025-04-15T19:18:34Z

runtime/vm/JFRConstantPoolTypes.hpp

@@ -396,6 +407,8 @@ class VM_JFRConstantPoolTypes {
 	ClassloaderEntry *_firstClassloaderEntry;
 	PackageEntry *_previousPackageEntry;
 	PackageEntry *_firstPackageEntry;
+	NativeLibraryEntry *_firstNativeLibraryEntry;
+	NativeLibraryEntry *_previousNativeLibraryEntry;


Perhaps "previous" should be "last"?

All the other events are following the above naming. Pls do let me know if I should still make the change.

I suppose we should follow the established pattern (which I suggest should be fixed, but later).

runtime/vm/JFRConstantPoolTypes.hpp

keithc-ca · 2025-04-15T19:20:02Z

runtime/vm/JFRConstantPoolTypes.hpp

+			return 1;
+		}
+		newEntry->ticks = j9time_nano_time();
+		newEntry->name = strdup(libraryName);


Suggest allocating the copy of the name above before newEntry is allocated to simplify error handling.

Capture libraryName, base and top addresses for each library

AditiS11 force-pushed the nativelibJFRChunk branch from 5619c41 to 1467092 Compare April 11, 2025 18:54

tajila reviewed Apr 11, 2025

View reviewed changes

tajila self-requested a review April 11, 2025 19:06

tajila reviewed Apr 11, 2025

View reviewed changes

AditiS11 force-pushed the nativelibJFRChunk branch 4 times, most recently from 27136b7 to 775f8c8 Compare April 14, 2025 14:56

tajila reviewed Apr 14, 2025

View reviewed changes

keithc-ca self-requested a review April 14, 2025 18:54

keithc-ca requested changes Apr 14, 2025

View reviewed changes

AditiS11 changed the title ~~Add JFR NativeLibrary Event.~~ Add JFR NativeLibrary event. Apr 15, 2025

AditiS11 changed the title ~~Add JFR NativeLibrary event.~~ Add JFR NativeLibrary event Apr 15, 2025

AditiS11 force-pushed the nativelibJFRChunk branch 2 times, most recently from 9baa3e8 to d135dca Compare April 15, 2025 16:37

keithc-ca requested changes Apr 15, 2025

View reviewed changes

AditiS11 force-pushed the nativelibJFRChunk branch from d135dca to d8cf6b9 Compare April 16, 2025 08:59

Add NativeLibrary event to JFR

0b5b4bb

Capture libraryName, base and top addresses for each library

AditiS11 force-pushed the nativelibJFRChunk branch from d8cf6b9 to 0b5b4bb Compare April 16, 2025 11:58

		@@ -899,6 +904,8 @@ class VM_JFRChunkWriter {

		requiredBufferSize += _vm->peakThreadCount * THREAD_DUMP_EVENT_SIZE_PER_THREAD;

		requiredBufferSize += ((_constantPoolTypes.getNativeLibraryCount() * 200) + _constantPoolTypes.getNativeLibraryCMDlineStrSizeTotal());

Add JFR NativeLibrary event #21629

Are you sure you want to change the base?

Add JFR NativeLibrary event #21629

Conversation

AditiS11 commented Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tajila commented Apr 11, 2025

Choose a reason for hiding this comment

tajila Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tajila Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

tajila Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AditiS11 commented Apr 11, 2025 •

edited

Loading

tajila Apr 14, 2025 •

edited

Loading

tajila Apr 14, 2025 •

edited

Loading

tajila Apr 14, 2025 •

edited

Loading