Z JIT Pinning Support #15752

r30shah · 2022-08-18T21:28:38Z

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 rahil@ca.ibm.com

r30shah · 2022-08-19T17:03:30Z

I am attaching snippet of the log files from the different cases here for the reference.

r30shah · 2022-08-19T17:10:35Z

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

openj9/runtime/compiler/z/codegen/ReduceSynchronizedFieldLoad.hpp

Lines 33 to 53 in 827e35f

    
           /** \brief 
        
            *     Reduces synchronized regions around a single field load into a codegen inlined recognized method call which 
        
            *     generates a hardware optimized instruction sequence which is semantically equivalent to the synchronized field 
        
            *     load. 
        
            * 
        
            *  \details 
        
            *     This codegen phase pass pattern matches the following tree sequences (modulo NOP trees such as aRegLoad): 
        
            * 
        
            *     \code 
        
            *     monent 
        
            *       object 
        
            *     aloadi / iloadi / i2a 
        
            *       ==> object 
        
            *     monexitfence 
        
            *     monexit 
        
            *       ==> object 
        
            *     \endcode 
        
            * 
        
            *     And replaces the entire treetop sequence, excluding the monexitfence, with a call to a codegen inlined method 
        
            *     <synchronizedFieldLoad>. 
        
            */

r30shah · 2022-08-19T17:10:55Z

@joransiu / @0xdaryl Can I get your review on this change?

gacholio · 2022-08-19T18:31:33Z

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

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

gacholio · 2022-08-24T17:10:01Z

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.

r30shah · 2022-08-24T20:14:12Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

@@ -7609,11 +7625,15 @@ reservationLockExit(TR::Node *node, int32_t lwOffset, TR::Register *objectClassR
      generateRXInstruction(cg, storeOp, node, monitorReg, generateS390MemoryReference(objReg, lwOffset, cg));

      if (!isPrimitive)
+         {   


In OOL of MonExit, for primitive lock reservation, the above lockward update (Increment to 1 ) in mon exit is done for the helper call. We do not need to update ownedMonitorCount here as in case of primitive we anyway call the helper. So we should be generating the ownedMonitorCount update in OOL in case of non primitive only,

If you call to the assembly helpers, they will manage the count for you.

Yes, so the JIT code currently in this path calls helper if for primitive lock reservation. For non primitive in this case we have a success so skip helper call. The reason we store the updated lock ward before calling the JIT helper in primitive case, is that, while generating a code for primitive lock reservation for MonEnt, we skip updating Recursion Count, but in case of MonExit, if OBJECT_HEADER_LOCK_RECURSION_MASK is set on lockward, it calls helper and to compensate of not incrementing it during monent, we do it here in monexit OOL before calling helper.

I also had a brief discussion about this with @IBMJimmyk , primitive lock reservation code generated by JIT seems to be incorrect in general. On Z, we would anyway won't generate the lock reservation code as it is disabled due to regression and deadlock seen due to it on couple of workloads.

r30shah · 2022-09-14T20:23:41Z

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]. r30shah@5348a4d

joransiu

Changes LGTM. Would like to see the comments for the various sequences updated to include the new owned monitor counter instructions too, for completeness and consistency .. and it's easier to read than the code.

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

r30shah · 2022-09-16T20:54:24Z

@joransiu I have made all the changes you suggested. This one is good to start tests.

joransiu · 2022-09-16T21:04:15Z

jenkins test sanity zlinux jdk17,jdk19

joransiu · 2022-09-16T21:10:04Z

@gacholio : Do you mind reviewing the latest changes too? Thanks!

r30shah · 2022-09-19T13:49:31Z

Changes in this PR depends on some common API function @0xdaryl implemented in #15552 (I apologize for forgetting to mention that). Marking this WIP till x86 change is merged.

joransiu · 2022-09-19T13:59:13Z

jenkins test sanity zlinux jdk17,jdk19 depends #15552

gacholio · 2022-09-20T15:37:40Z

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.

This commit adds necessary changes on Z needed to support Virtual Threads. 1. Increment and decrement ownedMonitorCount in J9VMThread when JIT compiled code on Z acquires and releases lock respectively. 2. Increment and decrement callOutCount in J9VMThread around JNI call dispatch. Signed-off-by: Rahil Shah <rahil@ca.ibm.com>

Signed-off-by: Rahil Shah <rahil@ca.ibm.com>

r30shah · 2022-09-26T18:36:08Z

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 dbcf4b6. If you are OK with that, Will go ahead and squash the commits, so this will be ready to merge.

gacholio

I would have made it a compile failure, but this will do.

0xdaryl · 2022-10-01T13:03:27Z

Jenkins test sanity zlinux,zos jdk17,jdk19

0xdaryl · 2022-10-03T01:05:11Z

@r30shah : The x86 changes including the common changes have merged. Can you confirm this PR can be merged now?

r30shah · 2022-10-03T03:08:42Z

@0xdaryl yes, now as all needed chages are merged, this one can be merged as well.

keithc-ca · 2022-10-03T13:22:25Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

@@ -7351,13 +7361,20 @@ reservationLockEnter(TR::Node *node, int32_t lwOffset, TR::Register *objectClass
      // XR    monitorReg, monitorReg
      // CS    monitorReg, valReg, #lwOffset(objectReg)
      // BRC   MASK6, callHelper
+      // #IF 64-Bit and JAVA_VERSION >= 19
+      //    BRC   incrementOwnedMonitorCountLabel
+      // #ELSEIF


Should this be ELSE is a condition missing?

Seems like it should be ELSE. @r30shah, if so, please correct in separate PR.

Fixed in #16063

r30shah force-pushed the JITPinningZPR branch from d6b49c6 to 84dda9f Compare August 18, 2022 21:37

r30shah changed the title ~~WIP: Z JIT Pinning Support~~ Z JIT Pinning Support Aug 22, 2022

joransiu reviewed Aug 22, 2022

View reviewed changes

runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved

r30shah force-pushed the JITPinningZPR branch 3 times, most recently from d879319 to 99ba654 Compare August 24, 2022 20:09

r30shah commented Aug 24, 2022

View reviewed changes

r30shah force-pushed the JITPinningZPR branch from 99ba654 to aa66f91 Compare August 24, 2022 20:23

r30shah force-pushed the JITPinningZPR branch from aa66f91 to e19a004 Compare September 6, 2022 14:57

fengxue-IS added the project:loom Used to track Project Loom related work label Sep 13, 2022

r30shah force-pushed the JITPinningZPR branch 2 times, most recently from bbc1024 to 501f816 Compare September 14, 2022 14:02

joransiu reviewed Sep 16, 2022

View reviewed changes

runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved

runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved

r30shah force-pushed the JITPinningZPR branch 2 times, most recently from 0542b72 to 0ba30b3 Compare September 16, 2022 20:51

joransiu approved these changes Sep 16, 2022

View reviewed changes

r30shah changed the title ~~Z JIT Pinning Support~~ WIP: Z JIT Pinning Support Sep 19, 2022

r30shah mentioned this pull request Sep 23, 2022

x64 JIT support for virtual threads (Loom) #15552

Merged

r30shah force-pushed the JITPinningZPR branch from 0ba30b3 to ac7dfad Compare September 26, 2022 15:19

Update the GUARDS around Loom related code on Z codegen

dbcf4b6

Signed-off-by: Rahil Shah <rahil@ca.ibm.com>

r30shah force-pushed the JITPinningZPR branch from ac7dfad to dbcf4b6 Compare September 26, 2022 16:19

r30shah changed the title ~~WIP: Z JIT Pinning Support~~ Z JIT Pinning Support Sep 26, 2022

gacholio approved these changes Sep 26, 2022

View reviewed changes

0xdaryl approved these changes Oct 3, 2022

View reviewed changes

0xdaryl self-assigned this Oct 3, 2022

0xdaryl merged commit 54ff24d into eclipse-openj9:master Oct 3, 2022

keithc-ca reviewed Oct 3, 2022

View reviewed changes

ChengJin01 mentioned this pull request Oct 14, 2022

JDK 19 MiniMix_5m_1 hang in Thread.getStateImpl #16036

Closed

babsingh added the comp:jit label May 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Z JIT Pinning Support #15752

Z JIT Pinning Support #15752

r30shah commented Aug 18, 2022

r30shah commented Aug 19, 2022

r30shah commented Aug 19, 2022

r30shah commented Aug 19, 2022

gacholio commented Aug 19, 2022

gacholio commented Aug 24, 2022

r30shah Aug 24, 2022

gacholio Aug 24, 2022

r30shah Aug 24, 2022

r30shah commented Sep 14, 2022 •

edited

Loading

joransiu left a comment

r30shah commented Sep 16, 2022

joransiu commented Sep 16, 2022

joransiu commented Sep 16, 2022

r30shah commented Sep 19, 2022

joransiu commented Sep 19, 2022

gacholio commented Sep 20, 2022

r30shah commented Sep 26, 2022 •

edited

Loading

gacholio left a comment

0xdaryl commented Oct 1, 2022

0xdaryl commented Oct 3, 2022

r30shah commented Oct 3, 2022

keithc-ca Oct 3, 2022

0xdaryl Oct 5, 2022

r30shah Oct 6, 2022

Z JIT Pinning Support #15752

Z JIT Pinning Support #15752

Conversation

r30shah commented Aug 18, 2022

r30shah commented Aug 19, 2022

r30shah commented Aug 19, 2022

r30shah commented Aug 19, 2022

gacholio commented Aug 19, 2022

gacholio commented Aug 24, 2022

r30shah Aug 24, 2022

Choose a reason for hiding this comment

gacholio Aug 24, 2022

Choose a reason for hiding this comment

r30shah Aug 24, 2022

Choose a reason for hiding this comment

r30shah commented Sep 14, 2022 • edited Loading

joransiu left a comment

Choose a reason for hiding this comment

r30shah commented Sep 16, 2022

joransiu commented Sep 16, 2022

joransiu commented Sep 16, 2022

r30shah commented Sep 19, 2022

joransiu commented Sep 19, 2022

gacholio commented Sep 20, 2022

r30shah commented Sep 26, 2022 • edited Loading

gacholio left a comment

Choose a reason for hiding this comment

0xdaryl commented Oct 1, 2022

0xdaryl commented Oct 3, 2022

r30shah commented Oct 3, 2022

keithc-ca Oct 3, 2022

Choose a reason for hiding this comment

0xdaryl Oct 5, 2022

Choose a reason for hiding this comment

r30shah Oct 6, 2022

Choose a reason for hiding this comment

r30shah commented Sep 14, 2022 •

edited

Loading

r30shah commented Sep 26, 2022 •

edited

Loading