Improve/Fix direct mapping support #919
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
twall
reviewed
Feb 5, 2018
| } | ||
| } | ||
| } | ||
| final Map<Class,PrimitiveConverter> convertes = new HashMap<Class,PrimitiveConverter>(); |
There was a problem hiding this comment.
Typo in convertes => converters
For direct mapped methods the java value is directly passed to the C side. For wrappers this means, that the object pointer is passed and not the primitive values. This changeset removes the mappings for the Wrapper classes and makes it possible to use a type converter to make the call possible, at the cost of higher calling costs. Closes: java-native-access#918
…M crash When a IntergetType based type is defined in a direct mapping and a NullPointer is passed to that call, a method call is tried on that NullPointer via JNI, resulting in a JVM crash. This changeset replaces the value with "0" if a NULL pointer is passed and adds unittests for the possible calling conventions. Closes: java-native-access#905
matthiasblaesing
force-pushed
the
directmapping
branch
from
e57d359
to
6af211f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changeset intends to to improve the stability of the direct mapping support.
For the implementation itself only the first two commits are relevant, as they cover
the changes, the other changes are follow-up work (rebuilding binaries) and
cleanups.
To summarize, there are two main issues:
the direct mapping causes errors in the code flow if primitive wrappers are used as parameters. From my analyses the problem is, that the JVM passes the values directly to the native code. That means the native side can get primitives and wrapper classes. For the interface bound code paths via the normal dispatch, all parameters are passed as objects and unwrapped as necessary, this is not possible in the direct path. Instead of trying to add the unwrapping information, I removed the wrapper mappings from the defaults and added a sample to the unittests where TypeMapped types are used to emulated Wrapper classes.
Given that direct mapping is used to gain performance I think this is acceptable. (first commit)
if a NULL value is passed to a mapping that expects an IntegerType, this leads to JVM crash - this is fixed by replacing the value with a primitive 0 (second commit)
@twall could you have a look please?