Skip to content

Conversation

egonw
Copy link
Member

@egonw egonw commented Dec 14, 2024

No description provided.

@egonw egonw requested a review from uli-f December 14, 2024 10:32
@uli-f
Copy link
Member

uli-f commented Dec 16, 2024

Unfortunately, this is not quite how it works.

To the best of my understanding there are two components to the optimization of String concatenations. The first one happens at compile time replacing those String concatenations with either StringBuilder calls (Java 8) or StringConcatFactory calls (Java >8). The second component is only implemented for Java >8 and takes place in the JVM translating those StringConcatFactory calls to a concrete String concatenation implementation of the JVM's choosing.

A prerequisite for this to work is that the compiler is able to optimize this at compile time. This is why concatenations carried out in loops cannot be optimized. More precisely,

String result = "";
for (int i = 0; i < 25; i++) {
  result = result + "counter=" + i;
}

This would result in one StringBuilder being instantiated for each concatenation in the loop. That's an improvement, but not the comprehensive optimization of only having a single StringBuilder for all concatenations.

When it comes to the use of other flow controls (conditional blocks, methods) I would assume that this cannot be determined at compile time either.

So in my understanding I would only use this in simple scenarios such as a locally confined concatenation:

public String toString() {
  return this.fieldA + this.fieldB + this.fieldC;
}

I apologize if my previous explanation was cut too short and thus misleading.

refs

@johnmay
Copy link
Member

johnmay commented Dec 16, 2024

This patch probably should just be StringBuffer (synchronized) => StringBuilder

Copy link
Member

@uli-f uli-f left a comment

Choose a reason for hiding this comment

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

See general feedback in comment.

@uli-f
Copy link
Member

uli-f commented Dec 16, 2024

This patch probably should just be StringBuffer (synchronized) => StringBuilder

I agree. Provided that thread-safety is not required for these classes

@uli-f
Copy link
Member

uli-f commented Feb 6, 2025

@egonw Do you want me to migrate that over to StringBuilder so that it can then be closed?

@egonw
Copy link
Member Author

egonw commented Feb 8, 2025

Do you want me to migrate that over to StringBuilder so that it can then be closed?

@uli-f, actually not sure. Since some sources suggest that in modern JVMs just using "foo" + "bar" can be optimized JIT pretty well, I was not entirely sure what the best approach is. Thanks for the ping!

https://howtodoinjava.com/java/string/string-concatenation-performance/ writes:

Since Java 9, String concatenation is a runtime decision, not a compile time one anymore. It means that after you the code Java 9 (or later) and it can change the underlying implementation however it pleases, without the need to re-compile.

@johnmay, what do you think?

@johnmay
Copy link
Member

johnmay commented Feb 8, 2025

Well the StringBuffer should go for sure, but I have no strong feeling on just using + vs a StringBuilder. Personally here I would probably use StringBuilder here but this isn't going to be a performance critical so doesn't matter what is done.

My reading of that article is mainly on a simple cases bases.

The invokedynamic call to java.lang.invoke.StringConcatFactory offers the facilities for a lazy linkage

In this case the files has multiple concats at different parts I so I think it's unlikely to be able to optimise across those and a StringBuilder is likely better.

@egonw
Copy link
Member Author

egonw commented Feb 8, 2025

@uli-f, conclusion: yes, please update to use the StringBuilder.

@uli-f
Copy link
Member

uli-f commented Feb 8, 2025

@uli-f, conclusion: yes, please update to use the StringBuilder.

Okay, will migrate to StringBuilder then.

@johnmay
Copy link
Member

johnmay commented Feb 25, 2025

Super seeded by #1150

@johnmay johnmay closed this Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants