-
Notifications
You must be signed in to change notification settings - Fork 724
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.
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
Off heap #18163
base: master
Are you sure you want to change the base?
Off heap #18163
Conversation
@@ -26,11 +26,14 @@ | |||
#include "Math.hpp" | |||
#include "MemorySpace.hpp" | |||
#if defined(J9VM_GC_ENABLE_DOUBLE_MAP) | |||
#include <sys/mman.h> | |||
#include <errno.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check if this is still needed
runtime/gc_modron_startup/mminit.cpp
Outdated
@@ -2872,10 +2872,10 @@ gcInitializeDefaults(J9JavaVM* vm) | |||
extensions->setOmrVM(vm->omrVM); | |||
vm->omrVM->_gcOmrVMExtensions = (void *)extensions; | |||
vm->gcExtensions = vm->omrVM->_gcOmrVMExtensions; | |||
#if defined(J9VM_ENV_DATA64) | |||
#if defined(J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION) | |||
vm->isIndexableDualHeaderShapeEnabled = FALSE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be true. It was toggle on a while back as part of Dual Header shape PR.
#endif /* defined(J9VM_ENV_DATA64) */ | ||
#endif /* defined(J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION) */ | ||
} else if (isAllIndexableDataContiguousEnabled) { | ||
#if defined(J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably set this to NULL temporarily (rather than leaving it random), to avoid possible complication with GC occurring while this object is partially initialized
break; | ||
|
||
case GC_ArrayletObjectModel::Discontiguous: | ||
case GC_ArrayletObjectModel::Hybrid: | ||
if (NULL != spine) { | ||
indexableObjectModel->setSizeInElementsForDiscontiguous(spine, _numberOfIndexedFields); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might be redundant (set earlier for chunked array)?
return (void *)((uintptr_t)arrayPtr + contiguousIndexableHeaderSize()); | ||
void *dataAddr = (void *)((uintptr_t)arrayPtr + contiguousIndexableHeaderSize());; | ||
#if defined(J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION) | ||
if (isVirtualLargeObjectHeapEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's probably more correct to do isDataAddressPresent check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disregard the comment - isVirtualLargeObjectHeapEnabled is more specific/deterministic (and consistent with what DDR does)
/* Free arraylet leaf addresses if dynamically allocated */ | ||
if (arrayletLeafCount > ARRAYLET_ALLOC_THRESHOLD) { | ||
env->getForge()->free((void *)arrayletLeaveAddrs); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might need a special state for this regions (different from just plain ARRAYLET), such as ARRAYLET_DECOMMITED or just DECOMMITED
{ | ||
uintptr_t heapBase = (uintptr_t)extensions->heap->getHeapBase(); | ||
uintptr_t heapTop = (uintptr_t)extensions->heap->getHeapTop(); | ||
return ((uintptr_t)address >= heapBase) && ((uintptr_t)address < heapTop); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this API exists or should be in extensions. does not look anything like array specific
#if defined(J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION) | ||
void *dataAddr = getDataAddrForIndexableObject(arrayPtr); | ||
bool isObjectWithinHeap = isAddressWithinHeap(extensions, dataAddr); | ||
return ((getDataSizeInBytes(arrayPtr) >= _omrVM->_arrayletLeafSize) && (!isObjectWithinHeap)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the check in reality is toо generic for the very specific name
should we do a more double-map specific check or change the name to be more generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a more generic name would be isDataNonadjacentToHeader (picked to somewhat resembled the method above)
The difference between the 2 methods is that the above one consumes a size and is used during allocation/construction, while this method is used later (during GC or during verification) and consumes an already constructed/existing object.
We could name the methods the same name (sDataAdjacentToHeader) and let only the parameter distinguish them, but then the call sites for this one would have to be adjust to negate the result of the call.
c15b61f
to
b818ba6
Compare
/* allocate the next arraylet leaf */ | ||
void *leaf = env->_objectAllocationInterface->allocateArrayletLeaf(env, &_allocateDescription, | ||
_allocateDescription.getMemorySpace(), true); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arraylet leaf is zeroed in the MM_AllocationContextBalanced::allocateArrayletLeaf()
. This function is called from MM_TLHAllocationInterface::allocateArrayletLeaf()
if Allocation Context is not NULL.
If I don't miss something we are zeroing leaf unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps the first step is to try to add an assert MM_AllocationContextBalanced::allocateArrayletLeaf() when we zero the memory that offheap allocation is enabled (it's still ok to zero it if double mapping is enabled)
1398a78
to
30ed6ce
Compare
runtime/gc_base/RootScanner.cpp
Outdated
if (region->isArrayletLeaf()) { | ||
J9Object *spineObject = (J9Object *)region->_allocateData.getSpine(); | ||
Assert_MM_true(NULL != spineObject); | ||
doObjectInVirtualLargeObjectHeap(spineObject); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to introduce a flag in region that we guarantie that we call this only once for each offheap object
doublemapping ensures this by setting arrayletDoublemapID->address only to first arraylet region
|
||
if (NULL == data) { | ||
data = indexableObjectModel->getDataAddrForContiguous(arrayObject); | ||
if ((NULL == data) || indexableObjectModel->isAddressWithinHeap(_extensions, data)) { | ||
/* Doublemap failed, but we still need to continue execution; therefore fallback to previous approach */ | ||
copyArrayCritical(vmThread, indexableObjectModel, functions, &data, arrayObject, isCopy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we will enter copy path for 0 sized arrays, what might work, but is suboptimal.
Also, it's not clear under what conditions dataAddr for discontiguous array will point within heap.
Add definitions for j9gc_off_heap_allocation_enabled and J9::ObjectModel::isOffHeapAllocationEnabled(). Adding these changes separate from eclipse-openj9#18163 to unblock JIT PR. Signed-off-by: midronij <[email protected]> Signed-off-by: Shubham Verma <[email protected]>
@@ -1651,6 +1663,12 @@ class MM_WriteOnceCompactFixupRoots : public MM_RootScanner { | |||
scanJVMTIObjectTagTables(env); | |||
#endif /* J9VM_OPT_JVMTI */ | |||
|
|||
#if defined(J9VM_GC_SPARSE_HEAP_ALLOCATION) | |||
if (_includeVirtualLargeObjectHeap) { | |||
scanObjectsInVirtualLargeObjectHeap(env); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't we override doObjectInVirtualLargeObjectHeap for WOC? seems like we should update offheap (dataAddr->spine) mapping when spine moved
@@ -1676,6 +1694,21 @@ class MM_WriteOnceCompactFixupRoots : public MM_RootScanner { | |||
Assert_MM_unreachable(); | |||
} | |||
|
|||
#if defined(J9VM_GC_SPARSE_HEAP_ALLOCATION) | |||
virtual void doObjectInVirtualLargeObjectHeap(J9Object *objectPtr, bool *sparseHeapAllocation) { | |||
J9Object *forwarded = (J9IndexableObject *)getForwardingPtr(objectPtr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getForwardedPtr is WOC Api - should do _compactScheme->getForwardingPtr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forwarded object could/should be of type IndexableObject
#if defined(J9VM_GC_SPARSE_HEAP_ALLOCATION) | ||
virtual void doObjectInVirtualLargeObjectHeap(J9Object *objectPtr, bool *sparseHeapAllocation) { | ||
J9Object *forwarded = (J9IndexableObject *)getForwardingPtr(objectPtr); | ||
if ((NULL != forwarded) && (objectPtr != forwarded)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see how it can be NULL - we can try to assert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use some more common name like fwdObjectPtr
@@ -1676,6 +1694,21 @@ class MM_WriteOnceCompactFixupRoots : public MM_RootScanner { | |||
Assert_MM_unreachable(); | |||
} | |||
|
|||
#if defined(J9VM_GC_SPARSE_HEAP_ALLOCATION) | |||
virtual void doObjectInVirtualLargeObjectHeap(J9Object *objectPtr, bool *sparseHeapAllocation) { | |||
J9Object *forwarded = (J9IndexableObject *)getForwardingPtr(objectPtr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forwarded object could/should be of type IndexableObject
7493ac3
to
e5df160
Compare
jenkins test sanity,extended all jdk21 |
@@ -1676,6 +1697,21 @@ class MM_WriteOnceCompactFixupRoots : public MM_RootScanner { | |||
Assert_MM_unreachable(); | |||
} | |||
|
|||
#if defined(J9VM_GC_SPARSE_HEAP_ALLOCATION) | |||
virtual void doObjectInVirtualLargeObjectHeap(J9Object *objectPtr, bool *sparseHeapAllocation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method could pass J9IndexableObject, but we can do that cleanup later
jenkins test sanity,extended all jdk21 |
jenkins test sanity.system,extended.system all jdk21 |
jenkins test sanity.system,extended.system xmac jdk21 |
} | ||
if (NULL != dstObj) { | ||
dstBytes = (*env)->GetPrimitiveArrayCritical(env, dstObj, NULL); | ||
dstAddr = dstBytes; | ||
/* The java caller has added Unsafe.arrayBaseOffset() to the offset. Remove it | ||
* here as GetPrimitiveArrayCritical returns a pointer to the first element. | ||
*/ | ||
dstOffset -= J9VMTHREAD_CONTIGUOUS_INDEXABLE_HEADER_SIZE((J9VMThread*)env); | ||
dstOffset -= J9VMTHREAD_UNSAFE_INDEXABLE_HEADER_SIZE((J9VMThread*)env); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should put this as a separate fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this file from the changes (although it does not seem to cause conflicts since the changes are identical).
After that squash all the commits.
slotObject.writeReferenceToSlot(forwardedPtr); | ||
_interRegionRememberedSet->rememberReferenceForCompact(env, spineObject, forwardedPtr); | ||
if (region->_compactData._shouldFixup) { | ||
/* This fixing up is specific for hybrid arraylets, for off-heap array the fix-up has been done for contiguous array pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify this comment. This is enough:
For off-heap/non-adjacent arrays, the fix up is done when any other contiguous/adjacent array is fixed up.
jenkins test sanity,extended all jdk21 |
2, Introduce Off-Heap Technology for Large Arrays Introduce an optimized memory management for large arrays in a region-based garbage collector. More specifically, this is a new heap scheme and large object organization (in-heap and off-heap), an enhanced method for allocating large objects (proxy objects), and an optimized technique of accessing large arrays during runtime (JIT optimizations). 3, Updated PR Changes 4, enable xint by default 5, remove unneeded changes 6, updated changes 7, more updated changes old update 1 Fix build flag related issues on Java 8 -Fix incomplete issue to replace J9VM_ENV_DATA64 with J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION -Add/Enable buildflag gc_enableSparseHeapAllocation for J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION in axxon Java28 build -update getDataPointerForContiguous() to handle VirtualLargeObjectHeapEnabled case 2, Free the sparse region only once for all reserved leaf regions - new bool field _sparseHeapAllocation in MM_HeapRegionDescriptorVLHGC for identiying the first leaf region reserved for sparse heap allocation (default = false). - set the first leaf region reserved for sparse heap allocation = true during getSparseAddressAndDecommitLeaves(). - doObjectInVirtualLargeObjectHeap only once (for the first reserved leaf region) in MM_RootScanner::scanObjectsInVirtualLargeObjectHeap() 3, new Update from code reviewing Fix retrieving pontential stale pointer in jniGetPrimitiveArrayCritical Update to retrieve arrayObject after enterCriticalRegion() to avoid copy/move collection happen just before enterCriticalRegion() (enterCriticalRegion() would prevent copy/move collection before exitCriticalRegion()), the copy/move collection might move the arrayObject. then cause jniGetPrimitiveArrayCritical return stale data pointer. -replace isAddressWithinHeap() with (NULL != regionManager->regionDescriptorForAddress()). 4, Update macros rJ9JAVAARRAYCONTIGUOUS_EA Update Macro J9JAVAARRAYCONTIGUOUS_EA and J9JAVAARRAYCONTIGUOUS_EA_VM to also check isVirtualLargeObjectHeapEnabled if isIndexableDataAddrPresent == true to handle VirtualLargeObjectHeapDisabled case in Balanced GC( release JIT to initialize IndexableDataAddr for VirtualLargeObjectHeapDisabled case). Limitation: ArrayletDoubleMapping would not work properly. - new boolean isVirtualLargeObjectHeapEnabled in J9JavaVM - new boolean isVirtualLargeObjectHeapEnabled in J9VMThread 5, Detect offheap option for unsafe header reset vm.unsafeIndexableHeaderSize and vm.isVirtualLargeObjectHeapEnabled for off-heap case. 6, remove double mapping 7, DDR fixes and fix the issue in jniReleasePrimitiveArrayCritical (for zero size array, do not copyBackArrayCritical(), because doesn't copyArrayCritical() in jniGetPrimitiveArrayCritical() for removing double-mapping change). 8, Update from code review 9, DDR Changes for Off-Heap his change would not have any dependance on off-heap runtime changes, and should handle old core file(before any off-heap changes introduced), intermediate core file(XXgc:disableIndexableDualHeaderShape) and new core file(XXgc:enableVirtualLargeObjectHeap or XXgc:disableVirtualLargeObjectHeap) smoothly. use gcExtensions.isVirtualLargeObjectHeapEnabled for identifying if off heap is enabled or not. verify the DataAddr via hasCorrectDataAddrPointer() if isVirtualLargeObjectHeapEnabled. use javaVM.isIndexableDataAddrPresent for identifying if there is extra DataAddr field in the header of IndexableObject. output DataAddr after size for IndexableObject if isIndexableDataAddrPresent. handle new structures J9IndexableObjectWithDataAddressContiguous/Discontiguous use arrayletObjectModel->_arrayletRangeBase/ _arrayletRangeTop to verify if the address in heap. 10, code review update 11, update 2 for code review 12, Cmake update for off-heap new define J9VM_GC_SPARSE_HEAP_ALLOCATION for cmake build 13, Update Java8 build specs for off-heap new define J9VM_GC_SPARSE_HEAP_ALLOCATION for the build specs 14, J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION --> J9VM_GC_SPARSE_HEAP_ALLOCATION 15, compiler update J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION --> J9VM_GC_SPARSE_HEAP_ALLOCATION 16, rollback 17, j9gc_objaccess_arrayObjectDataDisplacement 18, remove "--enable-OMR_GC_SPARSE_HEAP_ALLOCATION" in mk files 19, replace with <#if uma.spec.flags.gc_sparseHeapAllocation.enabled> --enable-OMR_GC_SPARSE_HEAP_ALLOCATION \ </#if> in configure_common.mk.ftl 20, J9JAVAARRAYCONTIGUOUS_WITHOUT_DATAADDRESS_EA to J9JAVAARRAYCONTIGUOUS_BASE_EA_VM 21, remove unrelated changes 22, update for match PR20648 (Extract copy/free arrayCritical code from StandardAccessBarrier) Change-Id: I119f4f4f5bc52636d97019f2d147091b1d216acf Signed-off-by: lhu <[email protected]>
Signed-off-by: lhu <[email protected]>
- This fixing up is specific for hybrid arraylets, for off-heap array the fix-up has been done for contiguous array pass. So we should skip it for offheap enabled. It's a correctness problem, since we might be finding garbage references to scan. Additionally, because the leaf regions are supposed to be left decommited till the array dies, we are recommiting them too early. Signed-off-by: lhu <[email protected]>
The java caller has added Unsafe.arrayBaseOffset() to the offset. need to remove it as GetPrimitiveArrayCritical returns a pointer to the first element, need to replace J9VMTHREAD_CONTIGUOUS_INDEXABLE_HEADER_SIZE() with J9VMTHREAD_UNSAFE_INDEXABLE_HEADER_SIZE() for off-heap case. Signed-off-by: lhu <[email protected]>
Signed-off-by: lhu <[email protected]>
jenkins test sanity,extended all jdk21 |
jenkins test sanity,extended aix jdk8 |
Introduce Off-Heap Technology for Large Arrays in Balanced Region-Based Garbage Collector
carryforward from #14667