IDE name and flavor/env: Intellij IDEA with SonarLint plugin, or Gradle id(“se.solrike.sonarlint”) version “2.1.0” with sonarlintPlugins(“org.sonarsource.java:sonar-java-plugin:8.2.0.36672”)
Write a simple loop where you modify String and use that String in any API that consumes String, i.e. you wouldn’t be able to simply pass in StringBuilder (which you could argue I should do if I used String.contains in that loop due to its overload).
public static void main(String[] args) {
java.util.function.Predicate<String> checker = "something"::contains;
String s = "s";
while (checker.test(s)) {
s += "o";
}
}
I think it’s a perfectly valid code where I build a new String for every iteration. Yet I get S1643.
Yes your code is valid ans i think that this rule too.
Strings are immutable objects, so concatenation doesn’t simply add the new String to the end of the existing string. Instead, in each loop iteration, the first String is converted to an intermediate object type, the second string is appended, and then the intermediate object is converted back to a String. Further, performance of these intermediate operations degrades as the String gets longer. Therefore, the use of StringBuilder is preferred.
By using a StringBuilder, you can avoid the first conversion which corresponds to as many new StringBuilders as iterations.
...
StringBuilder s = new StringBuilder("s");
while (checker.test(s.toString())) {
s.append("o");
}
...
Using the String API does not allow you to avoid the last conversion.
(I wonder what made Java compiler developers make such a blunder, as the same “discarded” StringBuilder could obviously be optimally reused )
Under a greater context, I guess the simplest solution will be:
String text = "s";
StringBuilder textBuilder = null;
while (checker.test(text)) {
if (textBuilder == null) {
builder = new StringBuilder(text);
}
textBuilder.append("o");
text = textBuilder.toString();
}
// use text
sacrificing more conditional branches that CPU can optimize for avoiding first instantiation if the condition fails.
(Note that s.toString() also allocates a new String, so it should only be called once.)
Actually, to avoid the if, a better syntax would be to duplicate the condition:
String text = "s";
if (checker.test(text)) {
StringBuilder textBuilder = new StringBuilder(text);
do {
textBuilder.append("o");
text = textBuilder.toString();
} while (checker.test(text));
}
// use text
I guess I’ll just suppress the warning if I assume the condition is almost never true, it’s just too impractical on code readability.
I was interested in where the SonarLint version/plugin comes from because I’m trying to figure out the version of the rule implementation. If it’s the latest implementation, then I’ll flag this for the language team. But if not, then I need you to (somehow) try the upgrade.
Are you in connected mode? And if so with which product (and version)?
Actually you’re right, there is also this Gradle configuration for the plugin (it’s latest): sonarlintPlugins("org.sonarsource.java:sonar-java-plugin:8.2.0.36672")
Hi @vodoc82730,
While the performance boost from this optimization may seem modest in this particular case, it’s essential to consider the bigger picture of sustainable development. Every little improvement, no matter how small, helps reduce our applications’ carbon footprint. Over time, these incremental gains can add up to make a real difference in protecting our planet.
If anything, the concatenation seems to be even faster!
And no, micro-optimization of this level with the cost of such a growth in unmaintainability, and the tens of thousands of hours of developers’ lives this wastes on analyzing False Positives, will never add up to protecting our planet. Surely in a couple of years, compiler developers can fix this in a new JDK (if they want to); it’s not like it’ll save a single tree over the next hundred years.
Anyway, I believe S1643 was intended for cases where you consume the String at the very end, and not when you pass it to an intermediate function in each cycle. You can see obvious gains that are 1000-fold in the original purpose of this report. The “intermediate object type” in docs probably referred to some internals of a StringBuilder. Or this may be specific to some versions of JDK where it behaves differently, idk, I welcome deeper analyses of edge cases where this may be needed, thanks for any input!
I executed youre code on GraalVM JDK21, it takes 95 more time with String concatenation then StringBuilder.
builder - 100000 - elapsed: 1802298us
concatenation - 100000 - elapsed: 172804925us
Okay that’s really interesting, from my observation it seems graalvm-jdk-21.0.4+8.1 optimizes the behavior in some way, sometimes I also got a sudden drop from 80s to 2-20s.
It’ll probably help the benchmark to change the builder.append(sample).toString(); by adding a String variable assignment, i.e.:
String text = null;
for (String sample : samples)
text = builder.append(sample).toString();
Now I typically observe a difference of 80 seconds vs. 84 seconds.
If the tested sample was extraordinarily large, I observed greater difference that never manifested with a small sample for some reason.
@Bachri_Abdel Your article only covers exactly the scenario where we don’t consume String in the loop, which obviously is important. The only question we ask in my scenario is whether there is an overhead of the intermediate object when StringBuilder itself isn’t reused.
Hi @alban.auzeill,
I understand that in this case, maintainability / readability are degraded.
I’m still convinced that this rule makes sense in terms of performance / substantiability, even if the String is consumed in the loop. This needs to be confirmed with a few tests.
Users for whom performance and eco-design are important would be disappointed to lose this detection and might go so far as to implement it in a plugin like eco-code.
It seems too complex to concede that SonarQube provides the right solution for all development contexts. I’d rather argue that SonarQube/SonarLint helps each developer to understand the impact of their code, and lets them decide whether or not to correct it. This is in line with the description of rule S1643 “Therefore, the use of StringBuilder is preferred.” .
We could enrich the description or parameterize the rule if compilation optimizations have not rendered it obsolete.
)
