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

Add property for directly loading libraries #565

Merged
merged 5 commits into from
Apr 13, 2022

Conversation

HGuillemet
Copy link
Contributor

Add a property (librariesAreProvided, feel free to suggest a better name) that allows to skip entirely library search in both resources and paths, and directly System.load them.
It's meant to be used when launching applications shipped with a Java runtime providing the native libraries and not including the native jars/modules.

Instead of a boolean property, we could arrange to allow specifying which libraries are provided, for instance with -DprovidedLibraries=opencv_core,opencv_imgproc but It probably doesn't worth the effort.

With the addition of this property, I'm not sur the newly added property cacheLibraries still has a use case. We could remove it.

This PR also include a small fix to shorten the codepath when defining styles and styles2.

@HGuillemet HGuillemet changed the title Add property for directly load libraries Add property for directly loading libraries Apr 2, 2022
@HGuillemet
Copy link
Contributor Author

An alternative that saves the addition of a new property is to check for the presence of the library to load in the directory of runtime libraries before searching in the resources and other paths, and if it is present, System.load it.
However the only way I know to determine this directory is through system property sun.boot.library.path and it is probably only supported on openjdk-based JVM, and maybe GraalVM.

@saudet
Copy link
Member

saudet commented Apr 2, 2022

Could you explain the purpose of this? Is it just to save a couple of ms at launch time? Do you have numbers to back your proposal?

@HGuillemet
Copy link
Contributor Author

It saves the search of resources we know they do not exist , indeed, but it's not the main reason of this PR:
after the search fails, Loader searchs paths in the system (for instance on linux, for a preloaded openblas: /lib64/, /lib/, /usr/lib64/, /usr/lib/, /opt/intel/oneapi/mkl/latest/lib/intel64/, /opt/intel/oneapi/compiler/latest/linux/compiler/lib/intel64_lin/) and, if the library is found there, create a symlink in the cache and link with it.
In our use case we don't want to link with these system-dependent version of the lib, but with the one installed in the runtime, and we don't want a cache be created in the used home directory either.
The cacheLibraries property prevents the cache creation, but not the search and link.

@saudet
Copy link
Member

saudet commented Apr 3, 2022

When the cache is disabled for libraries, no links are created, so I still don't understand what we get from disabling the search.

@HGuillemet
Copy link
Contributor Author

HGuillemet commented Apr 3, 2022

Since my english seems deficient, here are some command lines:

$ cd /home/java/javafx-sample/build/image/bin
$ ./java  -Dorg.bytedeco.javacpp.cacheLibraries=false -m org.bytedeco.javafxsample/org.bytedeco.javafxsample.HelloApplication &
[1] 3704
grep opencv /proc/3704/maps
...
7fe6f57f7000-7fe6f58bd000 r--p 00354000 103:02 2747494                   /usr/lib64/libopencv_core.so
...
$ ./java Dorg.bytedeco.javacpp.librariesAreProvided  -m org.bytedeco.javafxsample/org.bytedeco.javafxsample.HelloApplication &
[1] 3800
grep opencv /proc/3800/maps
...
7f526b9c2000-7f526ba19000 r--p 00000000 103:04 3605252                   /home/java/javafx-sample/build/image/lib/libopencv_core.so.405

In other words, and if I understand Loader correctly, the library search order is:

  1. resources
  2. various system paths
  3. sun.boot.library.path (via System.load fallback)
  4. java.library.path (via System.load.fallback)

This PR suggests to change the precedence of 3) so that it becomes prioritary.
The patch does this than removing steps 1) et 2) completly, since when we ship the libraries in the runtime, we probably don't need them, and the patch is quite simple.
The second post suggests to change the order in all cases:

  1. sun.boot.library.path
  2. resources
  3. various system paths
  4. java.library.path

This is probably more satisfactory. It doesn't require to set a property, and can work for mixed cases where some libraries are in the runtime, and some are not. But needs some more lines of codes, and the use of sun.boot.library.path property.

@saudet
Copy link
Member

saudet commented Apr 3, 2022

That's what the "org.bytedeco.javacpp.pathsFirst" system property is for, so I guess we should add "sun.boot.library.path" before "java.library.path"? Let me try to do that

saudet added a commit that referenced this pull request Apr 4, 2022
… for jlink (pull #565)

 * Add `__int8`, `__int16`, `__int32`, and `__int64` to `InfoMap` as "basic/types" to support combinations allowed by Visual Studio
@saudet
Copy link
Member

saudet commented Apr 4, 2022

I've added that in commit 213d621 and it seems to work fine for me. Please give it a try and let me know what you think!

@HGuillemet
Copy link
Contributor Author

HGuillemet commented Apr 5, 2022

What I understand to your commit is that, when pathsFirst is true, sun.boot.library.path is search in addition and before java.library.path (mimicing System.load). But still it's after the other paths (/lib64/, /lib/, /usr/lib64/, /usr/lib/, /opt/intel/oneapi/mkl/latest/lib/intel64/, /opt/intel/oneapi/compiler/latest/linux/compiler/lib/intel64_lin/ in my case).

So now without pathsFirst we still have this order:

  1. resources
  2. various system paths
  3. sun.boot.library.path (via System.load fallback)
  4. java.library.path (via System.load fallback)

and with pathFirst it becomes:

  1. various system paths
  2. sun.boot.library.path
  3. java.library.path
  4. resources

Right ?

This behaviour is logical for a property called pathFirst, but doesn't solve the problem of giving priority to sun.boot.library.path before other paths.
What about downgrading "various system paths" after java.library.path ?
If the user didn't set java.library.path, then it's value probably doesn't add anything compared to "various system path".
If the user did set it, shouldn't we honor his choice and give it priority over other paths ?

@saudet
Copy link
Member

saudet commented Apr 6, 2022

What are the "system paths" that you are referring to? Those get added to "java.library.path" by default, so if we search "sun.boot.library.path" before "java.library.path", then the "system paths" get searched after "sun.boot.library.path" because they are part of "java.library.path", which comes after, regardless of the value of pathFirst.

@HGuillemet
Copy link
Contributor Author

The content of platform.linkpath and platform.preloadpath properties. sun.boot.library.path and java.library.path are added after them in the list of paths searched for in findLibrary.

@HGuillemet
Copy link
Contributor Author

HGuillemet commented Apr 6, 2022

BTW, I'm currently preparing a MacOSX app for distribution and I'm realizing that, when an app is signed (which is more or less mandatory now), the executable cannot dynamically link with a library unless it is signed by the same team or by Apple.
This security feature could be removed explictely (see this entitlement) but then the notarization or execution of the app could be blocked.

It conforts me in the idea that extracting the libs in the runtime (before signature) is the way to go (and that disabling loading from resources or non-standard path could be disabled like proposed first in this PR, but I guess there are situations where we must link with library installed by the user that we cannot bundle, and thus we need platform.preloadpath)

@saudet
Copy link
Member

saudet commented Apr 6, 2022

There's the preload path, yes, but that's looked up after bundled resources, because it's also used at build time to bundle libraries. That makes it possible for users to leave out bundled resources and have everything loaded from the system, yes. However, for applications like yours, with pathsFirst, the preload path is searched last, so unless the libraries are actually missing from the library paths, it's not going to try and load from there.

@HGuillemet
Copy link
Contributor Author

However, for applications like yours, with pathsFirst, the preload path is searched last, so unless the libraries are actually missing from the library paths, it's not going to try and load from there.

Are you sure about this ?
What I understand from

if (libpath.length() > 0 && (pathsFirst || !isLoadLibraries() || reference)) {
// leave loading from "java.library.path" to System.loadLibrary() as fallback,
// which works better on Android, unless the user wants to rename a library
paths.addAll(Arrays.asList(libpath.split(File.pathSeparator)));
}

is that linkpath and preloadpath are searched before sub.boot.library.path and java.library.path

@saudet
Copy link
Member

saudet commented Apr 7, 2022

Hum, you're right. I think that should be changed/fixed because those paths are pretty much hard coded from an end-user perspective, so there's no point in always forcing them.

@saudet
Copy link
Member

saudet commented Apr 10, 2022

After looking into this some more, we can't move the library paths before the preload ones by default, because at build time, it would make it too easy to pick incorrectly libraries from the system instead of the ones in the build directories. I think your idea of a flag to disable searching altogether is probably more desirable than another setting to tamper further with the search order. I've pushed a couple of adjustments however. Could you give that a try and let me know what you think?

@HGuillemet
Copy link
Contributor Author

Thank you. I'll will try it out.
There is still the alternative to move sun.boot.library.path only before the other paths. I don't think there are situations where you don't want to give priority to sun.boot.library.path, and we will not need the definition and usage of a new property.

However, people may want to extract the libs in a directory which is not the runtime library directory (to avoid to mess with it) and point the JVM to this directory using java.library.path. In this case the new property is necessary. Unless maybe if they use org.bytedeco.javacpp.cachedir instead of java.library.path.

@saudet
Copy link
Member

saudet commented Apr 11, 2022

I don't think that's a good idea. On most non-Windows platforms, libraries have versions, and even when a library has the same name and version as another already loaded library, it is still possible to load both in the same process, as long as the symbols are not loaded globally, which is what System.load() and System.loadLibrary() do, so we really don't want to pick libraries from the system without a way to override that behavior.

@HGuillemet
Copy link
Contributor Author

After this fix, the new property works as expected.

@saudet saudet merged commit 936b4e0 into bytedeco:master Apr 13, 2022
@HGuillemet HGuillemet deleted the hg_librariesAreProvided branch April 13, 2022 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants