FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Independent JNI by mkitti · Pull Request #120 · JuliaInterop/JavaCall.jl · GitHub

/ JavaCall.jl Public

Independent JNI - #120

Merged
mkitti merged 12 commits into
JuliaInterop:masterfrom
mkitti:independent_JNI
May 14, 2020
ConversationCommits (12)ChecksFiles changed
Merged

Independent JNI#120
mkitti merged 12 commits into
JuliaInterop:masterfrom
mkitti:independent_JNI

Conversation

mkitti commented May 6, 2020

Copy link
Copy Markdown
Member

This is a follow up to creating the JNI module. Here we the JNI initialization code into JNI itself as well as the associated globals. Further those globals are now converted to type associated Ref. Additionally the Julia interface is also simplified since penv no longer needs to be passed

mkitti requested a review from aviks May 6, 2020 06:47

aviks commented May 8, 2020

Copy link
Copy Markdown
Collaborator

Did appveyor take out Java from their images?

mkitti commented May 9, 2020

Copy link
Copy Markdown
Member Author

Its my commit somehow. The master branch still passes when I rerun it.
https://ci.appveyor.com/project/aviks/javacall-jl-6c24s

mkitti added 5 commits May 9, 2020 11:39
Squashed commit of the following:
a8335a5 
commit 66c6c33
Author: markkitt@gmail.com <markkitt@gmail.com>
Date:   Sat May 9 01:38:52 2020 -0500

    Load multiple libraries in JNI if necessary

commit a459565
Author: markkitt@gmail.com <markkitt@gmail.com>
Date:   Sat May 9 00:26:39 2020 -0500

    Throw error from findjvm
Streamline appveyor
9449dfd
Deference penv in init_current_vm
f3a0b1f
Use GC preserve to retain options, clean up JNI references
93a2139
Making jnienv global helps x86 init
078278e

mkitti commented May 10, 2020

Copy link
Copy Markdown
Member Author

@aviks I fixed the appveyor issues.

Since I moved a lot of the low level functionality into the JNI submodule, I moved the Libdl import there as well. On Windows, mscrvt100.dll also needed to be loaded and that was causing the initial appveyor issues.

On further investigation into the appveyor configuration, I found that even when using the x86 platform, Julia was still trying to load the x64 JVM. By setting JAVA_HOME explicitly to the x86 JVM, and then fooling around with globals, I managed to get x86 init to work via 078278e. I don't know why that makes a difference.

mkitti commented May 12, 2020

Copy link
Copy Markdown
Member Author

@aviks I intend to merge this this week.

aviks reviewed May 12, 2020
View reviewed changes
Comment thread src/JNI.jl Outdated
@@ -19,6 +21,10 @@ include("jnienv.jl")
const jniref = Ref(JNINativeInterface())
global jnifunc

aviks May 10, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I haven't looked at this holistically yet, but whey do we need both jniref and jnifunc

mkitti May 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I kept jnifunc around mostly for backwards compatibility in case anyone else was accessing the global.

aviks May 12, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok cool. Maybe add a @deprecated_binding thing on it?

mkitti commented May 12, 2020
View reviewed changes
Comment thread src/JNI.jl Outdated
jnienv = unsafe_load(penv)
global jnienv = unsafe_load(penv)
jniref[] = unsafe_load(jnienv.JNINativeInterface_) #The JNI Function table
global jnifunc = jniref[]

mkitti May 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@aviks jnifunc is set here.

I also found that storing the jnienv as a global somehow makes x86 Windows work more smoothly. With that we have successful JavaCall.init() on x86 Windows. I wanted to see if I could understand why that makes a difference before I start cleaning up the globals.

mkitti commented May 13, 2020

Copy link
Copy Markdown
Member Author

@aviks The problem with Base.@deprecated_binding is that it declares a const at compile time for jnifunc which will be invalid. We could try to update it at runtime, but then this issues a warning about redefining a const.

I've left the deprecation warning, but trying to use jnifunc will fail now.

mkitti commented May 14, 2020

Copy link
Copy Markdown
Member Author

To summarize the net changes:

  1. Library loading and core JVM initialization, and destroy are moved into the JNI module. This means __init__() no longer loads the dynamic library. This is entirely done on init()
  2. penv is no longer a global in JavaCall and passed by default to ccall. All JNI calls from JavaCall do not require a JNIEnv pointer to be passed since that is handled entirely within JNI module now. penv can still be passed in as a named argument.
  3. JavaCall.findjvm() just returns a tuple of Strings describing the library or libraries to load rather than setting a global.
  4. Tests no longer use ccall directly but now refer to the wrapped versions in the JNI module.

As a side effect and for unknown reasons, JavaCall.init() and some functions now work on x86 Windows. This is somehow related to storing jnienv as a global.

mkitti commented May 14, 2020

Copy link
Copy Markdown
Member Author

Thanks for the review, @aviks . Merging now.

mkitti merged commit e4e76d8 into JuliaInterop:master May 14, 2020
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aviks aviks approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL