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

Update JVM_CopySwapMemory for off-heap enabled case #20742

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

LinHu2016
Copy link
Contributor

The java caller has added Unsafe.arrayBaseOffset() to the offset for JVM_CopySwapMemory JNI API, 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, because arrayBaseOffset() == 0 when off-heap is enabled.

The java caller has added Unsafe.arrayBaseOffset() to the offset
for JVM_CopySwapMemory JNI API, 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,
because arrayBaseOffset() == 0 when off-heap is enabled.

Signed-off-by: lhu <[email protected]>
@LinHu2016
Copy link
Contributor Author

@amicic @dmitripivkine please review the changes, Thanks

@LinHu2016 LinHu2016 mentioned this pull request Dec 3, 2024
@@ -129,15 +129,15 @@ JVM_CopySwapMemory(JNIEnv *env, jobject srcObj, jlong srcOffset, jobject dstObj,
/* The java caller has added Unsafe.arrayBaseOffset() to the offset. Remove it
* here as GetPrimitiveArrayCritical returns a pointer to the first element.
*/
srcOffset -= J9VMTHREAD_CONTIGUOUS_INDEXABLE_HEADER_SIZE((J9VMThread*)env);
srcOffset -= J9VMTHREAD_UNSAFE_INDEXABLE_HEADER_SIZE((J9VMThread*)env);
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what's preferred, but wonder if can you call VM_UnsafeAPI::arrayBase() instead

Copy link
Contributor

Choose a reason for hiding this comment

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

We cant use the C++ API in C files.

@amicic
Copy link
Contributor

amicic commented Dec 3, 2024

looks ok to me, but I think someone from VM team should approve/merge it

@pshipton pshipton requested review from tajila and removed request for pshipton December 3, 2024 22:44
@tajila
Copy link
Contributor

tajila commented Dec 4, 2024

jenkins test sanity alinux64 jdk21

@tajila tajila merged commit c963d9e into eclipse-openj9:master Dec 4, 2024
6 checks passed
@tajila
Copy link
Contributor

tajila commented Dec 4, 2024

@LinHu2016 does this need to go into 0.49?

@amicic
Copy link
Contributor

amicic commented Dec 4, 2024

No need, the 2 values will only differ once offheap is enabled.

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.

3 participants