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

Atomic-free JNI work #1762

Merged
merged 1 commit into from
Apr 25, 2018
Merged

Atomic-free JNI work #1762

merged 1 commit into from
Apr 25, 2018

Conversation

gacholio
Copy link
Contributor

  • re-enable JNI critical for atomic-free
  • fix JNI check bug related to critical

Signed-off-by: Graham Chapman graham_chapman@ca.ibm.com

- re-enable JNI critical for atomic-free
- fix JNI check bug related to critical

Signed-off-by: Graham Chapman <graham_chapman@ca.ibm.com>
@charliegracie
Copy link
Contributor

jenkins test sanity zlinux jdk8

@charliegracie
Copy link
Contributor

jenkins test sanity plinux jdk9

@@ -36,6 +36,10 @@
void
MM_JNICriticalRegion::reacquireAccess(J9VMThread* vmThread, UDATA accessMask)
{
#if defined(J9VM_INTERP_ATOMIC_FREE_JNI)
// Current thread must have entered the VM before acquiring exclusive
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if these asserts could be enabled all of the time? Not asking you to change them just thinking out loud.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flag has no meaning unless the ifdef is true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry thought that was a VMAccess check lol. My bad!

hasVMAccess = true;
#else /* J9VM_INTERP_ATOMIC_FREE_JNI */
MM_JNICriticalRegion::enterCriticalRegion(vmThread);
if (!hasVMAccess) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also just making a note for myself to come look at the hasVMAccess. Not sure how one thread entering here could have it and another thread not have it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this code can be refactored/optimized at some point.

Copy link
Contributor

@charliegracie charliegracie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just waiting for the CI builds to finish

@charliegracie charliegracie merged commit 17ad44a into eclipse-openj9:master Apr 25, 2018
@pshipton pshipton mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants