openj9
openj9 copied to clipboard
eclipse-openj9
WIP: Z JIT Pinning Support
This commit adds necessary changes on Z needed to support Virtual Threads.
- Increment and decrement ownedMonitorCount in J9VMThread when JIT compiled code on Z acquires and releases lock respectively.
- Increment and decrement callOutCount in J9VMThread around JNI call dispatch.
Signed-off-by: Rahil Shah [email protected]
I am attaching snippet of the log files from the different cases here for the reference.
One more thing I want to clarify about the synchronization sequence we generate on Z. During the trees lowering phase before generating instructions, we inspect trees to find out the synchronized region around a single field load and generate hardware optimized instruction Load Pair Disjoint which will load both field and the lockword in interlocked-fetch manner. In case if it fails to fetch both or the object is locked by another thread, we fall back to Out of line code section which calls helper for monent , reads the field and calls helper for monexit. [1]
This quick peek together on object lockward and read field skips acquiring lock and releasing it. As in this optimization pass we do not acquire a lock, we do not increment a ownedMonitorCount or decrement it.
@gacholio / @babsingh with respect to virtual thread, I wanted to check with both of you if it is OK?
[1]. https://github.com/eclipse-openj9/openj9/blob/827e35fef888a46159a4d91e17f0a7f3aeffeae5/runtime/compiler/z/codegen/ReduceSynchronizedFieldLoad.hpp#L33-L53
with respect to virtual thread, I wanted to check with both of you if it is OK?
There should be no issue with the optimization (presumably you issue the correct memory barriers in this case). Also, I assume this opt is disabled if the field read events are hooked (I believe this forces FSD).
Without having looked at the code for details, the counter needs to be managed any time you write to the lockword slot in any path, I believe.
We had couple of Loom related changes landing last week and with those changes, I retried the VirtualThread tests with stressing the JIT compiler [1] and test passes. I also have performed sanity tests (functional,openjdk and system) changes through internal jenkins build (Pipeline-Build-Test-Personal/14192/), with the changes with also enabling the Loom tests on Z with new JIT options I specified in [1]. (job/Test_openjdk19_j9_sanity.functional_s390x_linux_Personal_testList_1/14 and job/Test_openjdk19_j9_sanity.functional_s390x_linux_Personal_testList_0/14)
@joransiu Can I request you to give another round of review on this change?
[1]. https://github.com/r30shah/openj9/commit/5348a4d413a61cc7f71096a0155f1ae68684ce68
@joransiu I have made all the changes you suggested. This one is good to start tests.
Changes in this PR depends on some common API function @0xdaryl implemented in https://github.com/eclipse-openj9/openj9/pull/15552 (I apologize for forgetting to mention that). Marking this WIP till x86 change is merged.
I don't like the ifdef for 64-bit and Java19. I understand there are no plans to support 32-bit in future java versions, but these things always come back to haunt us. A better solution would be to ifdef for java19, then ifdef for 64-bit and #error in the 32-bit case, so at least we'll know what needs to be fixed if 32-bit comes back.
I don't like the ifdef for 64-bit and Java19. I understand there are no plans to support 32-bit in future java versions, but these things always come back to haunt us. A better solution would be to ifdef for java19, then ifdef for 64-bit and #error in the 32-bit case, so at least we'll know what needs to be fixed if 32-bit comes back.
@gacholio I have made the changes as you suggested. It will throw an ASSERT in case it is Java 19 on 31-Bit Z platform in https://github.com/eclipse-openj9/openj9/pull/15752/commits/dbcf4b6908142e6dcd1a7668e23f3dee26c60ba5. If you are OK with that, Will go ahead and squash the commits, so this will be ready to merge.
@r30shah : The x86 changes including the common changes have merged. Can you confirm this PR can be merged now?
@0xdaryl yes, now as all needed chages are merged, this one can be merged as well.