Skip to content

Independent JNI#120

Merged
mkitti merged 12 commits intoJuliaInterop:masterfrom
mkitti:independent_JNI
May 14, 2020
Merged

Independent JNI#120
mkitti merged 12 commits intoJuliaInterop:masterfrom
mkitti:independent_JNI

Conversation

@mkitti
Copy link
Member

@mkitti mkitti commented May 6, 2020

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 mkitti requested a review from aviks May 6, 2020 06:47
@aviks
Copy link
Collaborator

aviks commented May 8, 2020

Did appveyor take out Java from their images?

@mkitti
Copy link
Member Author

mkitti commented May 9, 2020

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
@mkitti
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
@mkitti
Streamline appveyor
9449dfd
@mkitti
Deference penv in init_current_vm
f3a0b1f
@mkitti
Use GC preserve to retain options, clean up JNI references
93a2139
@mkitti
Making jnienv global helps x86 init
078278e
@mkitti
Copy link
Member Author

mkitti commented May 10, 2020

@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
Copy link
Member Author

mkitti commented May 12, 2020

@aviks I intend to merge this this week.

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

@aviks aviks May 10, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@mkitti mkitti May 12, 2020

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@aviks aviks May 12, 2020

Choose a reason for hiding this comment

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

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

mkitti
mkitti commented May 12, 2020
View reviewed changes
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[]
Copy link
Member Author

@mkitti mkitti May 12, 2020

Choose a reason for hiding this comment

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

@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
Copy link
Member Author

mkitti commented May 13, 2020

@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
Copy link
Member Author

mkitti commented May 14, 2020

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
Copy link
Member Author

mkitti commented May 14, 2020

Thanks for the review, @aviks . Merging now.

@mkitti mkitti merged commit e4e76d8 into JuliaInterop:master May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aviks 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

@mkitti @aviks