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.

Sign up for GitHub

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

Jump to bottom

Use StringBuilder instead of String concat for startup code #45647

Merged
merged 1 commit into from
Jan 17, 2025
Merged

Use StringBuilder instead of String concat for startup code #45647

merged 1 commit into from
Jan 17, 2025

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jan 16, 2025

Unfortunately the JDK's indyfied String concatenation has a slight performance penalty at startup, so for code we know will always be run at startup, let's not pay that unnecessary price.
With this change we force javac to use the
StringBuilder strategy

@geoand geoand mentioned this pull request Jan 16, 2025
Closed
@geoand geoand requested review from dmlloyd and gsmet January 16, 2025 13:24
@geoand geoand force-pushed the config-string-concat-v2 branch from a435fbf to 4fc6d0c Compare January 16, 2025 13:24
franz1981
franz1981 reviewed Jan 16, 2025
View reviewed changes
Copy link
Contributor

@franz1981 franz1981 left a comment

Choose a reason for hiding this comment

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

Isn't dangerous if we use somewhere it in the hot path?
In the hot path it save to copy twice the content of the string, passing it directly from the constructure array into the final String...

@geoand
Copy link
Contributor Author

geoand commented Jan 16, 2025

See #45638 (comment)

@dmlloyd
Copy link
Member

dmlloyd commented Jan 16, 2025

I think we'd want to do some measurements on this one to make sure we don't lose any run time performance (I don't think we would, but it'd be worth verifying).

@geoand
Copy link
Contributor Author

geoand commented Jan 16, 2025

Makes sense

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@geoand
Use StringBuilder instead of String concat for startup code
ddb26ac 
Unfortunately the JDK's indyfied String concatenation
has a slight performance penalty at startup, so for
code we know will always be run at startup, let's not
pay that unnecessary price.
With this change we force javac to use the
StringBuilder strategy
@geoand
Copy link
Contributor Author

geoand commented Jan 17, 2025

I think we'd want to do some measurements on this one to make sure we don't lose any run time performance (I don't think we would, but it'd be worth verifying).

I did a basic stress test on a Hello World REST application and this change made no difference whatsoever

@geoand geoand force-pushed the config-string-concat-v2 branch from 4fc6d0c to ddb26ac Compare January 17, 2025 08:17
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Jan 17, 2025 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

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

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
✖ Native Tests - Misc4 Build Failures Logs Raw logs 🔍

Full information is available in the Build summary check run.
You can consult the Develocity build scans.

Failures

⚙️ Native Tests - Misc4 #

- Failing: integration-tests/gradle

📦 integration-tests/gradle

✖ io.quarkus.gradle.nativeimage.BasicJavaNativeBuildIT.shouldBuildNativeImage - History - More details - Source on GitHub

java.lang.AssertionError: Gradle build failed with exit code 137
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:140)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:57)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:52)
	at app//io.quarkus.gradle.nativeimage.QuarkusNativeGradleITBase.runGradleWrapper(QuarkusNativeGradleITBase.java:36)
	at app//io.quarkus.gradle.nativeimage.BasicJavaNativeBuildIT.shouldBuildNativeImage(BasicJavaNativeBuildIT.java:24)
	at [email protected]/java.lang.reflect.Method.invoke(Method.java:569)
	at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1511)

✖ io.quarkus.gradle.nativeimage.BasicJavaNativeBuildIT.shouldBuildNativeImageWithCustomName - History - More details - Source on GitHub

java.lang.AssertionError: Gradle build failed with exit code 137
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:140)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:57)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:52)
	at app//io.quarkus.gradle.nativeimage.QuarkusNativeGradleITBase.runGradleWrapper(QuarkusNativeGradleITBase.java:36)
	at app//io.quarkus.gradle.nativeimage.BasicJavaNativeBuildIT.shouldBuildNativeImageWithCustomName(BasicJavaNativeBuildIT.java:53)
	at [email protected]/java.lang.reflect.Method.invoke(Method.java:569)
	at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1511)

✖ io.quarkus.gradle.nativeimage.BasicJavaNativeBuildIT.shouldBuildNativeImageWithCustomNameWithoutSuffix - History - More details - Source on GitHub

java.lang.AssertionError: Gradle build failed with exit code 137
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:140)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:57)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:52)
	at app//io.quarkus.gradle.nativeimage.QuarkusNativeGradleITBase.runGradleWrapper(QuarkusNativeGradleITBase.java:36)
	at app//io.quarkus.gradle.nativeimage.BasicJavaNativeBuildIT.shouldBuildNativeImageWithCustomNameWithoutSuffix(BasicJavaNativeBuildIT.java:84)
	at [email protected]/java.lang.reflect.Method.invoke(Method.java:569)
	at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1511)

✖ io.quarkus.gradle.nativeimage.CustomNativeTestSourceSetIT.runNativeTests - History - More details - Source on GitHub

java.lang.AssertionError: Gradle build failed with exit code 137
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:140)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:57)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:52)
	at app//io.quarkus.gradle.nativeimage.QuarkusNativeGradleITBase.runGradleWrapper(QuarkusNativeGradleITBase.java:36)
	at app//io.quarkus.gradle.nativeimage.CustomNativeTestSourceSetIT.runNativeTests(CustomNativeTestSourceSetIT.java:17)
	at [email protected]/java.lang.reflect.Method.invoke(Method.java:569)
	at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1511)

✖ io.quarkus.gradle.nativeimage.NativeIntegrationTestIT.nativeTestShouldRunIntegrationTest - History - More details - Source on GitHub

java.lang.AssertionError: Gradle build failed with exit code 137
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:140)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:57)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:52)
	at app//io.quarkus.gradle.nativeimage.QuarkusNativeGradleITBase.runGradleWrapper(QuarkusNativeGradleITBase.java:36)
	at app//io.quarkus.gradle.nativeimage.NativeIntegrationTestIT.nativeTestShouldRunIntegrationTest(NativeIntegrationTestIT.java:24)
	at [email protected]/java.lang.reflect.Method.invoke(Method.java:569)
	at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1511)

✖ io.quarkus.gradle.nativeimage.NativeIntegrationTestIT.runNativeTestsWithOutputName - History - More details - Source on GitHub

java.lang.AssertionError: Gradle build failed with exit code 137
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:140)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:57)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:52)
	at app//io.quarkus.gradle.nativeimage.QuarkusNativeGradleITBase.runGradleWrapper(QuarkusNativeGradleITBase.java:36)
	at app//io.quarkus.gradle.nativeimage.NativeIntegrationTestIT.runNativeTestsWithOutputName(NativeIntegrationTestIT.java:35)
	at [email protected]/java.lang.reflect.Method.invoke(Method.java:569)
	at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1511)

✖ io.quarkus.gradle.nativeimage.NativeIntegrationTestIT.runNativeTestsWithoutRunnerSuffix - History - More details - Source on GitHub

java.lang.AssertionError: Gradle build failed with exit code 137
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:140)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:57)
	at app//io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:52)
	at app//io.quarkus.gradle.nativeimage.QuarkusNativeGradleITBase.runGradleWrapper(QuarkusNativeGradleITBase.java:36)
	at app//io.quarkus.gradle.nativeimage.NativeIntegrationTestIT.runNativeTestsWithoutRunnerSuffix(NativeIntegrationTestIT.java:45)
	at [email protected]/java.lang.reflect.Method.invoke(Method.java:569)
	at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1511)

@geoand
Copy link
Contributor Author

geoand commented Jan 17, 2025

So what do we think about this?

@dmlloyd
Copy link
Member

dmlloyd commented Jan 17, 2025

Seems OK to me.

@gsmet gsmet merged commit dc82c8a into quarkusio:main Jan 17, 2025
52 of 53 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Jan 17, 2025
@geoand geoand deleted the config-string-concat-v2 branch January 17, 2025 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@franz1981 franz1981 franz1981 left review comments

@gsmet gsmet gsmet approved these changes

@dmlloyd dmlloyd dmlloyd approved these changes

Assignees
No one assigned
Labels
area/config area/core triage/flaky-test
Projects
None yet
Milestone
3.19 - main
Development

Successfully merging this pull request may close these issues.
4 participants
@geoand @dmlloyd @gsmet @franz1981