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

JEP-370 Implementation (Part 3) #8501

Merged
merged 5 commits into from
Mar 1, 2020

Conversation

babsingh
Copy link
Contributor

@babsingh babsingh commented Feb 4, 2020

Closes: #8292.

1. Use OpenJDK ViewHandles in Java14+ to support JEP-370

OpenJDK implementation of ByteArrayViewHandle and ByteBufferViewHandle
is used to support JEP-370 in Java14+.

OpenJ9 ViewHandles don't handle memory scopes properly. Example:

try (MemorySegment segment = MemorySegment.allocateNative(bytes)) {
   ByteBuffer byteBuffer = segment.asByteBuffer();
   MethodHandle handle = Derived from ByteBuffer & ByteBufferViewHandle;
   handle.invoke();
}

Invoking handle.invoke() outside the memory scope should result in
IllegalStateException, which is thrown from jdk.internal.foreign.MemoryScope.

With OpenJ9 ViewHandles, the IllegalStateException is not thrown. The
correct behavior is achieved by using OpenJDK ViewHandles.

The deficiency in OpenJ9 ViewHandles could not be identified.

2. Handle NULL VarHandle.handleTable entries in VarHandle.accessModeType

3. Handle NULL VarHandle.handleTable entries in (Bytecode|MH)Interpreter

4. Handle NULL VarHandle.handleTable entries in VarHandle.toMethodHandle

5. Support isAccessModeSupportedHelper method with VarForm constructor

OpenJ9 ViewVarHandles have their own implementation for the
isAccessModeSupported method. OpenJDK ViewVarHandles are used in OpenJ9
Java 14+ which rely upon VarHandle.isAccessModeSupported instead of
having their own isAccessModeSupported implementation.

VarHandle.isAccessModeSupported is updated to support OpenJDK
VarHandles, which rely upon OpenJ9's VarHandle(VarForm varForm)
constructor.

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

@babsingh
Copy link
Contributor Author

babsingh commented Feb 4, 2020

Depends on #8330, #8414 and #8460. Will require a rebase after dependents are merged.

@babsingh babsingh force-pushed the jep370_impl4 branch 2 times, most recently from 21a245c to d721d80 Compare February 5, 2020 00:56
@babsingh babsingh marked this pull request as ready for review February 5, 2020 01:08
@babsingh
Copy link
Contributor Author

babsingh commented Feb 13, 2020

@DanHeidinga Rebased this pull request since #8414 is merged (#8414 (comment)).

@DanHeidinga
Copy link
Member

Jenkins test sanity plinux jdk11,jdk14

@pshipton
Copy link
Member

jenkins compile win jdk8,jdk14

@pshipton
Copy link
Member

Pls rebase to pick up #8583

@babsingh
Copy link
Contributor Author

Rebased. @AdamBrousseau do we apply the pull request changes on top of openj9/master? are rebases required?

@AdamBrousseau
Copy link
Contributor

Rebases are not required. We use the merge ref of the PR which is a merge of the PR into master. That being said, GitHub has publicly stated these are an undocumented, unsupported feature of a PR and they encourage users to create the merge themselves. I believe it is because when something is merged to master, not every pr is updated automatically by default. Certain events need to happen on a PR in order for the ref to be refreshed.

@pshipton
Copy link
Member

jenkins compile win jdk8

@pshipton
Copy link
Member

@pshipton
Copy link
Member

Past PR testing has found that sometimes (often?) a rebase is required to pick up other changes. I could have tried it without the rebase, but I know the rebase will fix it.

@babsingh
Copy link
Contributor Author

babsingh commented Feb 14, 2020

https://ci.eclipse.org/openj9/job/PullRequest-OpenJ9/2729/: the plinux failure is a segfault in Jep359Tests_0, which is reported in #8582. This failure happens even without the changes of this pull request.

runtime/vm/MHInterpreter.cpp Outdated Show resolved Hide resolved
runtime/oti/vmconstantpool.xml Outdated Show resolved Hide resolved
OpenJDK implementation of ByteArrayViewHandle and ByteBufferViewHandle
is used to support JEP-370 in Java14+.

OpenJ9 ViewHandles don't handle memory scopes properly. Example:

try (MemorySegment segment = MemorySegment.allocateNative(bytes)) {
   ByteBuffer byteBuffer = segment.asByteBuffer();
   MethodHandle handle = Derived from ByteBuffer & ByteBufferViewHandle;
   handle.invoke();
}

Invoking handle.invoke() outside the memory scope should result in
IllegalStateException, which is thrown from
jdk.internal.foreign.MemoryScope.

With OpenJ9 ViewHandles, the IllegalStateException is not thrown. The
aforementioned behavior is achieved by using OpenJDK ViewHandles.

The deficiency in OpenJ9 ViewHandles could not be identified.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
In OpenJ9, derived VarHandle classes define all AccessMode operations,
and UnsupportedOperationException is thrown from the operation methods
that are not supported. All OpenJ9 VarHandle.handleTable entries are
non-null when supporting derived OpenJ9 VarHandle classes.

In OpenJDK, derived VarHandle classes do not define unsupported
AccessMode operations. Thus, OpenJ9 VarHandle.handleTable entries are
null for unsupported AccessMode operations when supporting derived
OpenJDK VarHandle classes.

VarHandle.accessModeType relies upon VarHandle.handleTable to derive the
AccessMode MethodType. When supporting derived OpenJDK VarHandle
classes, accessModeTypeUncached is used to retrieve the AccessMode
MethodType in case VarHandle.handleTable entries are null.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh babsingh force-pushed the jep370_impl4 branch 2 times, most recently from a894bae to 1f80a9f Compare February 18, 2020 21:00
@babsingh
Copy link
Contributor Author

Currently, investigating the failing extended tests in Test_openjdk14_j9_extended.functional_x86-64_windows_Personal-2:

05:45:44  FAILED test targets:
05:45:44  	VarHandle_0
05:45:44  	View-LE-OnHeap_0
05:45:44  	View-LE-OffHeap_0
05:45:44  	View-BE-OnHeap_0
05:45:44  	View-BE-OffHeap_0

@babsingh
Copy link
Contributor Author

babsingh commented Feb 26, 2020

The assertion failures in VarHandle_0 are addressed in the following commit:

Support isAccessModeSupportedHelper method with VarForm constructor

@babsingh
Copy link
Contributor Author

babsingh commented Feb 26, 2020

All failures seen in the extended functional test suite have been addressed.

Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

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

@gacholio Anything major I missed in the interpreter changes?

runtime/vm/BytecodeInterpreter.hpp Show resolved Hide resolved
runtime/vm/BytecodeInterpreter.hpp Outdated Show resolved Hide resolved
runtime/vm/BytecodeInterpreter.hpp Outdated Show resolved Hide resolved
runtime/vm/MHInterpreter.cpp Outdated Show resolved Hide resolved
@gacholio
Copy link
Contributor

@DanHeidinga I suggested one trivial change.

It might be better to move all of this code out of the interpreter - just do the update/call/hasbeenupdated in the interpreter itself.

@DanHeidinga
Copy link
Member

It might be better to move all of this code out of the interpreter - just do the update/call/hasbeenupdated in the interpreter itself.

@babsingh Can you update the code to move most of this out of the interpreter as GAC suggested?

@gacholio
Copy link
Contributor

You'll need to use the J9AllocateObject API instead of the inline allocate.

@babsingh
Copy link
Contributor Author

@gacholio @DanHeidinga any suggestions on where the new function should be added? VMHelpers.hpp?

@gacholio
Copy link
Contributor

exceptionsupport.cpp - the whole idea is that it not be inlined into the interpreter.

@babsingh
Copy link
Contributor Author

@DanHeidinga Moved code out of the interpreter as per @gacholio's suggestion. New code is added to the following commit:

Handle NULL VarHandle.handleTable entries in (Bytecode|MH)Interpreter

@gacholio
Copy link
Contributor

Yeah, literal means the direct string. If it's a #define then it can be used.

OpenJ9 VarHandle.handleTable entries are null for unsupported AccessMode
operations when supporting derived OpenJDK VarHandle classes.

VM_MHInterpreter::dispatchLoop and BytecodeInterpreter::invokevarhandle
look up entries in VarHandle.handleTable to invoke the operation method.
A null handleTable entry reflects a unsupported AccessMode operation. In
this case, an UnsupportedOperationException should be thrown.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
OpenJ9 VarHandle.handleTable entries are null for unsupported AccessMode
operations when supporting derived OpenJDK VarHandle classes.

When supporting derived OpenJDK VarHandle classes,
accessModeTypeUncached is used to retrieve the AccessMode MethodType in
case VarHandle.handleTable entries are null.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
OpenJ9 ViewVarHandles have their own implementation for the
isAccessModeSupported method. OpenJDK ViewVarHandles are used in OpenJ9
Java 14+ which rely upon VarHandle.isAccessModeSupported instead of
having their own isAccessModeSupported implementation. 

VarHandle.isAccessModeSupported is updated to support OpenJDK
VarHandles, which rely upon OpenJ9's VarHandle(VarForm varForm)
constructor.

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

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

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

lgtm

@DanHeidinga
Copy link
Member

Jenkins test sanity zlinux jdk14,jdk11

@DanHeidinga
Copy link
Member

Jenkins test extended win jdk14

@DanHeidinga
Copy link
Member

DanHeidinga commented Mar 1, 2020

@babsingh can you open a PR for this against the 0.19 release branch, assuming the PR builds will pass?

I've ported it in #8703

@DanHeidinga DanHeidinga merged commit dd6fe85 into eclipse-openj9:master Mar 1, 2020
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.

JEP-370 Foreign-Memory Access API
6 participants