[java] [doc] ConsecutiveAppendsShouldReuse is it really an optimization?

[pmd]

[java] [doc] ConsecutiveAppendsShouldReuse is it really an optimization?

[wipu]

I'm not convinced. I haven't seen the sources of Benchmark, but I suspect it doesn't do anything with the strings, which makes the test artificial. Here is my benchmark that uses the strings (adds up their length), and I don't get significant differences, at least not anything like 4x. (In fact, sometimes it's even faster without chaining):

`
public class StringBuilderChainingMicrobenchmark {

private static final int LOOP_LENGTH = 50_000_000;
private static final int STRING_LENGTH = 1377777780;
private String d = "d....";
private String e = "e...";

public static void main(String[] args) {
	new StringBuilderChainingMicrobenchmark().benchmark();
}

private void benchmark() {
	Microbenchmarker.benchmark("chained", () -> chained());
	Microbenchmarker.benchmark("unchained", () -> unchained());
}

private static void assertLength(int sum) {
	if (sum != STRING_LENGTH) {
		throw new IllegalStateException("Incorrect string length: " + sum);
	}
}

private void chained() {
	int sum = 0;
	for (int i = 0; i < LOOP_LENGTH; i++) {
		sum += stringBuiltWithChaining(i).length();
	}
	assertLength(sum);
}

private void unchained() {
	int sum = 0;
	for (int i = 0; i < LOOP_LENGTH; i++) {
		sum += stringBuiltWithoutChaining(i).length();
	}
	assertLength(sum);
}

private String stringBuiltWithChaining(int i) {
	StringBuilder b = new StringBuilder();
	b.append("a").append(i).append("b").append(i).append("c").append(d)
			.append(e);
	return b.toString();
}

private String stringBuiltWithoutChaining(int i) {
	StringBuilder b = new StringBuilder();
	b.append("a");
	b.append(i);
	b.append("b");
	b.append(i);
	b.append("c");
	b.append(d);
	b.append(e);
	return b.toString();
}

}
`

And this is my Microbenchmark.java:

`
public class Microbenchmarker {

public static void benchmark(String name, Runnable task) {
	for (int i = 0; i < 3; i++) {
		System.out.println("Calling gc");
		System.gc();
		System.out.println("Warming up " + name);
		task.run();
	}
	System.out.println("Calling gc");
	System.gc();
	System.out.println("Running " + name);
	long t1 = System.nanoTime();
	task.run();
	long t2 = System.nanoTime();
	double secondsTaken = (t2 - t1) / 1000D / 1000D;
	System.out.println("Task " + name + " took " + secondsTaken + "ms");
}

}
`

An example output:

Calling gc
Warming up chained
Calling gc
Warming up chained
Calling gc
Warming up chained
Calling gc
Running chained
Task chained took 6799.459034ms
Calling gc
Warming up unchained
Calling gc
Warming up unchained
Calling gc
Warming up unchained
Calling gc
Running unchained
Task unchained took 6620.444455ms

[jsotuyod]

@noamtamim thanks for your comments. I'd like to know however what you base them on to support your claims.

In the meantime, I'd gladly show you what evidence supports this rule:

Let's start by analyzing javac output. Given the code:

class Main {
  public String appendInline() {
    final StringBuilder sb = new StringBuilder().append("some").append(' ').append("string");
    return sb.toString();
  }

  public String appendPerLine() {
    final StringBuilder sb = new StringBuilder();
    sb.append("some");
    sb.append(' ');
    sb.append("string");
    return sb.toString();
  }
}

We compile with javac, and check the output with javap -c -s

  public java.lang.String appendInline();
    descriptor: ()Ljava/lang/String;
    Code:
       0: new           #2                  // class java/lang/StringBuilder
       3: dup
       4: invokespecial #3                  // Method java/lang/StringBuilder."<init>":()V
       7: ldc           #4                  // String some
       9: invokevirtual #5                  // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
      12: bipush        32
      14: invokevirtual #6                  // Method java/lang/StringBuilder.append:(C)Ljava/lang/StringBuilder;
      17: ldc           #7                  // String string
      19: invokevirtual #5                  // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
      22: astore_1
      23: aload_1
      24: invokevirtual #8                  // Method java/lang/StringBuilder.toString:()Ljava/lang/String;
      27: areturn

  public java.lang.String appendPerLine();
    descriptor: ()Ljava/lang/String;
    Code:
       0: new           #2                  // class java/lang/StringBuilder
       3: dup
       4: invokespecial #3                  // Method java/lang/StringBuilder."<init>":()V
       7: astore_1
       8: aload_1
       9: ldc           #4                  // String some
      11: invokevirtual #5                  // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
      14: pop
      15: aload_1
      16: bipush        32
      18: invokevirtual #6                  // Method java/lang/StringBuilder.append:(C)Ljava/lang/StringBuilder;
      21: pop
      22: aload_1
      23: ldc           #7                  // String string
      25: invokevirtual #5                  // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
      28: pop
      29: aload_1
      30: invokevirtual #8                  // Method java/lang/StringBuilder.toString:()Ljava/lang/String;
      33: areturn

As seen, the appendPerLine variant produces a much larger bytecode, by producing several extra aload_1 and pop instructions. In turn, this means the JRE will produce a larger callsite and has a greater overhead. Smaller bytecode means not only less instructions to execute, but also higher chances of inlining by the JIT, which will further improve performance.

This alone improves the performance for a cold start. One could argue that the JRE should be able to optimize these instructions away once the VM has warmed up. However, this claim needs support, and would still only apply to long-running processes.

So, let's check this claim, and validate the performance even after warmup. Let's use JMH:

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.State;

@State(Scope.Benchmark)
public class StringBenchmark {
	private String from = "Alex";
	private String to = "Readers";
	private String subject = "Benchmarking with JMH";

	@Param({"16"})
	private int size;
	
	@Benchmark
	public String testEmailBuilderSimple() {
		StringBuilder builder = new StringBuilder(size);
		builder.append("From");
		builder.append(from);
		builder.append("To");
		builder.append(to);
		builder.append("Subject");
		builder.append(subject);
		return builder.toString();
	}

	@Benchmark
	public String testEmailBufferSimple() {
		StringBuffer buffer = new StringBuffer(size);
		buffer.append("From");
		buffer.append(from);
		buffer.append("To");
		buffer.append(to);
		buffer.append("Subject");
		buffer.append(subject);
		return buffer.toString();
	}

	@Benchmark
	public String testEmailBuilderChain() {
		return new StringBuilder(size).append("From").append(from).append("To").append(to).append("Subject")
				.append(subject).toString();
	}

	@Benchmark
	public String testEmailBufferChain() {
		return new StringBuffer(size).append("From").append(from).append("To").append(to).append("Subject")
				.append(subject).toString();
	}
}

We compile and run it and we obtain:

Benchmark                               (size)   Mode  Cnt         Score        Error  Units
StringBenchmark.testEmailBufferChain        16  thrpt  200  22981842.957 ± 238502.907  ops/s
StringBenchmark.testEmailBufferSimple       16  thrpt  200   5789967.103 ±  62743.660  ops/s
StringBenchmark.testEmailBuilderChain       16  thrpt  200  22984472.260 ± 212243.175  ops/s
StringBenchmark.testEmailBuilderSimple      16  thrpt  200   5778824.788 ±  59200.312  ops/s

So, even after warming up, following the rule produces a ~4X improvement in throughput.

Of course, you don't have to believe me, others have done similar analysis and you can even try it yourself.

[noamtamim]

@jsotuyod Thank you very much for this analysis. My claims are based on the fact that the only reference I found to ConsecutiveAppendsShouldReuse is a StackOverflow post, with the consensus that this is a micro-optimizations that will be done by the runtime anyway.
I therefore suggest that you add reference in the docs, either to that link you provided or to a generalized version of your response.

Thanks again!

[jsotuyod]

@noamtamim glad I could help.

Well, this certainly is a micro optimization. Most programs should first focus on sorting out big algorithmic issues reducing complexity, but once that is done, even small changes in critical paths can have a big effect. For instance, we recently obtained a 15% performance boost (~3.4 secs) on PMD execution by simply changing a .substring(xxx).endsWith(".") with .charAt(xxx) == '.'.

None the less, this is something that the JVM is not currently optimizing, and for some programs may be of interest. This is certainly not the case for all programs (maybe not even most), but then it's up to the development team to choose which rules to enforce appropriately.

I agree on the need for better documentation, I'll take your advice and improve that rule's doc ASAP. We have plans on our roadmap to provide better default rulesets for new users.

It's true 'though that maybe that rule would fit better the optimization ruleset than the string ruleset.

[noamtamim]

Note that the best practice for using StringBuffer/Builder is still chaining the append() method calls. This is its design. So other than the micro-optimization, it's also a matter of style.

Thanks again for your time.