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

[FFI] Remove PUSH_OBJECT_IN_SPECIAL_FRAME in the upcall dispatcher #16427

Conversation

ChengJin01
Copy link

@ChengJin01 ChengJin01 commented Dec 7, 2022

The idea is to store the argument objects in native to the java array rather to avoid
misusing PUSH_OBJECT_IN_SPECIAL_FRAME so as to stop GC from updating the
references before calling into the interpreter in upcall.

Fixes: #16410, #16422

Signed-off-by: ChengJin01 jincheng@ca.ibm.com

@ChengJin01
Copy link
Author

ChengJin01 commented Dec 7, 2022

As explained at #16410 (comment), PUSH_OBJECT_IN_SPECIAL_FRAME is removed everywhere given it is problematic to use this macro in our special situations in terms of multiple GC points during the memory allocation for struct/pointer/arguments after building the call-in frame.

To avoid any unexpected behavior with GC, we simly store the allocated struct/pointer objects to a java array (already created in java code in advance) instead and load them to the java stack prior to upcall.

The changes were verified on all supported platforms.

Reviewer: @gacholio, @tajila
FYI: @pshipton, @dmitripivkine

@ChengJin01 ChengJin01 added comp:vm project:panama Used to track Project Panama related work labels Dec 7, 2022
@ChengJin01 ChengJin01 force-pushed the ffi_remove_push_special_frame_upcall_dispatcher_v1 branch from a2794b5 to 092faff Compare December 7, 2022 15:43
@ChengJin01 ChengJin01 force-pushed the ffi_remove_push_special_frame_upcall_dispatcher_v1 branch 2 times, most recently from 9f0aad6 to 1836872 Compare December 7, 2022 19:44
@ChengJin01 ChengJin01 force-pushed the ffi_remove_push_special_frame_upcall_dispatcher_v1 branch 3 times, most recently from d4e7895 to eafaa74 Compare December 7, 2022 20:48
@ChengJin01
Copy link
Author

ChengJin01 commented Dec 7, 2022

A session specific test failure was detected in one of Jtreg test suites. I will need to double-check the code to see whether something was modified incorrectly for the session.

test LibraryLookupTest.testLoadLibrarySharedClosed(): failure
java.lang.AssertionError: expected [true] but found [false]
        at org.testng.Assert.fail(Assert.java:99)
        at org.testng.Assert.failNotEquals(Assert.java:1037)
        at org.testng.Assert.assertTrue(Assert.java:45)
        at org.testng.Assert.assertTrue(Assert.java:55)
        at LibraryLookupTest.testLoadLibrarySharedClosed(LibraryLookupTest.java:144)

@ChengJin01
Copy link
Author

ChengJin01 commented Dec 8, 2022

I went through the code with session in the PR and didn't find anything incorrect in dealing with session.

The reason for the failing test LibraryLookupTest.testLoadLibrarySharedClosed at #16427 (comment) is that it is supposed to complete at a fixed max time at https://github.com/ibmruntimes/openj9-openjdk-jdk19/blob/master/test/jdk/java/foreign/LibraryLookupTest.java#L144

void testLoadLibrarySharedClosed() throws Throwable {
...
        accessExecutor.shutdown();
        assertTrue(accessExecutor.awaitTermination(MAX_EXECUTOR_WAIT_SECONDS, TimeUnit.SECONDS)); <---------
    }

in which case exceeding the max time leads to failure. That being said, the explicit GC calls (especially the global GC) delayed the normal execution of test case (and also disturbed other time -sensitive tests). So these tests passed by simply disabling the global GC call; otherwise, I didn't spot anything wrong/abnormal in tests in terms of stack walker due to the explicit GC calls except that it took much longer in execution.

The idea is to store the argument objects in native to the java
array rather to avoid misusing PUSH_OBJECT_IN_SPECIAL_FRAME so
as to stop GC from updating the references before calling into
the interpreter in upcall.

Fixes: eclipse-openj9#16410, eclipse-openj9#16422

Signed-off-by: ChengJin01 <jincheng@ca.ibm.com>
@ChengJin01 ChengJin01 force-pushed the ffi_remove_push_special_frame_upcall_dispatcher_v1 branch from eafaa74 to d620448 Compare December 8, 2022 13:59
@gacholio
Copy link
Contributor

gacholio commented Dec 8, 2022

jenkins test sanity zlinux jdk19

@gacholio gacholio merged commit 909f046 into eclipse-openj9:master Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:vm project:panama Used to track Project Panama related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jdk17 Jep389Tests_testClinkerFfi_UpCall Object neither in heap nor stack-allocated in thread main
3 participants