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

Improve native library loading #34

Merged
merged 14 commits into from
Dec 28, 2022
Merged

Improve native library loading #34

merged 14 commits into from
Dec 28, 2022

Conversation

Glavo
Copy link
Contributor

@Glavo Glavo commented Dec 16, 2022

  • Build jmod files for the platform artifact (Jigsaw support? provided as jmod & could be used for jlink to build os specific runtime #2);
  • Basic cross compilation support;
  • Detect more architectures and try to load native libraries;
  • Load native libraries from more places.
    • When the system property skija.loadFromLibraryPath is set to true, use System.loadLibrary(String) to load the native library;
    • When the system property skija.library.path is set, load the native library from this path;
    • If skija is bundled in JRE through jlink, try to load the native library through System.loadLibrary(String) (used with platform jmod files);
    • Finally, fallback to the original loading mode.

Incompatible change:

  • Module io.github.humbleui.skija.windows is renamed to io.github.humbleui.skija.windows.x64;
  • Module io.github.humbleui.skija.linux is renamed to io.github.humbleui.skija.linux.x64;
  • All LibraryFinders are deprecated. Now I use ClassLoader to find resources;
  • The module-info of the platform modules no longer declares requires on the shared module, which helps to solve some edge conditions (when module path and class path are mixed);
  • io.github.humbleui.skija.impl.Platform is rewritten as a class to better support more os and architectures.

This PR introduces large changes and may require more testing before being merged.

@Glavo
Copy link
Contributor Author

Glavo commented Dec 16, 2022

Purpose of this PR:

  • The platform jmod can place the native library in the corresponding path in the JRE when use jlink, instead of being decompressed as a resource at runtime;
  • Detect more platforms. Even if we do not support them on the main line, users can easily adapt these platforms by themselves in a non-invasive way;
  • No longer assume that the linux/windows system only works on the x86-64 platform, in preparation for supporting ARM;
  • The simplest way to support ARM platform may be through cross compilation, so we should make some preparations for this.

@tonsky
Copy link
Contributor

tonsky commented Dec 23, 2022

Thanks for the PR! Adding _arch everywhere is a good call, I was meaning to do it myself eventually.

Lots of good stuff, but hard to accept all at once. It worked for me on Mac M1 and on Ubuntu x64. My windows toolchain is currently broken (not just on this PR, in general), so I can’t check yet

Some notes/questions I have:

  • Probably extract OperatingSystem/Architecture from Platform and remove Platform. I think these two classes will be enough
  • skija.version is being put into platform jars (as it should) but loaded using classloader of shared jar. IIRC it doesn’t worked for me with Java 9 modules, that’s why I had to create a class in each platform
  • one of the goals of having arch classifier in e.g. libskija_x64.dylib was to make sure I can include all possible skija jars at the same time and still get the right one loaded. I think by removing _x64/_arm64 you broke that? Now if I add both jars to class path there’ll be two identical resources which might be tricky to distinguish
  • “The module-info of the platform modules no longer declares requires on the shared module” — I think it was needed for the previous mechanism (shared accessing libraryfinders), but don’t remember correctly. If you can restore it and keep modules separated I’m happy to be proven wrong
  • I see you use both jmod command and ZipFile to produce .jmod. Shouldn’t one or the other be enough? Also, currently it seems to use no compression, resulting in 21 Mb file instead of 9 Mb for jar
  • Shouldn’t we publish jmod somewhere? Right now it’s built, but not published, so kind of misses the point, right?
  • Will everything here continue to work with Java 8?

And I would really prefer this to be split into smaller bite-size PRs

@Glavo
Copy link
Contributor Author

Glavo commented Dec 23, 2022

@tonsky

skija.version is being put into platform jars (as it should) but loaded using classloader of shared jar. IIRC it doesn’t worked for me with Java 9 modules, that’s why I had to create a class in each platform

It's not true. I tested these situations. In these cases, ClassLoader performs well in locating resources:

  • Shared jar and platform jar are both in the classpath;
  • The shared jar is in the classpath and the platform jar is in the module path;
  • The shared jar is in the module path and the platform jar is in the classpath;
  • Shared jar and platform jar are both in the module path.

When jlink is not used, there is no difference between using ClassLoader and using LibraryFinder to locate resources.

Only in the case of using jlink, there may be problems in some edge cases. However, I have examined this issue again these days, it is not difficult to solve. This problem can be solved by using ModuleFinder to find modules and locate resources. I just need a little time to achieve it.

one of the goals of having arch classifier in e.g. libskija_x64.dylib was to make sure I can include all possible skija jars at the same time and still get the right one loaded. I think by removing _x64/_arm64 you broke that? Now if I add both jars to class path there’ll be two identical resources which might be tricky to distinguish

Native libraries of different architectures are already in different packages, and they will not conflict in any case. I don't understand why it's even doing this.

I see you use both jmod command and ZipFile to produce .jmod. Shouldn’t one or the other be enough? Also, currently it seems to use no compression, resulting in 21 Mb file instead of 9 Mb for jar

The jmod command is difficult to configure. I ran into trouble when using the jmod command, which made it is difficult to place all resources where they should be by directly calling the jmod command

If I want to generate the correct jmod file only through the jmod command, I must create a temporary directory, copy the resource files to the corresponding layout location, and then call the jmod command. Compared with this, I think ZipFile is a cleaner solution.

The reason why I don't use ZipFile directly to create the jmod file is that there are magic numbers (0x4A 0x4D 0x01 0x00) at the beginning of the jmod file. It seems that there is no simple way to use ZipFile to write these magic numbers.

The lack of compression might be an oversight on my part, I'll check that out later.

Shouldn’t we publish jmod somewhere? Right now it’s built, but not published, so kind of misses the point, right?

I think the jmod file should be placed in GitHub Release? Or should we push it to Maven Central?

There seems to be no best practice for publishing jmod files, so you need to make a choice.

Will everything here continue to work with Java 8?

Of course, I certainly don't want to break the work I just finished.

And I would really prefer this to be split into smaller bite-size PRs

I agree that this is a big change. The reason why I make so many changes in a PR is that I think these changes are actually closely related. A part of them may be split into separate PRs, but the rest will still be no small modifications.

Do you think it must be split?

@tonsky
Copy link
Contributor

tonsky commented Dec 24, 2022

Native libraries of different architectures are already in different packages, and they will not conflict in any case. I don't understand why it's even doing this.

Oh I see, the name is the same but base path is different. That works! My mistake

If I want to generate the correct jmod file only through the jmod command, I must create a temporary directory, copy the resource files to the corresponding layout location, and then call the jmod command.

Let’s do this? I just don’t like that two approaches are mixed

I think the jmod file should be placed in GitHub Release?

Let’s do that, yes

Do you think it must be split?

Not this one, but future ones please?

@Glavo
Copy link
Contributor Author

Glavo commented Dec 24, 2022

@tonsky

Now I have made some updates:

  • GitHub Action will create a release for the tag and upload all the jmod files;
  • The mechanism for finding native libraries has been updated. When resources cannot be found through ClassLoader, ModuleFinder will be used to find them on the module path.
    I did a lot of different tests, including mixing module paths and class paths, which always found resources.
  • Now I only use Python scripts to generate jmod files, instead of mixing jmod commands.

Not this one, but future ones please?

I'm sorry that this PR is so large.

The original intention of this PR is to optimize the way to load the local library.

As part of this work, I went to adding _arch everywhere, but when I started adding, I found that there were more changes needed than I thought.

If I implement the functions in this PR from scratch, I will first add arch anywhere, and other functions can be split into separate PRs. But I don't want to do these things again, because it will take me a lot of time to test again.

I apologize for the trouble this has caused you.

@tonsky tonsky merged commit f18a6e6 into HumbleUI:master Dec 28, 2022
@tonsky
Copy link
Contributor

tonsky commented Dec 28, 2022

Ok, merged, tagged as 0.109.1, let’s wait until it appears in Maven Central and see if it works

@tonsky
Copy link
Contributor

tonsky commented Dec 28, 2022

Looks like I fucked up GH Release artifacts upload, should be fixed in next release

@Glavo Glavo deleted the load-native branch December 28, 2022 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants