-
Notifications
You must be signed in to change notification settings - Fork 170
Removed StringBuffer use #1135
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
Removed StringBuffer use #1135
Conversation
Unfortunately, this is not quite how it works. To the best of my understanding there are two components to the optimization of 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,
This would result in one 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:
I apologize if my previous explanation was cut too short and thus misleading. refs |
This patch probably should just be StringBuffer (synchronized) => StringBuilder |
There was a problem hiding this 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.
I agree. Provided that thread-safety is not required for these classes |
@egonw Do you want me to migrate that over to |
@uli-f, actually not sure. Since some sources suggest that in modern JVMs just using https://howtodoinjava.com/java/string/string-concatenation-performance/ writes:
@johnmay, what do you think? |
Well the StringBuffer should go for sure, but I have no strong feeling on just using My reading of that article is mainly on a simple cases bases.
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. |
@uli-f, conclusion: yes, please update to use the |
Okay, will migrate to |
Super seeded by #1150 |
No description provided.