Loom Support: Pin Counters #15383

babsingh · 2022-06-21T17:37:56Z

Reasons for pinning:

	public enum Pinned {
		/** In native code */
		NATIVE,
		/** Holding monitor(s) */
		MONITOR,
		/** In critical section */
		CRITICAL_SECTION
	}

Support for #15174 and #15175:

J9VMThread->continuationPinCount will support native methods in
class Continuation: pin() and unpin(). It will also be associated to
Pinned.CRITICAL_SECTION.
J9VMThread->ownedMonitorCount will be associated to Pinned.MONITOR.
J9VMThread->callOutCount will be associated to Pinned.NATIVE.

Signed-off-by: Babneet Singh sbabneet@ca.ibm.com

tajila · 2022-06-21T17:43:26Z

runtime/oti/j9nonbuilder.h

@@ -4885,6 +4885,7 @@ typedef struct J9VMContinuation {
 	UDATA* stackOverflowMark;
 	UDATA* stackOverflowMark2;
 	J9JavaStack* stackObject;
+	UDATA pinCount;


why is this separate from the others?

or perhaps, I'm misinterpreting this. Is that to track to number of times the continuation has been pinned?

Because it is a property of the continuation. Others are properties of the thread.

or perhaps, I'm misinterpreting this. Is that to track to number of times the continuation has been pinned?

Yes.

I don't see linkNext and linkPrevious fields in J9VMContinuation. Will J9VMThread maintain a list of Continuations? To support Continuation.isPinned, it needs to be a list since the parent of a Continuation also needs to be checked to determine if it is pinned.

tracking continuation lineage can be done in java code (ie. continuation.parent/child).

I don't see linkNext and linkPrevious fields in J9VMContinuation

This was intended for the purposes of finding all continuations in core dumps

Okay, it looks like the pinning fields are a lot like the interpreterfields in that the "live" versions are on the vmthread, and the "saved" versions are on the j9vmcontinuation.

So the short answer is all three can be put on the thread (this way it remains a fast path access). When we add support for nesting we'll need to add all three to the continuation as well. Updating the fields in the continuation will be handled by the code that does the enter/yield.

The pin/unpin natives pin the current continuation (and in the case of nesting, the most deeply nested one), which will always be the one with the live state on the j9vmthread.

Updated as suggested. Note: RI handles nested continuations in its current implementation. Our current implementation won't be able to handle the below example. We are forcing a 1-to-1 relationship between a ContinuationScope and Continuation.

public static boolean isPinned(ContinuationScope scope) { ContinuationScope X { Continuation A, Continuation B, Continuation C } Continuation A { //Pinned ... Continuation B { //Pinned ... Continuation C { ... } ... } ... }

If nesting is required then we would need to put all 3 counters in the continuation object

We may only need ownedMonitorCount and pinCount in continuation. But, we will need to keep ownedMonitorCount in J9VMThread. We will also need to stackwalk in Coninuation.isPinned(ContinuationScope scope) to determine if any of the nested continuations has a stack frame on the thread. This will remove the need for callOutCount in J9VMThread.

Since RI currently handles nested continuations, should we do the same?

We will, but for the purposes of unblocking JIT work I think we can keep this PR to the minimal set of changes we need right now

tajila · 2022-06-22T14:47:30Z

Jenkins test sanity amac jdk19

gacholio · 2022-06-22T17:18:35Z

10:52:30  /Users/jenkins/workspace/Build_JDK19_aarch64_mac_Personal/openj9/runtime/vm/vmthread.cpp:265:13: error: no member named 'pinnedStateCounter' in 'J9VMThread'
10:52:30          newThread->pinnedStateCounter = 0;

gacholio · 2022-06-22T17:19:30Z

Is there a good reason to add the flag in this PR? It's not something the JIT will need to see. I'd prefer if it were added and used in the same PR.

babsingh · 2022-06-22T18:04:32Z

Is there a good reason to add the flag in this PR? It's not something the JIT will need to see. I'd prefer if it were added and used in the same PR.

As per #15175 (comment), the JIT plans to directly check if the bit is set.

babsingh · 2022-06-22T18:13:54Z

error: no member named 'pinnedStateCounter' in 'J9VMThread'

Fixed. Testing locally.

tajila · 2022-06-22T18:15:53Z

Jenkins test sanity amac jdk19

gacholio · 2022-06-22T18:26:52Z

I don't see why the JIT would check a bit (runtime checks are the very last thing the JIT ever wants to do). I'm also not clear on what thread this bit would be set - there are no J9VMThreads for virtual threads last time we discussed this.

gacholio · 2022-06-22T18:50:08Z

I suppose it's the carrier thread that gets the bit.

To me, it would make more sense to inc/dec the counter in the inline JIT monent/exit code always (if the JCL level is high enough), and zero the counters whenever a vthread is mounted (we don't care about the values on the carrier thread itself).

tajila · 2022-06-22T18:53:09Z

I don't see why the JIT would check a bit (runtime checks are the very last thing the JIT ever wants to do). I'm also not clear on what thread this bit would be set - there are no J9VMThreads for virtual threads last time we discussed this.

The JIT needs a fast way to tell if there is a virtual thread mounted on the carrier thread (j9vmthread) so they know if they need to update the pin state counters or not. The flag seems like the simplest way. They could also check J9OBJECT_CLAZZ(j9vmthread->threadObject) == J9VM_JAVALANGVIRTUALTHREAD but thats more overhead.

tajila · 2022-06-22T18:54:00Z

To me, it would make more sense to inc/dec the counter in the inline JIT monent/exit code always (if the JCL level is high enough), and zero the counters whenever a vthread is mounted (we don't care about the values on the carrier thread itself).

@0xdaryl what are your thoughts on this?

babsingh · 2022-06-22T20:56:28Z

https://openj9-jenkins.osuosl.org/job/Test_openjdk19_j9_sanity.functional_aarch64_mac_Personal/2/tapResults/

The test failure occurs in thrstatetest_0. All tests pass, but test suite has failed to destroy the JVM.

306/306 Tests passed. 0 Failures ignored. 123 Failures expected.
ALL TESTS COMPLETED AND PASSED
Failed to destroy JVM

thrstatetest_0_FAILED

I am investigating. Meanwhile, a decision on the new approach can be finalized.

gacholio · 2022-06-23T02:09:45Z

We have our meeting today, so we can discuss.

0xdaryl · 2022-06-23T12:07:41Z

The suggestion for a flag check was based on the understanding that the counters could only be updated under certain conditions and as a much faster alternative to calling out to the VM to determine those conditions (whether a virtual thread is mounted). If that's not true then I think unconditionally inc/dec'ing a counter will be a good solution (assuming the counter fields are on the VMThread and are in the cache).

Reasons for pinning: public enum Pinned { /** In native code */ NATIVE, /** Holding monitor(s) */ MONITOR, /** In critical section */ CRITICAL_SECTION } Support for eclipse-openj9#15174 and eclipse-openj9#15175: - J9VMThread->continuationPinCount will support native methods in class Continuation: pin() and unpin(). It will also be associated to Pinned.CRITICAL_SECTION. - J9VMThread->ownedMonitorCount will be associated to Pinned.MONITOR. - J9VMThread->callOutCount will be associated to Pinned.NATIVE. Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

babsingh · 2022-06-23T14:48:20Z

The flag has been removed.
The failure reported in Loom Support: Pin Counters #15383 (comment) also happens without the changes from this PR. Opened Segfault in j9shmem_handle_stat: Corrupt handle=0x2 #15408 to track this failure.

gacholio

Zeroing the fields is unnecessary but we can fix that later.

babsingh requested review from gacholio and tajila June 21, 2022 17:38

babsingh mentioned this pull request Jun 21, 2022

Loom: JIT Pinning support #15175

Closed

babsingh force-pushed the loom_support branch from 3dbe18c to 1eea957 Compare June 21, 2022 17:41

tajila reviewed Jun 21, 2022

View reviewed changes

babsingh force-pushed the loom_support branch 3 times, most recently from c74eba5 to 7bc8de4 Compare June 21, 2022 21:22

tajila approved these changes Jun 22, 2022

View reviewed changes

babsingh force-pushed the loom_support branch 2 times, most recently from cb66784 to df0d5c2 Compare June 22, 2022 18:09

babsingh mentioned this pull request Jun 23, 2022

Segfault in j9shmem_handle_stat: Corrupt handle=0x2 #15408

Closed

babsingh force-pushed the loom_support branch from df0d5c2 to 4f10765 Compare June 23, 2022 14:11

babsingh changed the title ~~Loom Support: Pinning + Virtual Thread Mounted Flag~~ Loom Support: Pin Counters Jun 23, 2022

gacholio approved these changes Jun 23, 2022

View reviewed changes

gacholio merged commit 58c648d into eclipse-openj9:master Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loom Support: Pin Counters #15383

Loom Support: Pin Counters #15383

babsingh commented Jun 21, 2022 •

edited

Loading

tajila Jun 21, 2022 •

edited

Loading

tajila Jun 21, 2022

babsingh Jun 21, 2022 •

edited

Loading

babsingh Jun 21, 2022 •

edited

Loading

tajila Jun 21, 2022

tajila Jun 21, 2022

tajila Jun 21, 2022

babsingh Jun 21, 2022 •

edited

Loading

babsingh Jun 21, 2022

tajila Jun 22, 2022

tajila commented Jun 22, 2022

gacholio commented Jun 22, 2022

gacholio commented Jun 22, 2022

babsingh commented Jun 22, 2022

babsingh commented Jun 22, 2022

tajila commented Jun 22, 2022

gacholio commented Jun 22, 2022

gacholio commented Jun 22, 2022

tajila commented Jun 22, 2022

tajila commented Jun 22, 2022

babsingh commented Jun 22, 2022

gacholio commented Jun 23, 2022

0xdaryl commented Jun 23, 2022

babsingh commented Jun 23, 2022

gacholio left a comment

Loom Support: Pin Counters #15383

Loom Support: Pin Counters #15383

Conversation

babsingh commented Jun 21, 2022 • edited Loading

tajila Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

tajila Jun 21, 2022

Choose a reason for hiding this comment

babsingh Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

babsingh Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

tajila Jun 21, 2022

Choose a reason for hiding this comment

tajila Jun 21, 2022

Choose a reason for hiding this comment

tajila Jun 21, 2022

Choose a reason for hiding this comment

babsingh Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

babsingh Jun 21, 2022

Choose a reason for hiding this comment

tajila Jun 22, 2022

Choose a reason for hiding this comment

tajila commented Jun 22, 2022

gacholio commented Jun 22, 2022

gacholio commented Jun 22, 2022

babsingh commented Jun 22, 2022

babsingh commented Jun 22, 2022

tajila commented Jun 22, 2022

gacholio commented Jun 22, 2022

gacholio commented Jun 22, 2022

tajila commented Jun 22, 2022

tajila commented Jun 22, 2022

babsingh commented Jun 22, 2022

gacholio commented Jun 23, 2022

0xdaryl commented Jun 23, 2022

babsingh commented Jun 23, 2022

gacholio left a comment

Choose a reason for hiding this comment

babsingh commented Jun 21, 2022 •

edited

Loading

tajila Jun 21, 2022 •

edited

Loading

babsingh Jun 21, 2022 •

edited

Loading

babsingh Jun 21, 2022 •

edited

Loading

babsingh Jun 21, 2022 •

edited

Loading