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

Verify off heap entry during update and free offheap array #20985

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

LinHu2016
Copy link
Contributor

Verify the consistency (dataAddr, size and related proxy array object) of off heap Entry before freeing the off heap array or updating related proxy object.

depends on: eclipse-omr/omr#7614

Signed-off-by: lhu [email protected]

@LinHu2016
Copy link
Contributor Author

@amicic @dmitripivkine please review the changes, thanks

@LinHu2016 LinHu2016 force-pushed the update_isValidDataPtr branch 3 times, most recently from a89417b to 1c5f6bb Compare January 27, 2025 17:15
@@ -468,10 +468,19 @@ GC_CheckEngine::checkJ9Object(J9JavaVM *javaVM, J9Object* objectPtr, J9MM_Iterat
if (extensions->isVirtualLargeObjectHeapEnabled && extensions->objectModel.isIndexable(objectPtr)) {
/* Check that the indexable object has the correct data address pointer */
void *dataAddr = extensions->indexableObjectModel.getDataAddrForIndexableObject((J9IndexableObject *)objectPtr);
bool isValidDataAddr = extensions->largeObjectVirtualMemory->getSparseDataPool()->isValidDataPtr(dataAddr);
if (!isValidDataAddr && !extensions->indexableObjectModel.isValidDataAddr((J9IndexableObject *)objectPtr, dataAddr, isValidDataAddr)) {
bool isDataAddrForOffHeapObject = false;
Copy link
Contributor

@amicic amicic Feb 3, 2025

Choose a reason for hiding this comment

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

this could be simply named isOffheapObject
here and in the arguments of isValidDataAddr()

edit: should be renamed to isDataNonAdjacent to be consistent with the method rename (isValidDataAddrForAdjacentData)

*/
MMINLINE bool
isValidDataAddr(J9IndexableObject *arrayPtr, bool isValidDataAddrForOffHeapObject)
isValidDataAddr(J9IndexableObject *arrayPtr, bool *isDataAddrForOffHeapObject)
Copy link
Contributor

@amicic amicic Feb 3, 2025

Choose a reason for hiding this comment

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

perhaps these 2 methods should be named more specifically, for example isValidDataAddrForHeapObject

@dmitripivkine opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

or isValidDataAddrForAdjacentData

Copy link
Contributor

Choose a reason for hiding this comment

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

latter one is better

Copy link
Contributor

@amicic amicic Feb 3, 2025

Choose a reason for hiding this comment

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

ok, then to be consistent we should rename the output parameter to be isDataNonAdjacent (instead of isOffheapObject as I suggested in an earlier comment)

* @return if the dataAddr field of the indexable object is correct
* @param arrayPtr[in] Pointer to the indexable object
* @param isDataAddrForOffHeapObject[out] set true if the given indexable object is off heap
* @return if the dataAddr field of the indexable object is correct((not heap object case), return false if indexable object is off heap
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: ((

@LinHu2016 LinHu2016 force-pushed the update_isValidDataPtr branch from 1c5f6bb to 3440684 Compare February 4, 2025 15:51
Verify the consistency (dataAddr, size and related proxy array object)
of off heap Entry before freeing the off heap array or updating related
proxy object.

Signed-off-by: lhu <[email protected]>
@LinHu2016 LinHu2016 force-pushed the update_isValidDataPtr branch from 3440684 to 2ea0866 Compare February 4, 2025 15:59
@amicic
Copy link
Contributor

amicic commented Feb 4, 2025

jenkins test sanity xLinux jdk21

@amicic amicic merged commit a134afa into eclipse-openj9:master Feb 5, 2025
6 checks passed
*sparseHeapAllocation = false;
} else {
void *dataAddr = _extensions->indexableObjectModel.getDataAddrForContiguous((J9IndexableObject *)fwdOjectPtr);
if (NULL != dataAddr) {
/* There might be the case that GC finds a floating arraylet, which was a result of an allocation
* failure (reason why this GC cycle is happening).
*/
_extensions->largeObjectVirtualMemory->updateSparseDataEntryAfterObjectHasMoved(dataAddr, fwdOjectPtr);
}
_extensions->largeObjectVirtualMemory->updateSparseDataEntryAfterObjectHasMoved(dataAddr, objectPtr, _extensions->indexableObjectModel.getDataSizeInBytes((J9IndexableObject *)fwdOjectPtr), fwdOjectPtr); }
Copy link
Contributor

Choose a reason for hiding this comment

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

@LinHu2016 Could you create a follow-up change to put the closing brace back on a line by itself, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have created a PR, which also clean up some others format issues, #21072

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants