WIP: RamClass: Segment allocation enhancements #20831
Conversation
There was a problem hiding this comment.
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.
| if (NULL != udataFreeListBlock) { | ||
| UDATA* sub4gListBlock = udataFreeListBlock->ramClassSub4gUDATABlockFreeList; | ||
| UDATA* freqListBlock = udataFreeListBlock->ramClassFreqUDATABlockFreeList; | ||
| UDATA* inFreqListBlock = udataFreeListBlock->ramClassInFreqUDATABlockFreeList; |
There was a problem hiding this comment.
These pointer-typed variables should have right-aligned *. e.g. UDATA *inFreqListBlock = udataFreeListBlock...
| struct J9RAMClassFreeListBlock* ramClassLargeBlockFreeList; | ||
| struct J9RAMClassFreeListBlock* ramClassSmallBlockFreeList; | ||
| struct J9RAMClassFreeListBlock* ramClassTinyBlockFreeList; | ||
| } J9RAMClassFreeListBlockType; |
There was a problem hiding this comment.
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.
| struct J9RAMClassFreeListBlock* ramClassTinyBlockFreeList; | ||
| } J9RAMClassFreeListBlockType; | ||
|
|
||
| typedef struct RamClassUDATABlockFreelist { |
There was a problem hiding this comment.
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?
| SUB4G = 0, | ||
| FREQUENTLY_ACCESSED, | ||
| INFREQUENTLY_ACCESSED | ||
| }; |
There was a problem hiding this comment.
Let's rename this enum to SegmentKind.
There was a problem hiding this comment.
We also support enum class. Those are scoped types and are generally safer.
| @@ -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; | |||
There was a problem hiding this comment.
Remove space after cast ) (for all casts lines changed in the PR).
| allocateFreeListBlock (request, classLoader, prev, &classLoader->frequentlyAccessedBlock, classLoader->ramClassUDATABlocks.ramClassFreqUDATABlockFreeList); | ||
| } else if (INFREQUENTLY_ACCESSED == request->segmentType) | ||
| { | ||
| allocateFreeListBlock (request, classLoader, prev, &classLoader->inFrequentlyAccessedBlock, classLoader->ramClassUDATABlocks.ramClassInFreqUDATABlockFreeList); |
There was a problem hiding this comment.
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 #20644.
Adding segment categories
Closes: #20644
Signed-off-by: Nick Kamal <[email protected]>