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

Use StringBuilder instead of String concat for startup code #45638

Closed
wants to merge 1 commit into from

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

For this to have an effect, we would also need smallrye/smallrye-config#1292.

@franz1981 @radcortez I don't really know how I feel about this one. On one hand it does make a difference as the following before and after flamegraphs show:

before
concat-unpatched

after
concat-patched

but on the other hand:

  • The code is much uglier
  • The can turn into a continuous Whac-A-Mole

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
@gsmet
Copy link
Member

gsmet commented Jan 16, 2025

I would try to see if we can get most of the improvement without specifying the length. For me it's what makes things ugly (and cumbersome to maintain). The rest is quite straightforward.

Now, I do agree on the continuous Whac-A-Mole but on the other side, there's not many of us adjusting this code so if we're all on the same page, it should be (mostly?) fine?

@geoand
Copy link
Contributor Author

geoand commented Jan 16, 2025

I would try to see if we can get most of the improvement without specifying the length

We would indeed, I just felt like going the extra small step 🙂

Now, I do agree on the continuous Whac-A-Mole but on the other side, there's not many of us adjusting this code so if we're all on the same page, it should be (mostly?) fine?

I do agree with this, which I why I started doing the work 🙂

@punkratz312
Copy link

is this kind of related?

#45592

@geoand
Copy link
Contributor Author

geoand commented Jan 16, 2025

@dmlloyd pointed out a better way to do this: #45647

@geoand geoand closed this Jan 16, 2025
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Jan 16, 2025
@dmlloyd
Copy link
Member

dmlloyd commented Jan 16, 2025

The risk is that individual call sites might be faster on a one time or initial basis, but the amortized cost might be greater in the end if we apply this change broadly.

@geoand
Copy link
Contributor Author

geoand commented Jan 16, 2025

That's why I only did it for the core module where the classes should pretty much be used at startup, as opposed to extensions which could be handling concats where throughput matters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants