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

Lazy System Properties for native image #45997

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Jan 31, 2025

The GraalVM provides a lazy implementation to access system properties that are expensive to calculate: https://github.com/oracle/graal/blob/master/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

Still, it ends up calculating all the properties anyway when System#getProperties() is called, which is a common call. Used, for instance, to get the list of names in Quarkus configuration, but also in GetPropertyAction#privilegedGetProperties() which is used in many JVM APIs, for instance, when determining the default timezone. Such initialization may cost a few milliseconds of the native image startup time (measured between 5-6, depending on the system).

https://quarkusio.zulipchat.com/#narrow/channel/187038-dev/topic/System.20Properties.20in.20Native.20Image.20.285.20to.206.20ms.20startup.20time.29

Copy link

quarkus-bot bot commented Jan 31, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit a63de4a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@gsmet
Copy link
Member

gsmet commented Jan 31, 2025

@radcortez could you point me to where determining the default timezone is done?

@radcortez
Copy link
Member Author

Yes, for the log pattern formatter:

[email protected]/java.lang.Thread.getStackTrace(Thread.java:623)
org.graalvm.nativeimage.builder/com.oracle.svm.core.jdk.SystemPropertiesSupport.getProperties(SystemPropertiesSupport.java:150)
[email protected]/java.lang.System.getProperties(System.java:461)
[email protected]/sun.security.action.GetPropertyAction.privilegedGetProperties(GetPropertyAction.java:153)
[email protected]/java.util.TimeZone.setDefaultZone(TimeZone.java:695)
[email protected]/java.util.TimeZone.getDefaultRef(TimeZone.java:685)
[email protected]/java.util.TimeZone.getDefault(TimeZone.java:674)
org.jboss.logmanager.formatters.FormatStringParser.getSteps(FormatStringParser.java:64)
org.jboss.logmanager.formatters.PatternFormatter.setPattern(PatternFormatter.java:80)
org.jboss.logmanager.formatters.ColorPatternFormatter.<init>(ColorPatternFormatter.java:54)
io.quarkus.runtime.logging.LoggingSetupRecorder.configureConsoleHandler(LoggingSetupRecorder.java:601)
io.quarkus.runtime.logging.LoggingSetupRecorder.initializeLogging(LoggingSetupRecorder.java:181)
io.quarkus.runner.recorded.LoggingResourceProcessor$setupLoggingRuntimeInit1041640541.deploy_0(Unknown Source)
io.quarkus.runner.recorded.LoggingResourceProcessor$setupLoggingRuntimeInit1041640541.deploy(Unknown Source)
io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
io.quarkus.runtime.Application.start(Application.java:101)
io.quarkus.runtime.ApplicationLifecycleManager.run(ApplicationLifecycleManager.java:121)
io.quarkus.runtime.Quarkus.run(Quarkus.java:77)
io.quarkus.runtime.Quarkus.run(Quarkus.java:48)
io.quarkus.runtime.Quarkus.run(Quarkus.java:137)
io.quarkus.runner.GeneratedMain.main(Unknown Source)

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I added a small question but I would like @zakkak @jerboaa or @galderz to have a look.

@gsmet
Copy link
Member

gsmet commented Feb 4, 2025

@zakkak friendly ping :)

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

It looks good to me but way more "intrunsive" than expected.

@radcortez do you see any reason not to propose this change to upstream Graal?

@radcortez
Copy link
Member Author

@radcortez do you see any reason not to propose this change to upstream Graal?

Yes, I wanted to propose something to fix the issue there, but I also wanted to have something to quickly fix the problem on our side

@gsmet gsmet merged commit 1c310a9 into quarkusio:main Feb 4, 2025
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Feb 4, 2025
@gsmet
Copy link
Member

gsmet commented Feb 4, 2025

FWIW, I agree on the fact that's it's too intrusive. Let's try to improve upstream!

@radcortez
Copy link
Member Author

Yes. I'll open up an issue, and I can also submit a PR to them.

@zakkak
Copy link
Contributor

zakkak commented Feb 5, 2025

Upstream has changed this class quite a bit https://github.com/oracle/graal/blob/master/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java so you would first need to check if the issue is still reproducible with the latest GraalVM (you can get a build from https://github.com/graalvm/graalvm-ce-dev-builds/releases

zakkak added a commit to zakkak/quarkus that referenced this pull request Feb 5, 2025
quarkusio#45997 breaks compatibility
with GraalVM for JDK 25 as the method being substituted doesn't exist
there.
zakkak added a commit to zakkak/quarkus that referenced this pull request Feb 5, 2025
quarkusio#45997 breaks compatibility
with GraalVM for JDK 25 as the method being substituted doesn't exist
there.
@radcortez
Copy link
Member Author

I opened this one on Graal VM side: oracle/graal#10630

zakkak added a commit to zakkak/quarkus that referenced this pull request Feb 5, 2025
quarkusio#45997 breaks compatibility
with GraalVM for JDK 25 as the method being substituted has been
renamed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants