WIP: RamClass: Segment allocation enhancements #20831

h3110n3rv3 · 2024-12-13T14:46:22Z

The changes reflect the feature request #20644.

Adding segment categories

Personal builds:
sanity.functional
sanity.perf
sanity.system
sanity.openjdk
special.functional
extended.functional
extended.perf
extended.openjdk
extended.system
Test failures seen in extended.openjdk are known: #12795

Closes: #20644
Signed-off-by: Nick Kamal <[email protected]>

ThanHenderson

Given that this is still a WIP, my initial review focused on more superficial - but still important - things that should be addressed. Once it is changed to a non-WIP, I will focus on semantics and functionality. Feel free to start a discussion on any of my comments if something isn't clear or you disagree.

Additionally, we typically squash all the commits in a PR into one commit before merge. This doesn't mean that we can't have multiple commits before merging, but in the case of this PR where the commits are initial changes, compiling versions, and remove prints, and not logical units of added functionality, I suggest you consider squashing them.

ThanHenderson · 2024-12-13T15:25:23Z

runtime/jcl/common/mgmtmemory.c

+		if (NULL != udataFreeListBlock) {
+			UDATA* sub4gListBlock = udataFreeListBlock->ramClassSub4gUDATABlockFreeList;
+			UDATA* freqListBlock = udataFreeListBlock->ramClassFreqUDATABlockFreeList;	
+			UDATA* inFreqListBlock = udataFreeListBlock->ramClassInFreqUDATABlockFreeList;


These pointer-typed variables should have right-aligned *. e.g. UDATA *inFreqListBlock = udataFreeListBlock...

ThanHenderson · 2024-12-13T15:37:57Z

runtime/oti/j9nonbuilder.h

+	struct J9RAMClassFreeListBlock* ramClassLargeBlockFreeList;
+	struct J9RAMClassFreeListBlock* ramClassSmallBlockFreeList;
+	struct J9RAMClassFreeListBlock* ramClassTinyBlockFreeList;
+} J9RAMClassFreeListBlockType;


I think Type in the struct name is a little ambiguous here. Could we think of a better name? Something like: J9RAMClassFreeLists.

Also, nit: the members should be from Tiny to Large, not the inverse.

ThanHenderson · 2024-12-13T15:46:19Z

runtime/oti/j9nonbuilder.h

+	struct J9RAMClassFreeListBlock* ramClassTinyBlockFreeList;
+} J9RAMClassFreeListBlockType;
+
+typedef struct RamClassUDATABlockFreelist {


The l in list should be capitalized. Also, we should keep consistency in struct naming:
RamClassUDATABlockFreelist should be J9RAMClassUDATABlockFreeList.

Though a higher level discussion point is: why are some types/variables FreeListBlock and others BlockFreeList?

ThanHenderson · 2024-12-13T15:48:31Z

runtime/vm/createramclass.cpp

+	SUB4G = 0,
+	FREQUENTLY_ACCESSED,
+	INFREQUENTLY_ACCESSED
+};


Let's rename this enum to SegmentKind.

We also support enum class. Those are scoped types and are generally safer.

ThanHenderson · 2024-12-13T15:51:15Z

runtime/vm/createramclass.cpp

@@ -3745,19 +3766,19 @@ addBlockToFreeList(J9ClassLoader *classLoader, UDATA address, UDATA size)
 	}
 	if (sizeof(UDATA) == size) {
 		UDATA *block = (UDATA *) address;
-		*block = (UDATA) classLoader->ramClassUDATABlockFreeList;
-		classLoader->ramClassUDATABlockFreeList = block;
+		*block = (UDATA) ramClassUDATABlockFreelist;


Remove space after cast ) (for all casts lines changed in the PR).

ThanHenderson · 2024-12-13T16:02:41Z

runtime/vm/createramclass.cpp

+				allocateFreeListBlock (request, classLoader, prev, &classLoader->frequentlyAccessedBlock, classLoader->ramClassUDATABlocks.ramClassFreqUDATABlockFreeList);
+			} else if (INFREQUENTLY_ACCESSED == request->segmentType)
+			{
+				allocateFreeListBlock (request, classLoader, prev, &classLoader->inFrequentlyAccessedBlock, classLoader->ramClassUDATABlocks.ramClassInFreqUDATABlockFreeList);


The formatting of this if chain is incorrect, here is a link to the coding standards that we follow if you don't already have it.

Particularly, the if and else if blocks should be:

if (...) { ... } else if (...) { ... } else { ... }

with the opening brace on the same line as the statement keyword, and a space between the keyword and the opening (, if any. Of course there are exceptions to this (e.g. splitting long conditions across multiple lines), but in general we follow the example above.

The changes reflect the feature request eclipse-openj9#20644. Adding segment categories Closes: eclipse-openj9#20644 Signed-off-by: Nick Kamal <[email protected]>

ThanHenderson · 2024-12-17T21:36:39Z

runtime/jcl/common/mgmtmemory.c

+			}
+			while (NULL != inFreqListBlock) {
+				used -= sizeof(UDATA);
+				inFreqListBlock = *(UDATA **) inFreqListBlock;


Please remove these spaces. e.g. *(UDATA **) inFreqListBlock; change to *(UDATA **)inFreqListBlock;

ThanHenderson · 2024-12-17T21:38:27Z

runtime/vm/createramclass.cpp

 typedef struct RAMClassAllocationRequest {
 	UDATA prefixSize;
 	UDATA alignment;
 	UDATA alignedSize;
 	UDATA *address;
 	UDATA index;
 	UDATA fragmentSize;
+	SegmentKind segmentType;


We can also change the variable name to match the type name here.

ThanHenderson · 2024-12-17T21:44:27Z

runtime/oti/j9nonbuilder.h

@@ -1641,7 +1641,7 @@ typedef struct J9MethodParametersData {
 	J9MethodParameter parameters;
 } J9MethodParametersData;

-#if defined(__xlC__) || defined(__ibmxl__) || defined(__open_xl__) || defined(__GNUC__) || defined(_MSC_VER) || defined(J9ZOS390) || defined(LINUX) || defined(AIXPPC) || defined(WIN32)
+#if defined(__xlC__) || defined(__ibmxl__) || defined(__GNUC__) || defined(_MSC_VER) || defined(J9ZOS390) || defined(LINUX) || defined(AIXPPC) || defined(WIN32)


You'll need to be careful when rebasing. You've accidentally included reverts to recent patches that shouldn't be reverted. Please make sure that only the code that you are wanting to change is included in the PR.

ThanHenderson · 2024-12-17T21:44:47Z

runtime/oti/j9nonbuilder.h

@@ -4950,9 +4961,6 @@ typedef struct J9InternalVMFunctions {
 	void  ( *printThreadInfo)(struct J9JavaVM *vm, struct J9VMThread *self, char *toFile, BOOLEAN allThreads) ;
 	void  (JNICALL *initializeAttachedThread)(struct J9VMThread *vmContext, const char *name, j9object_t *group, UDATA daemon, struct J9VMThread *initializee) ;
 	void  ( *initializeMethodRunAddressNoHook)(struct J9JavaVM* vm, J9Method *method) ;
-#if defined(J9VM_OPT_SNAPSHOTS)
-	void  ( *initializeMethodRunAddressForSnapshot)(struct J9JavaVM *vm, struct J9Method *method) ;
-#endif /* defined(J9VM_OPT_SNAPSHOTS) */


Same rebasing comment here. This shouldn't be removed.

ThanHenderson · 2024-12-17T21:45:59Z

runtime/vm/createramclass.cpp

-					continue;
-				}
+			if(SUB4G == request->segmentType)
+			{


if(SUB4G == request->segmentType) {

should be:

if (SUB4G == request->segmentType) {

ThanHenderson · 2024-12-17T21:47:03Z

runtime/vm/createramclass.cpp

-					addBlockToFreeList(classLoader, fragmentAddress, allocationRequests[i].fragmentSize);
+					UDATA fragmentAddress = ((UDATA)allocationRequests[i].address) - allocationRequests[i].prefixSize;
+					if(SUB4G == allocationRequests[i].segmentType)
+					{


if(SUB4G == allocationRequests[i].segmentType) {

should be:

if (SUB4G == allocationRequests[i].segmentType) {

ThanHenderson · 2024-12-17T23:02:19Z

runtime/vm/createramclass.cpp

@@ -3828,20 +3849,20 @@ allocateRAMClassFragmentFromFreeList(RAMClassAllocationRequest *request, J9RAMCl
 			Trc_VM_internalAllocateRAMClass_AllocatedFromFreeList(request->index, freeListBlock, freeListBlock->size, request->address, request->prefixSize, request->alignedSize, request->alignment);

 			if (islargeBlocksList) {
-				removeBlockFromLargeFreeList(classLoader, (J9RAMClassFreeListLargeBlock **) freeListBlockPtr, (J9RAMClassFreeListLargeBlock *) freeListBlock);
+				removeBlockFromLargeFreeList(classLoader, (J9RAMClassFreeListLargeBlock **) freeListBlockPtr, (J9RAMClassFreeListLargeBlock *) freeListBlock, blockType);


Remove spaces between casts and variable names; here and throughout the PR.

ThanHenderson · 2024-12-17T23:03:50Z

runtime/vm/createramclass.cpp

@@ -4035,24 +4086,28 @@ internalAllocateRAMClass(J9JavaVM *javaVM, J9ClassLoader *classLoader, RAMClassA

 			/* Add a new block with the remaining space at the start of this block, if any, to an appropriate free list */
 			if (0 != alignmentShift) {
-				addBlockToFreeList(classLoader, (UDATA) allocAddress, alignmentShift);
+				if(SUB4G == request->segmentType)
+				{


Same comment as other ifs

ThanHenderson · 2024-12-17T23:08:01Z

runtime/vm/createramclass.cpp


 	block->nextFreeListBlock = tailBlock;
-	classLoader->ramClassLargeBlockFreeList = (J9RAMClassFreeListBlock *) block;
+	blockType->ramClassLargeBlockFreeList = (J9RAMClassFreeListBlock *) block;


Maybe we should now change blockType to something like freeLists or blockFreeLists.

ThanHenderson added the comp:vm label Dec 13, 2024

ThanHenderson reviewed Dec 13, 2024

View reviewed changes

h3110n3rv3 force-pushed the ramClass-segment-allocation branch 2 times, most recently from 954942c to 2e22055 Compare December 16, 2024 23:59

RamClass: Segment allocation enhancements

343b33b

The changes reflect the feature request eclipse-openj9#20644. Adding segment categories Closes: eclipse-openj9#20644 Signed-off-by: Nick Kamal <[email protected]>

h3110n3rv3 force-pushed the ramClass-segment-allocation branch from 1037394 to 343b33b Compare December 17, 2024 12:54

ThanHenderson reviewed Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: RamClass: Segment allocation enhancements #20831

WIP: RamClass: Segment allocation enhancements #20831

h3110n3rv3 commented Dec 13, 2024 •

edited

Loading

ThanHenderson left a comment

ThanHenderson Dec 13, 2024

ThanHenderson Dec 13, 2024

ThanHenderson Dec 13, 2024

ThanHenderson Dec 13, 2024

ThanHenderson Dec 13, 2024

ThanHenderson Dec 13, 2024

ThanHenderson Dec 13, 2024

ThanHenderson Dec 17, 2024

ThanHenderson Dec 17, 2024

ThanHenderson Dec 17, 2024

ThanHenderson Dec 17, 2024

ThanHenderson Dec 17, 2024

ThanHenderson Dec 17, 2024

ThanHenderson Dec 17, 2024 •

edited

Loading

ThanHenderson Dec 17, 2024

ThanHenderson Dec 17, 2024

WIP: RamClass: Segment allocation enhancements #20831

Are you sure you want to change the base?

WIP: RamClass: Segment allocation enhancements #20831

Conversation

h3110n3rv3 commented Dec 13, 2024 • edited Loading

ThanHenderson 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

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

ThanHenderson Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h3110n3rv3 commented Dec 13, 2024 •

edited

Loading

ThanHenderson Dec 17, 2024 •

edited

Loading