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

Integrate Native Image API changes in GraalVM 22.2+ #25943

Closed
1 of 2 tasks
galderz opened this issue Jun 3, 2022 · 18 comments
Closed
1 of 2 tasks

Integrate Native Image API changes in GraalVM 22.2+ #25943

galderz opened this issue Jun 3, 2022 · 18 comments
Labels
kind/epic Large issue with links to sub-issues

Comments

@galderz
Copy link
Member

galderz commented Jun 3, 2022

Description

Adjust Quarkus to upcoming Native Image API changes. An initial discussion happened during the Native Image Committer Community Meeting on 2nd of June.

Analysis

The following required changes were noted during the discussion:

  • Switch from AutomaticFeature to Feature and then pass in features in use via --feature.
  • AlwaysInline won't be exposed in the API so we should stop using it. io/quarkus/runtime/graal/ConfigurationSubstitutions and io/quarkus/jdbc/h2/runtime/graal/RemoteOnly are the only remaining users.
  • [ ] -H: options are not public API. During the meeting @christianwimmer said some of those should become API (e.g. --blah option) and others might no longer be used (e.g. We should stop disabling ParseOnce with 22.2 because memory footprint there has been improved, see Remove use of -H:-ParseOnce in GraalVM 22.2 #25944 for more details). He said he'd investigate Quarkus' use of those are create an issue with recommendations. Work split into Stop relying on native-image's-H: options #27784

FYI @zakkak @jerboaa @n1hility @Sanne @geoand

@jerboaa
Copy link
Contributor

jerboaa commented Jun 27, 2022

There are two cases of internal API usage which recently popped up breaking a potential future API contract (worked-around for now by this PR):

  1. org.graalvm.nativeimage.impl.ConfigurationCondition: Used here, for example:
    tc.load("org.graalvm.nativeimage.impl.ConfigurationCondition"),
  2. org.graalvm.nativeimage.impl.RuntimeSerializationSupport: Used here, for example:
    duringSetup.loadClassFromTCCL("org.graalvm.nativeimage.impl.RuntimeSerializationSupport"));

@galderz
Copy link
Member Author

galderz commented Jul 15, 2022

There are two cases of internal API usage which recently popped up breaking a potential future API contract (worked-around for now by this PR):

  1. org.graalvm.nativeimage.impl.ConfigurationCondition: Used here, for example:
    tc.load("org.graalvm.nativeimage.impl.ConfigurationCondition"),

Unless I'm mistaken, ConfigurationCondition was first introduced by @zakkak in this commit to support resource registration.

  1. org.graalvm.nativeimage.impl.RuntimeSerializationSupport: Used here, for example:
    duringSetup.loadClassFromTCCL("org.graalvm.nativeimage.impl.RuntimeSerializationSupport"));

Yeah, that's used to enable registering lambdas for serialization.

I'll investigate these 2 use case to see if there are ways to do the same via public APIs, otherwise I'll create issue(s) in oracle/graal.

@galderz
Copy link
Member Author

galderz commented Jul 15, 2022

I think these ^ fall within the "JNI / Resource / Proxy / Serialization registration" section in the discussion. A public API version to register resources, serialization and others are required. The discussion already highlights this issue, so it should be solved with the API work.

@jerboaa
Copy link
Contributor

jerboaa commented Aug 9, 2022

oracle/graal#4783 has some more details about what replacements are for some currently internal API usage. Particularly:
ReflectionUtil (replace with own) => oracle/graal#4783 (comment)
LocalizationFeature => oracle/graal#4783 (comment)

@jerboaa
Copy link
Contributor

jerboaa commented Aug 9, 2022

The annotations (except AlwaysInline as promised) will move to a different artifact (sdk), but remain in the same package:
oracle/graal#4683

@olpaw
Copy link

olpaw commented Aug 11, 2022

We should stop disabling ParseOnce

@jerboaa, @galderz while at it you could also remove -H:+JNI since this is always enabled anyway (since Oct 10, 2018, see oracle/graal@6346801)

@zakkak
Copy link
Contributor

zakkak commented Aug 12, 2022

@jerboaa, @galderz while at it you could also remove -H:+JNI since this is always enabled anyway (since Oct 10, 2018, see oracle/graal@6346801)

Thanks for pointing this out @olpaw. #27267 takes care of it.

@galderz
Copy link
Member Author

galderz commented Aug 16, 2022

Re: ParseOnce. At the end of June I carried some tests to verify its effects on 22.2 and indeed the memory usage had improved, so it no longer resulted in a max RSS penalty. So I think it's safe to remove disabling it.

@zakkak
Copy link
Contributor

zakkak commented Aug 17, 2022

Re: ParseOnce. At the end of June I carried some tests to verify its effects on 22.2 and indeed the memory usage had improved, so it no longer resulted in a max RSS penalty. So I think it's safe to remove disabling it.

FYI I replied to this in #25944 (comment) to move the discussion under the more specific issue.

@jerboaa
Copy link
Contributor

jerboaa commented Sep 15, 2022

With #27960 there would be no more usage of @NeverInline in Quarkus as far as I can see. As far as @AlwaysInline is concerned, I see these remaining usages:

$ grep -rn '@AlwaysInline'
extensions/jdbc/jdbc-mssql/runtime/src/main/java/io/quarkus/jdbc/mssql/runtime/graal/com/microsoft/sqlserver/jdbc/SQLServerJDBCSubstitutions.java:94:    @AlwaysInline("We need this to be constant folded")
extensions/jdbc/jdbc-h2/runtime/src/main/java/io/quarkus/jdbc/h2/runtime/graal/RemoteOnly.java:17:    @AlwaysInline("Method org.h2.engine.SessionRemote.connectEmbeddedOrServer must be able to realize it's only ever going remote")
core/runtime/src/main/java/io/quarkus/runtime/graal/ConfigurationSubstitutions.java:26:    @AlwaysInline("trivial")

@Sanne
Copy link
Member

Sanne commented Sep 21, 2022

@dmlloyd could you comment in regards to Target_io_smallrye_config_SmallRyeConfigProviderResolver#getConfig ? Assuming tests pass w/o the @AlwaysInline("trivial") can we remove it safely?

@dmlloyd
Copy link
Member

dmlloyd commented Sep 21, 2022

Yes, the semantics would not change so it is okay to remove.

@Sanne
Copy link
Member

Sanne commented Sep 21, 2022

@galderz is it worth to work on "-H: options are not public API" in this cycle? As I commented above I see no big value in it - especially no need to rush it. I certainly don't mind if you all want to keep trying that but I'm wondering if it could be broken out into a separate issue, so to allow deferring it, or at least prioritize differently.

@jerboaa
Copy link
Contributor

jerboaa commented Sep 21, 2022

@galderz is it worth to work on "-H: options are not public API" in this cycle? As I commented above I see no big value in it - especially no need to rush it. I certainly don't mind if you all want to keep trying that but I'm wondering if it could be broken out into a separate issue, so to allow deferring it, or at least prioritize differently.

It's not very realistic that this can be completed in it's entirety. For example -H:BuildOutputJSONFile is a new option in 22.3. Also some of the diagnostic ones are unlikely to get public equivalents. So +1 on this being best effort.

@zakkak
Copy link
Contributor

zakkak commented Sep 21, 2022

+1 IMHO we should just try and minimize the use of -H: options and more importantly avoid introducing new Quarkus options mapping 1to1 to -H: options. We should instead mention such options in the documentation where needed and pass them through quarkus.native.additional-build-args.

@galderz
Copy link
Member Author

galderz commented Sep 22, 2022

I'll split the -H: part into a different issue.

@galderz
Copy link
Member Author

galderz commented Sep 22, 2022

I've split -H: work into #28148. We might need individual issues in the future to address individual -H: options, rather than a single issue. Feel free to close the new issue if needed.

@gsmet
Copy link
Member

gsmet commented Mar 6, 2023

I think this has been already handled. If there is remaining work, better open new issues as title would be misleading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/epic Large issue with links to sub-issues
Projects
None yet
Development

No branches or pull requests

8 participants