Use StringBuilder in favor of StringBuffer

[pickypg]

This removes all instances of StringBuffer that are removeable and marks the remaining usages as suppressed forbidden.

Uncontended synchronization in Java is pretty cheap, but it's unnecessary in these cases.

[jasontedor]

I think in all the cases that you changed, the StringBuffer instances are non-escaping local variables so the locks can be elided (the lock can never be contended). While changing these to use StringBuilder is fine, it's adding StringBuffer to forbidden that bothers me. The reason that I find this problematic is because there are legitimate uses for it (as evidenced by all the methods that you marked with @SuppressForbidden. Yet, @SuppressForbidden is currently all or nothing, meaning that now other forbidden methods can slip into usage in the methods that you marked. Let's just keep things simple here.

[pickypg]

I wouldn't call those remaining uses legitimate so much as required because Java never improved its APIs.

The motivation for the change was a recent commit that I noticed after it was merged that added it. If I remove the forbidden aspect, then it will just happen again.

[jasontedor]

There's nothing wrong with the usage in the recent commit that you noticed (I'm assuming it's the shard lock obtained failed exception change?) We need to keep the forbidden list small (until we can suppress a single method) and really for truly dangerous or abused methods. It's adding suppressions to those methods that bothers me, it just lets methods that should truly be forbidden slip in.

[pickypg]

That's fair. I'll remove the forbidden method aspect.

Maybe we should have a separate, non-failing suggestion of "you should probably use". I don't think that StringBuffer is the end of the world, even when it does use synchronization, but it's one of those things that we should avoid when we can.

[jasontedor]

I don't think that StringBuffer is the end of the world, even when it does use synchronization, but it's one of those things that we should avoid when we can.

Again, in the two cases that are left in this pull request, the JVM will elide the locks. The uses of StringBuffer here are non-escaping local variables (effectively thread local). The C2 compiler knows via escape analysis that these objects will never be visible to another thread and it does use the result of the escape analysis to not even use locking at all (not a biased lock, just no locking at all). There's nothing to avoid here.

[pickypg]

The C2 compiler knows via escape analysis that these objects will never be visible to another thread and it does use the result of the escape analysis to not even use locking at all (not a biased lock, just no locking at all). There's nothing to avoid here.

I get that, but depending on a compiler optimization to make one class' code equivalent to another class is the wrong way to write and review code. Just use the right class, which is what this PR does.

[ywelsch]

LGTM

[jasontedor]

I get that, but depending on a compiler optimization to make one class' code equivalent to another class is the wrong way to write and review code.

I fundamentally disagree with this. In fact, I disagree so strongly that I'm wondering if maybe you did not mean what you've said, or if you meant something different than how I'm interpreting it?

I think it's critical to understand what the compiler and the runtime are doing, primarily for correctness, but also to understand where to rewrite code and where to not. For example, there's this little gem from the JLS (15.8.1):

To increase the performance of repeated string concatenation, a Java compiler may use the StringBuffer class or a similar technique to reduce the number of intermediate String objects that are created by evaluation of an expression.

And the OpenJDK compiler will do this using StringBuilder so you can write code more cleanly using + instead of directly using a StringBuilder and that's okay.

Just use the right class, which is what this PR does.

The implication here is that there was something wrong with the previous uses and that's where I take issue: there was nothing wrong with the previous uses of StringBuffer.

The change is fine, we can make it, my point is that it doesn't matter though.

[jpountz]

I see value in this change from a consistency point of view. LGTM.

[s1monw]

++ for consistency