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.
| 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.
h3110n3rv3
force-pushed
the
ramClass-segment-allocation
branch
2 times, most recently
from
954942c to
2e22055
Compare
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
|
Personal builds: |
h3110n3rv3
changed the title
tajila
requested review from
theresa-m and
ThanHenderson
and removed request for
ThanHenderson
runtime/vm/createramclass.cpp
Outdated
| @@ -4005,8 +4048,14 @@ internalAllocateRAMClass(J9JavaVM *javaVM, J9ClassLoader *classLoader, RAMClassA | |||
| /* TODO attempt to coalesce free blocks? */ | |||
There was a problem hiding this comment.
line 4020->4103 should be done in a separate function since you need to do it 3 times for each segment kind.
For example
for (request = requests; NULL != request; request = request->next) {
fragmentsLeftToAllocate++;
newSegmentSize += request->fragmentSize + request->alignment;
}
That part needs to be calculated for a specific segment kind.
There was a problem hiding this comment.
/* Add sizeof(UDATA) to hold the "lastAllocatedClass" pointer */
newSegmentSize += sizeof(UDATA);
The following should only be done in the sub4g segment
| struct J9MemorySegment* classSegments; | ||
| struct J9RAMClassFreeListBlock* ramClassLargeBlockFreeList; | ||
| struct J9RAMClassFreeListBlock* ramClassSmallBlockFreeList; | ||
| struct J9RAMClassFreeListBlock* ramClassTinyBlockFreeList; | ||
| UDATA* ramClassUDATABlockFreeList; | ||
| struct J9RAMClassFreeLists sub4gBlock; |
There was a problem hiding this comment.
I think these names would be more descriptive as something like "sub4gFreeLists". I'm not sure what block is referring to here.
| struct J9RAMClassFreeListBlock* ramClassLargeBlockFreeList; | ||
| } J9RAMClassFreeLists; | ||
|
|
||
| typedef struct J9RAMClassUDATABlockFreeList { |
There was a problem hiding this comment.
I would suggest adding a UDATA * field to J9RAMClassFreeLists instead of having a separate structure to hold these values.
runtime/jcl/common/mgmtmemory.c
Outdated
| J9RAMClassFreeLists *sub4gFreeListBlock = &classLoader->sub4gBlock; | ||
| J9RAMClassFreeLists *freqFreeListBlock = &classLoader->frequentlyAccessedBlock; | ||
| J9RAMClassFreeLists *InFreqFreeListBlock = &classLoader->inFrequentlyAccessedBlock; | ||
| if (NULL != udataFreeListBlock) { |
There was a problem hiding this comment.
This check is not necessary since classLoader->ramClassUDATABlocks is not a pointer. This applies to the J9RAMClassFreeLists's as well.
| enum SegmentKind { | ||
| SUB4G = 0, | ||
| FREQUENTLY_ACCESSED, | ||
| INFREQUENTLY_ACCESSED |
There was a problem hiding this comment.
INFREQUENTLY_ACCESSED is never assigned as a segment type. Is that intentional?
There was a problem hiding this comment.
Yes, following this PR the plan is to do some benchmarking to determine which fragments are infrequently accessed.
| if (request->prefixSize != 0) { | ||
| request->address++; | ||
| } | ||
| Trc_VM_internalAllocateRAMClass_AllocatedFromFreeList(request->index, block, sizeof(UDATA), request->address, request->prefixSize, request->alignedSize, request->alignment); |
There was a problem hiding this comment.
nit: this may be more useful if information was provided on which list the block is coming from
…0n3rv3/openj9 into ramClass-segment-allocation
The changes reflect the feature request eclipse-openj9#20644. Adding segment categories Closes: eclipse-openj9#20644 Signed-off-by: Nick Kamal <[email protected]>
Remove "-zos" suffix before checking whether the version is "next". Signed-off-by: Keith W. Campbell <[email protected]>
A test level of "sanity" previously mapped to "sanity.functional", but now maps to "sanity.functional,sanity.openjdk". This change updates build documentation to reflect that. It also adds "openjdk" to the list of recognized test groups, "functional" and "system". Signed-off-by: Henry Zongaro <[email protected]>
If null-restricted array is enabled and the class is an array class, the null-restricted array class and the nullable array class share the same signature. The null-restricted array can be viewed as a sub-type of the nullable array. Therefore, the constraint cannot be fixed class. Related: eclipse-openj9#20522 Signed-off-by: Annabelle Huo <[email protected]>
Since the keepCycles array is zero-initialized, if the test is aborted in the very first cycle (i == 0) the cleanup procedure at the end cannot rely on the fact that keepCycles[j] >= i implies that segment j must be freed. Before these changes, such a scenario would cause the test to segfault during cleanup. Signed-off-by: Christian Despres <[email protected]>
- explicitly initialize prevContextSwitches - calculate switchRate with float Related: eclipse-openj9#20725 Signed-off-by: Gengchen Tuo <[email protected]>
The IProfiler does not profile direct calls. However, the optimizer may create artificial profiler entries when it calls setCallCount(). These artificial entries are marked to be non-persistent and we only set the frequency (totalCount) ignoring the class of the method that is called. This commit also records the class of the method to be called. Doing so does not require additional storage. Signed-off-by: Marius Pirvu <[email protected]>
This change will allow createBalancedBST to be called from places where the Compilation object is not readily available. Signed-off-by: Marius Pirvu <[email protected]>
The goal is to use the code from TR_AggregationHT in more places (for instance for dumping all IProfiler info into the SCC). Signed-off-by: Marius Pirvu <[email protected]>
Signed-off-by: Marius Pirvu <[email protected]>
This commit implements the TR_IProfiler::persistAllEntries() method which can write all IProfiler entries into the SCC on a ROMMethod by ROMMethod basis. Signed-off-by: Marius Pirvu <[email protected]>
With the removal of the security manager (JEP 486) the System.security field is not used for anything meaningful. Signed-off-by: Theresa Mammarella <[email protected]>
These calls are not needed since System.getSecurityManager returns null for JDK 24+. Signed-off-by: Theresa Mammarella <[email protected]>
Signed-off-by: Theresa Mammarella <[email protected]>
h3110n3rv3
force-pushed
the
ramClass-segment-allocation
branch
from
c9294f1 to
61bea9f
Compare
…0n3rv3/openj9 into ramClass-segment-allocation
|
@h3110n3rv3 did you close this intentionally? |
|
yes sorry i messed up the rebasing. I tried to fix it but couldn't so I closed this one and created another one. I am still working on changes based on Tobi's comments. I will update there once I am done. |
Sounds good! If you need some help on this in the future let me know, it can certainly be a mess. |
The changes reflect the feature request #20644.
Adding segment categories for RAMCLass fragments:
Closes: #20644
Signed-off-by: Nick Kamal <[email protected]>