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

native-image - use Feature implementation instead of config files #835

Merged
merged 21 commits into from
May 2, 2023

Conversation

kkriske
Copy link
Contributor

@kkriske kkriske commented Feb 1, 2023

This PR replaces the resource-config.json and jni-config.json files by a org.graalvm.nativeimage.hosted.Feature implementation.

  • The registered JNI configuration is unchanged, but is now configured using the native-image java API. This is easier to maintain than the json files.
  • The resource config is different, as previously all library resources were included. Now only the library relevant for the target runtime of the native-image compilation is included as a resource. To do this, the existing logic in SQLiteJDBCLoader was reused. Additionally the metadata resources containing version info and properties are read at build-time in static code blocks which avoid the need to include those resources and avoid the need to do the IO operations at runtime.

Finally, I added a new org.sqlite.lib.exportPath which takes in a directory that is used by the SqliteJdbcFeature to export the JNI library to at build-time. If this property is set, the library will no longer be included as a resource and it becomes the responsibility of the end user to provide the exported library next to the native-image executable.
The benefit of doing this is that the library no longer needs to be unpacked at runtime and again some IO operations can be eliminated.

@pyckle
Copy link
Contributor

pyckle commented Feb 6, 2023

Nice work (: This should reduce native image size by only including the relevant JNI lib.

@kkriske
Copy link
Contributor Author

kkriske commented Feb 6, 2023

This should reduce native image size by only including the relevant JNI lib.

That's the intention indeed. I would like to go one step further even, and allow to choose between having the resource unpacked at build-time or at runtime, probably in a property that can be set at build time. Because having it stored internally is bad for startup time, and it is entirely possible to unpack it at build-time and store it next to the executable. Then no library would need to be stored in the executable itself, and it could be loaded directly from next to the executable.

The library is fairly small so you could argue that startup time isn't really impacted that much, but it does make a difference if antivirusses decide to scan the library you just unpacked. It can also makes a sizeable difference in for example AWS Lambda.

pom.xml Show resolved Hide resolved
@pyckle
Copy link
Contributor

pyckle commented Feb 6, 2023

you could argue that startup time isn't really impacted that much

This sort of bloat adds up as software use more and more libraries. I think that adding this as an option is useful and wise.

@kkriske kkriske marked this pull request as ready for review March 12, 2023 22:00
README.md Show resolved Hide resolved
@kkriske
Copy link
Contributor Author

kkriske commented Mar 13, 2023

@gotson If you want, native-image support can be officialized by including sqlite-jdbc on this page: https://www.graalvm.org/native-image/libraries-and-frameworks/

@gotson
Copy link
Collaborator

gotson commented Mar 20, 2023

@kkriske i see you moved the PR from draft to ready, but it doesn't have any description at all.

Would you be able to provide some general description of what this changes aim to address, and how?

@gotson gotson self-assigned this Mar 24, 2023
@gotson
Copy link
Collaborator

gotson commented Mar 24, 2023

@kkriske looks like there's a conflict to resolve

@morki
Copy link

morki commented Mar 29, 2023

@kkriske is it possible to somehow statically link the exported library into resulting executable?

@kkriske
Copy link
Contributor Author

kkriske commented Mar 29, 2023

@kkriske is it possible to somehow statically link the exported library into resulting executable?

No. It's a dynamic library. Static linking would require building static libraries.
It would be possible to statically link this hypothetical static library but that would rely on native-image internal api's which I would not recommend using as a library. That would force the end user to add certain --add-exports arguments to the native-image command in order to allow the build to work and obviously there's no stability guarantee of that api.

@morki
Copy link

morki commented Mar 29, 2023

@kkriske thank you for the fast response :)

@kkriske
Copy link
Contributor Author

kkriske commented Apr 8, 2023

@gotson FYI this should be ready for consideration/review.

@gotson
Copy link
Collaborator

gotson commented Apr 17, 2023

@gotson FYI this should be ready for consideration/review.

i will have a look once i have some time. Can you fix the conflicts in the meantime?

@kkriske
Copy link
Contributor Author

kkriske commented Apr 19, 2023

There didn't seem to be any conflicts, but I have updated the feature branch.

@gotson
Copy link
Collaborator

gotson commented Apr 20, 2023

There didn't seem to be any conflicts, but I have updated the feature branch.

Yes my bad, the github screen was showing conflicts for a rebase, but not for a squash merge. I don't know why. Thanks.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/main/java/org/sqlite/SQLiteJDBCLoader.java Outdated Show resolved Hide resolved
src/main/java9/module-info.java Show resolved Hide resolved
src/main/java/org/sqlite/core/NativeDB.java Outdated Show resolved Hide resolved
docs: fix typos
chore: remove outdated openjdk7 workaround
@gotson
Copy link
Collaborator

gotson commented Apr 24, 2023

it seems CI is failing for GraalVM

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@gotson gotson left a comment

Choose a reason for hiding this comment

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

A very solid PR, thanks a lot for the hard work!

@gotson gotson merged commit 6f42683 into xerial:master May 2, 2023
@pyckle
Copy link
Contributor

pyckle commented May 2, 2023

A very solid PR, thanks a lot for the hard work!

Seconded. Very nice work done here!

@kkriske kkriske deleted the use-native-image-feature branch June 12, 2023 18:33
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.

4 participants