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

ScopedMemoryAccess closeScope0 synchronization #19412

Merged
merged 1 commit into from
May 1, 2024

Conversation

babsingh
Copy link
Contributor

There are gaps where async exceptions are not processed in time (e.g.
JIT compiled code in a loop). Threads will wait in closeScope0 until
J9VMThread->scopedError (async exception) is transferred to
J9VMThread->currentException. The wait prevents a MemorySession to be
closed until no more operations are being performed on it.

Related: #13211

@babsingh babsingh marked this pull request as draft April 29, 2024 21:05
@babsingh babsingh requested review from tajila and gacholio April 29, 2024 21:05
@babsingh babsingh force-pushed the main1 branch 2 times, most recently from 53bb636 to f65a081 Compare April 29, 2024 22:27
@babsingh babsingh marked this pull request as ready for review April 29, 2024 23:03
@tajila
Copy link
Contributor

tajila commented Apr 30, 2024

jenkins test sanity plinux jdk22

@tajila
Copy link
Contributor

tajila commented Apr 30, 2024

jenkins test sanity alinux jdk21

@gacholio
Copy link
Contributor

The use of atomics seems unnecessary here, as you have a mutex. I'd also make the counter UDATA instead of U_64 (even though that makes no practical difference in this JDK level).

There are gaps where async exceptions are not processed in time (e.g.
JIT compiled code in a loop). Threads will wait in closeScope0 until
J9VMThread->scopedError (async exception) is transferred to
J9VMThread->currentException. The wait prevents a MemorySession to be
closed until no more operations are being performed on it.

Related: eclipse-openj9#13211

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh
Copy link
Contributor Author

The use of atomics seems unnecessary here, as you have a mutex. I'd also make the counter UDATA instead of U_64 (even though that makes no practical difference in this JDK level).

@gacholio Updated.

if (0 != vm->closeScopeNotifyCount) { // <--- Can this read happen outside monitor enter/exit?
	omrthread_monitor_enter(vm->closeScopeMutex);
	while (0 != vm->closeScopeNotifyCount) {
		omrthread_monitor_wait(vm->closeScopeMutex);
	}
	omrthread_monitor_exit(vm->closeScopeMutex);
}

@gacholio
Copy link
Contributor

gacholio commented May 1, 2024

I'm also wondering how this global counter will work with multiple concurrent scope closures - we probably need to associate the count with the scope, either via internal hash table, or by adding a hidden field to the scope object.

@babsingh
Copy link
Contributor Author

babsingh commented May 1, 2024

I'm also wondering how this global counter will work with multiple concurrent scope closures - we probably need to associate the count with the scope, either via internal hash table, or by adding a hidden field to the scope object.

  • Only one thread will invoke closeScope0 (native) per SharedMemorySession. There is a CAS in SharedSession.java#L79-L87, which prevents multiple threads to invoke closeScope0 per MemorySession.
  • At a time, a thread can only operate on max. two MemorySessions; see Ops-1 and Ops-2 for the various SharedMemorySession operations (i.e. methods with the @Scoped annotation).
  • The common ScopedAccessError thrown for closing a MemorySession should close all the MemorySessions that need to be closed; the Java code wraps the MemorySession operations with the appropriate try/catch block.
  • In our closeScope0 impl, if multiple MemorySessions are being closed concurrently and J9VMThread->scopedError is already set, we skip setting scopedError (see ScopedMemoryAccess.cpp#L77-L80) because operations on all scopes should stop in a thread once the thread throws J9VMThread->scopedError. Due to this approach, we don't need to track counts per MemorySession.
  • The global counter tracks the number of threads that still need to throw the scopedError. The goal is to wait in closeScope0 until all scopedErrors are transferred to currentException.

@gacholio
Copy link
Contributor

gacholio commented May 1, 2024

jenkins test sanity plinux jdk22

@gacholio
Copy link
Contributor

gacholio commented May 1, 2024

jenkins test sanity alinux jdk21

@gacholio
Copy link
Contributor

gacholio commented May 1, 2024

  • closeScope0 per MemorySession.

Does this mean the native can be invoked on multiple threads (say each thread has its own session)?

@gacholio
Copy link
Contributor

gacholio commented May 1, 2024

Nevermind, I see you've answered this.

@gacholio
Copy link
Contributor

gacholio commented May 1, 2024

PPC is down - this has been sufficiently tested.

@gacholio gacholio merged commit 7e54abb into eclipse-openj9:master May 1, 2024
5 of 6 checks passed
babsingh added a commit to babsingh/aqa-tests that referenced this pull request May 2, 2024
TestHandshake has been fixed by

- eclipse-openj9/openj9#19167
- eclipse-openj9/openj9#19412

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/aqa-tests that referenced this pull request May 2, 2024
TestHandshake has been fixed by

- eclipse-openj9/openj9#19167
- eclipse-openj9/openj9#19412

Related: eclipse-openj9/openj9#13211

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/aqa-tests that referenced this pull request May 2, 2024
TestHandshake has been fixed by

- eclipse-openj9/openj9#19167
- eclipse-openj9/openj9#19412

Related: eclipse-openj9/openj9#13211

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
pshipton pushed a commit to adoptium/aqa-tests that referenced this pull request May 2, 2024
TestHandshake has been fixed by

- eclipse-openj9/openj9#19167
- eclipse-openj9/openj9#19412

Related: eclipse-openj9/openj9#13211

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/aqa-tests that referenced this pull request May 2, 2024
TestHandshake has been fixed by

- eclipse-openj9/openj9#19167
- eclipse-openj9/openj9#19412

Related: eclipse-openj9/openj9#13211

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
pshipton pushed a commit to adoptium/aqa-tests that referenced this pull request May 3, 2024
TestHandshake has been fixed by

- eclipse-openj9/openj9#19167
- eclipse-openj9/openj9#19412

Related: eclipse-openj9/openj9#13211

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants