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

Stop relying on native-image's-H: options #27784

Closed
zakkak opened this issue Sep 7, 2022 · 10 comments
Closed

Stop relying on native-image's-H: options #27784

zakkak opened this issue Sep 7, 2022 · 10 comments
Labels

Comments

@zakkak
Copy link
Contributor

zakkak commented Sep 7, 2022

Description

native-image options starting with -H: are internal and subject to change without notice. As a result we should avoid using them where possible.

Quarkus currently uses the following options:

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java
659:                    nativeImageArgs.add("-H:IncludeLocales=" + includeLocales);
690:                    nativeImageArgs.add("-H:-ParseOnce");
713:                        nativeImageArgs.add("-H:+PrintAnalysisCallTree");
715:                        nativeImageArgs.add("-H:PrintAnalysisCallTreeType=CSV");
722:                    nativeImageArgs.add("-H:+CollectImageBuildStatistics");
723:                    nativeImageArgs.add("-H:ImageBuildStatisticsFile=" + nativeImageName + "-timing-stats.json");
725:                    nativeImageArgs.add("-H:BuildOutputJSONFile=" + nativeImageName + "-build-output-stats.json");
736:                        "-H:InitialCollectionPolicy=com.oracle.svm.core.genscavenge.CollectionPolicy$BySpaceAndTime"); //the default collection policy results in full GC's 50% of the time
737:                nativeImageArgs.add("-H:+AllowFoldMethods");
744:                    nativeImageArgs.add("-H:FallbackThreshold=5");
748:                    nativeImageArgs.add("-H:FallbackThreshold=0");
760:                    nativeImageArgs.add("-H:+ReportUnsupportedElementsAtRuntime");
763:                    nativeImageArgs.add("-H:+ReportExceptionStackTraces");
767:                    nativeImageArgs.add("-H:DebugInfoSourceSearchPath=" + APP_SOURCES);
794:                    nativeImageArgs.add("-H:+AddAllCharsets");
796:                    nativeImageArgs.add("-H:-AddAllCharsets");
799:                    nativeImageArgs.add("-H:EnableURLProtocols=" + String.join(",", protocols));
804:                        nativeImageArgs.add("-H:+InlineBeforeAnalysis");
807:                    nativeImageArgs.add("-H:-InlineBeforeAnalysis");
810:                    nativeImageArgs.add("-H:NativeLinkerOption=" + noPIE);
814:                    nativeImageArgs.add("-H:-SpawnIsolates");
830:                    nativeImageArgs.add("-H:+AllowVMInspection");
833:                    nativeImageArgs.add("-H:+UseServiceLoaderFeature");
835:                    nativeImageArgs.add("-H:+TraceServiceLoaderFeature");
837:                    nativeImageArgs.add("-H:-UseServiceLoaderFeature");
840:                    nativeImageArgs.add("-H:+StackTrace");
842:                    nativeImageArgs.add("-H:-StackTrace");
846:                    nativeImageArgs.add("-H:DashboardDump=" + outputTargetBuildItem.getBaseName() + "_dashboard.dump");
847:                    nativeImageArgs.add("-H:+DashboardAll");
854:                    nativeImageArgs.add("-H:AdditionalSecurityProviders=" + additionalSecurityProviders);

Relates to #25943

Implementation ideas

Some of these can be replaced with their respective -- prefixed option (which is done in #27783). For the rest we need to reconsider whether they are still applicable/make-sense and see if it would make sense to ask the GraalVM team to make them API options (aka prefixed with -- instead of -H:)

@zakkak
Copy link
Contributor Author

zakkak commented Sep 9, 2022

Quoting related discussion from oracle/graal#4862 (comment)

For the other option-use you mentioned there are the following remarks:

* `-H:-ParseOnce`
  We recommend not using this option anymore.

This is only used with GraalVM < 22.2 so there is no issue moving forward.

* `-H:InitialCollectionPolicy=com.oracle.svm.core.genscavenge.CollectionPolicy$BySpaceAndTime`
  What is the reasoning behind that? Did you measure that this is actually beneficial?

It seems like this was based on some past evaluation of the then options and needs to be re-evaluated. There are people working on it.

* `-H:+AllowFoldMethods`
  This should not be necessary. Please try to remove the option and report back if it causes problems.

@galderz it looks like this was introduced by you in quarkusio/quarkus#13376. Can you please have a look?

* `-H:FallbackThreshold=5`
  This is equivalent to API option `--auto-fallback`

* `-H:FallbackThreshold=0`
  This is equivalent to API option `--no-fallback`

* `-H:EnableURLProtocols`
  This is equivalent to API option `--enable-url-protocols`

The above have been replaced in quarkusio/quarkus#27783

* `-H:-InlineBeforeAnalysis`
  This option should not be used anymore.

The option adding this flag has been deprecated in quarkusio/quarkus#27783

* `-H:-SpawnIsolates`
  Except for native-debugging on Windows this option should not be used anymore.

This was introduced back in Dec 2018, when isolates where spawned by default(?). We should mark this as deprecated and eventually remove it. Users (and testers) wil still disable full stack traces using quarkus.native.additional-build-args.

* `-H:-StackTrace`
  What is the reasoning behind that? Did you measure that this is actually beneficial?

@Sanne this was introduced as an option in quarkusio/quarkus@8d16a42 do you remember why it was needed for? In any case I believe we should deprecate the corresponding option and eventually delete it. Users will still be able to disable full stack traces using quarkus.native.additional-build-args.

@Sanne
Copy link
Member

Sanne commented Sep 9, 2022

-H:-StackTrace

Yes I remember seeing some substantial benefits from it - but it's been a while, I wouldn't know if they are still applicable.
The drawback being that stacktraces aren't printed exactly as in Java, but people looking at it didn't find it too bad (they are still very understandable) so we considered it a good tradeoff.

IMO it has value to have specific configuration knobs, such as fullStackTraces be called out explicitly in the documented configuration. It encourages people to try these out to see what the platform is really capable of.

Rather than removing these just because they are "at risk" of being removed upstream, we could add integration tests to cover for their existance? So we'd know if they are indeed removed and we can amend or document the changes as necessary.

@zakkak
Copy link
Contributor Author

zakkak commented Sep 14, 2022

IMO it has value to have specific configuration knobs, such as fullStackTraces be called out explicitly in the documented configuration. It encourages people to try these out to see what the platform is really capable of.

We can still ensure the same visibility for such configuration knobs in a different manner.
I suggest we stop adding such parameters to Quarkus (while gradually removing the existing ones) and instead create a documentation page with "advanced" native-image tuning options, that can be used by setting quarkus.native.additional-build-args accordingly. This way we:

  • keep sharing the gathered knowledge about potential optimization opportunities when using non-API options
  • make clear that these options may become unavailable any time

Rather than removing these just because they are "at risk" of being removed upstream, we could add integration tests to cover for their existance? So we'd know if they are indeed removed and we can amend or document the changes as necessary.

I see this as an orthogonal issue and agree that anything we mention in the docs would be nice to be tested using some integration test.

WDYT?

@Sanne
Copy link
Member

Sanne commented Sep 14, 2022

Honestly I don't see much value in migrating away from the existing options. What's the point - are we actually terrified that some of these might cease to exist?
Surely that would happen in a minor, no? Which gives us ample time to react - but most importantly it's hypotethical, it might never be a problem.

@zakkak
Copy link
Contributor Author

zakkak commented Sep 14, 2022

What's the point - are we actually terrified that some of these might cease to exist?

I wouldn't say "terrified", but there is indeed chance they might cease to exist (we have seen it happen with other options in the past, see the Deprecated options in NativeConfig).

Surely that would happen in a minor, no?

Not necessarily, there is no such commitment from upstream Graal. Non-API code (including parameters) may change anytime without notice.

but most importantly it's hypotethical, it might never be a problem.

That's true, but we have seen it happen for other options (see the Deprecated options in NativeConfig).

@Sanne
Copy link
Member

Sanne commented Sep 14, 2022

Well ok if you badly want to change this, feel free to go ahead and explore that option; personally I'm not too concerned and would hope you don't spend too much time on it.

@geoand
Copy link
Contributor

geoand commented Apr 10, 2023

What's the status of this?

@jerboaa
Copy link
Contributor

jerboaa commented Apr 11, 2023

Personally, I'm not sure we can realistically remove reliance on those -H options entirely. We might be able to reduce usage, but removing them all seems unlikely to me. YMMV.

@Sanne
Copy link
Member

Sanne commented Apr 11, 2023

I agree with @jerboaa . What about we close this issue and keep it as an ongoing goal to try to avoid using -H options when possible? I think that goes w/o saying... so no need for specific trackers.

@geoand
Copy link
Contributor

geoand commented Apr 11, 2023

+1 for me to what you propose @Sanne

@Sanne Sanne closed this as not planned Won't fix, can't repro, duplicate, stale Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants