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.

Sign up for GitHub

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

Jump to bottom

[Loader] improve performance of resource extraction and library search #512

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Loader] improve performance of resource extraction and library search #512

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 33 additions & 18 deletions src/main/java/org/bytedeco/javacpp/Loader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,15 @@
import java.util.Enumeration;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.Set;
import java.util.WeakHashMap;
import java.util.jar.JarEntry;
import java.util.jar.JarFile;

import org.bytedeco.javacpp.annotation.Cast;
import org.bytedeco.javacpp.annotation.Name;
import org.bytedeco.javacpp.annotation.Platform;
Expand Down Expand Up @@ -765,15 +768,16 @@ public static File extractResource(URL resourceURL, File directoryOrFile,
File file = new File(directoryOrFile, entryName.substring(jarEntryName.length()));
if (entry.isDirectory()) {
file.mkdirs();
file.setLastModified(entryTimestamp);
} else if (!cacheDirectory || !file.exists() || file.length() != entrySize
|| file.lastModified() != entryTimestamp || !file.equals(file.getCanonicalFile())) {
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure what the intention of the comparison with the canonical file is, but the file is not canonicalized in the called extractResource() method. Furthermore canonicalization does not (or at least should not) change the effective target the file is pointing to, so the metadata checked before should not change because of that.

Copy link
Member

Choose a reason for hiding this comment

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

Symbolic links get created and we need to resolve them to prevent creating cycles.

Copy link
Author

Choose a reason for hiding this comment

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

Can you please explain how this prevents cycles or is there an example anywhere? Because I don't understand it.
I'm asking because avoiding the canonicalization had a significant impact on the runtime improvement (around one third of the runtime reduction).
So if we can avoid this call or at least have a cheaper pre-check (e.g. canCreateSymbolicLink==true? or Files.isSymbolicLink()==true, I'm not sure if this is much faster), it should make it faster.
But of course the logic has to be correct in any case.

Copy link
Member

Choose a reason for hiding this comment

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

There can be symbolic links anywhere in the path, not just the last file in the path.

I'm pretty sure we can disable those checks when org.bytedeco.javacpp.cachedir.nosubdir is set though, and I think that's what you're already using, right? If so, I'd be happy to skip all that when that flag is set. How does that sound?

|| file.lastModified() != entryTimestamp) {
// ... extract it from our resources ...
file.delete();
String s = resourceURL.toString();
URL u = new URL(s.substring(0, s.indexOf("!/") + 2) + entryName);
file = extractResource(u, file, prefix, suffix);
file.setLastModified(entryTimestamp);
}
file.setLastModified(entryTimestamp);
}
}
return directoryOrFile;
Expand Down Expand Up @@ -1247,7 +1251,7 @@ public static String load(Class cls, Properties properties, boolean pathsFirst,
foundLibraries.put(preload, urls = findLibrary(cls, p, preload, pathsFirst));
}
String filename = null;
if (oldUrls == null || urls.length > 0) {
if (oldUrls == null && urls.length > 0) {
Copy link
Author

@HannesWell HannesWell Aug 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure the 'or' was intentional or not, but before it called the following loadLibrary method when

  1. oldURLs==null and urls.length>0 , so the library in question was not searched before and found now.
  2. oldURLs==null and urls.length==0, e.g. the library in question was not searched before but not found now.
  3. oldURLs!=null and urls.length>0, e.g. the library in question was searched before but and found.

For me it does not make sense to attempt to load the library when it was not found (case 2) or was already loaded (case 3). Or did I oversee something?

Copy link
Member

Choose a reason for hiding this comment

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

If a previously found library failed to load, we should try to keep finding other versions that might be able to load.

filename = loadLibrary(cls, urls, preload, preloaded.toArray(new String[preloaded.size()]));
}
if (filename != null && new File(filename).exists()) {
Expand Down Expand Up @@ -1341,7 +1345,7 @@ public static String load(Class cls, Properties properties, boolean pathsFirst,
foundLibraries.put(library, urls = findLibrary(cls, p, library, pathsFirst));
}
String filename = null;
if (oldUrls == null || urls.length > 0) {
if (oldUrls == null && urls.length > 0) {
filename = loadLibrary(cls, urls, library, preloaded.toArray(new String[preloaded.size()]));
}
if (cacheDir != null && filename != null && filename.startsWith(cacheDir)) {
Expand Down Expand Up @@ -1423,28 +1427,37 @@ public static URL[] findLibrary(Class cls, ClassProperties properties, String li
String[] extensions = properties.get("platform.extension").toArray(new String[0]);
String prefix = properties.getProperty("platform.library.prefix", "");
String suffix = properties.getProperty("platform.library.suffix", "");
String[] styles = {
String[] styles = !version.isEmpty() ? new String[] {
prefix + libname + suffix + version, // Linux style
prefix + libname + version + suffix, // Mac OS X style
prefix + libname + suffix // without version
};
String[] styles2 = {
} : new String[] { prefix + libname + suffix };

String[] styles2 = !version2.isEmpty() ? new String[] {
prefix + libname2 + suffix + version2, // Linux style
prefix + libname2 + version2 + suffix, // Mac OS X style
prefix + libname2 + suffix // without version
};
} : new String[] { prefix + libname2 + suffix };

String[] suffixes = properties.get("platform.library.suffix").toArray(new String[0]);
if (suffixes.length > 1) {
styles = new String[3 * suffixes.length];
styles2 = new String[3 * suffixes.length];
styles = !version.isEmpty() ? new String[3 * suffixes.length] : new String[suffixes.length];
styles2 = !version2.isEmpty() ? new String[3 * suffixes.length] : new String[suffixes.length];
for (int i = 0; i < suffixes.length; i++) {
styles[3 * i ] = prefix + libname + suffixes[i] + version; // Linux style
styles[3 * i + 1] = prefix + libname + version + suffixes[i]; // Mac OS X style
styles[3 * i + 2] = prefix + libname + suffixes[i]; // without version
styles2[3 * i ] = prefix + libname2 + suffixes[i] + version2; // Linux style
styles2[3 * i + 1] = prefix + libname2 + version2 + suffixes[i]; // Mac OS X style
styles2[3 * i + 2] = prefix + libname2 + suffixes[i]; // without version
if(!version.isEmpty()) {
styles[3 * i ] = prefix + libname + suffixes[i] + version; // Linux style
styles[3 * i + 1] = prefix + libname + version + suffixes[i]; // Mac OS X style
styles[3 * i + 2] = prefix + libname + suffixes[i]; // without version
}else {
styles[i] = prefix + libname + suffixes[i]; // without version
}
if (!version2.isEmpty()) {
styles2[3 * i ] = prefix + libname2 + suffixes[i] + version2; // Linux style
styles2[3 * i + 1] = prefix + libname2 + version2 + suffixes[i]; // Mac OS X style
styles2[3 * i + 2] = prefix + libname2 + suffixes[i]; // without version
}else {
styles2[i] = prefix + libname2 + suffixes[i]; // without version
}
}
}
if (nostyle) {
Expand All @@ -1455,13 +1468,15 @@ public static URL[] findLibrary(Class cls, ClassProperties properties, String li
List<String> paths = new ArrayList<String>();
paths.addAll(properties.get("platform.linkpath"));
paths.addAll(properties.get("platform.preloadpath"));
List<String> resources = properties.get("platform.preloadresource");
List<String> resourcesList = properties.get("platform.preloadresource");
String libraryPath = properties.getProperty("platform.library.path", "");
Set<String> resources = new LinkedHashSet<String>();
if (libraryPath.length() > 0 && pathsFirst) {
// leave loading from "platform.library.path" to System.loadLibrary() as fallback,
// which works better on Android, unless the user wants to run an executable
resources.add(0, libraryPath);
resources.add(libraryPath);
}
resources.addAll(resourcesList);
resources.add(null);
String libpath = System.getProperty("java.library.path", "");
if (libpath.length() > 0 && (pathsFirst || !isLoadLibraries() || reference)) {
Expand Down