Skip to content
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

Merged
merged 1 commit into from
Dec 5, 2024
Merged

Off heap #18163

merged 1 commit into from
Dec 5, 2024

Conversation

LinHu2016
Copy link
Contributor

@LinHu2016 LinHu2016 commented Sep 19, 2023

Introduce Off-Heap Technology for Large Arrays in Balanced Region-Based Garbage Collector
carryforward from #14667

@@ -26,11 +26,14 @@
#include "Math.hpp"
#include "MemorySpace.hpp"
#if defined(J9VM_GC_ENABLE_DOUBLE_MAP)
#include <sys/mman.h>
#include <errno.h>
Copy link
Contributor

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

@@ -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;
Copy link
Contributor

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)
Copy link
Contributor

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);
Copy link
Contributor

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()) {
Copy link
Contributor

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

Copy link
Contributor

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);
}
Copy link
Contributor

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);
Copy link
Contributor

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));
Copy link
Contributor

@amicic amicic Sep 20, 2023

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?

Copy link
Contributor

@amicic amicic Jul 10, 2024

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.

@LinHu2016 LinHu2016 force-pushed the off-heap branch 4 times, most recently from c15b61f to b818ba6 Compare September 23, 2023 16:07
/* allocate the next arraylet leaf */
void *leaf = env->_objectAllocationInterface->allocateArrayletLeaf(env, &_allocateDescription,
_allocateDescription.getMemorySpace(), true);

Copy link
Contributor

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.

Copy link
Contributor

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)

@LinHu2016 LinHu2016 force-pushed the off-heap branch 2 times, most recently from 1398a78 to 30ed6ce Compare October 3, 2023 14:57
if (region->isArrayletLeaf()) {
J9Object *spineObject = (J9Object *)region->_allocateData.getSpine();
Assert_MM_true(NULL != spineObject);
doObjectInVirtualLargeObjectHeap(spineObject);
Copy link
Contributor

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);
Copy link
Contributor

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.

VermaSh added a commit to VermaSh/openj9 that referenced this pull request Oct 12, 2023
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]>
MM_JNICriticalRegion::enterCriticalRegion(vmThread, true);
Assert_MM_true(vmThread->publicFlags & J9_PUBLIC_FLAGS_VM_ACCESS);
arrayObject = (J9IndexableObject*)J9_JNI_UNWRAP_REFERENCE(array);
arrayObject = (J9IndexableObject *)J9_JNI_UNWRAP_REFERENCE(array);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is overkill since we acquired VM access at the start of the method
(or we should acquire VM access selectively at later points)

}
/* if this is a contiguous array, we must have exhausted the iterator */
Assert_MM_true(NULL == it.nextSlot());
} else if (GC_ArrayletObjectModel::Discontiguous == layout) {
/* do nothing - no inline pointers */
Copy link
Contributor

@amicic amicic Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could assert is zero-sized array for offheap

@@ -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);
Copy link
Contributor

@amicic amicic Nov 22, 2024

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);
Copy link
Contributor

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

Copy link
Contributor

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)) {
Copy link
Contributor

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.

Copy link
Contributor

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);
Copy link
Contributor

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

@LinHu2016 LinHu2016 force-pushed the off-heap branch 4 times, most recently from 7493ac3 to e5df160 Compare November 25, 2024 15:44
@amicic
Copy link
Contributor

amicic commented Nov 25, 2024

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) {
Copy link
Contributor

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

@amicic
Copy link
Contributor

amicic commented Nov 25, 2024

jenkins test sanity,extended all jdk21

@amicic
Copy link
Contributor

amicic commented Nov 25, 2024

jenkins test sanity.system,extended.system all jdk21

@amicic
Copy link
Contributor

amicic commented Nov 25, 2024

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);
Copy link
Contributor

@amicic amicic Dec 3, 2024

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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.
Copy link
Contributor

@amicic amicic Dec 3, 2024

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.

@amicic
Copy link
Contributor

amicic commented Dec 3, 2024

jenkins test sanity,extended all jdk21

@amicic
Copy link
Contributor

amicic commented Dec 4, 2024

jenkins test sanity,extended all jdk21

@amicic
Copy link
Contributor

amicic commented Dec 4, 2024

jenkins test sanity,extended aix jdk8

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)
23, updateSparseDataEntryAfterObjectHasMoved in
WriteOnceCompactFixupRoots
24, Update fixupArrayletLeafRegionContentsAndObjectLists

	- 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.

25, fix JVM_CopySwapMemory crash issue for off-heap

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.

26, Rollback JVM_CopySwapMemory changes

Change-Id: I2597c71ccf4c5fcc0f3aa3aea9f793e783a0a304
Signed-off-by: lhu <[email protected]>
@amicic
Copy link
Contributor

amicic commented Dec 5, 2024

Last commit was just a squash so we lost the references to jenkins builds, but they were stable other than a couple infra problems.

@amicic amicic merged commit d1a8805 into eclipse-openj9:master Dec 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants