Skip to content
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

Suggestion : continuous calls of StringBuilder.append #425

Closed
o0lwj0o opened this issue Mar 10, 2018 · 3 comments · Fixed by #426
Closed

Suggestion : continuous calls of StringBuilder.append #425

o0lwj0o opened this issue Mar 10, 2018 · 3 comments · Fixed by #426

Comments

@o0lwj0o
Copy link

o0lwj0o commented Mar 10, 2018

Hi,
    I have found some continuous calls about StringBuilder.append(..) in some files. For example, there are eleven continuous calls in the function toString from the file influxdb-java/src/main/java/org/influxdb/dto/BatchPoints.java.

public String toString() {
    StringBuilder builder = new StringBuilder();
    builder.append("BatchPoints [database=");
    builder.append(this.database);
    builder.append(", retentionPolicy=");
    builder.append(this.retentionPolicy);
    builder.append(", consistency=");
    builder.append(this.consistency);
    builder.append(", tags=");
    builder.append(this.tags);
    builder.append(", points=");
    builder.append(this.points);
    builder.append("]");
    return builder.toString();
}

    If it was achieved like StringBuilder.append(..).append(..)append(..), it will promote its performance.

@majst01
Copy link
Collaborator

majst01 commented Mar 10, 2018

I'm not sure if this brings any performance benefits, can you try with a test whats the performance gain ?

@fmachado
Copy link
Contributor

Disclaimer: I'm not a JVM specialist and take this as a naive test as I did just for fun.

My first thought when I read this message was "nah, JIT will eventually optimize the 'hot' code and inline the multi-line calls because that BatchPoints.toString() is very short" (this seems to be a reliable answer on how the hotspot decides how and when to optimize).

Well, I was wrong; :)

With JMH, I got these numbers:

# Run complete. Total time: 00:00:12

Benchmark                            Mode  Cnt  Score   Error  Units
MyBenchmark.testAppendChained          ss  100  0.011 ± 0.001   s/op
MyBenchmark.testAppendMultipleLines    ss  100  0.027 ± 0.003   s/op
MyBenchmark.testSimpleStringConcat     ss  100  0.011 ± 0.001   s/op

on a 8-core machine

$ cat /proc/cpuinfo | grep "model name"
model name	: Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz
...

running

$ java -version
java version "1.8.0_111"
Java(TM) SE Runtime Environment (build 1.8.0_111-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.111-b14, mixed mode)

And here is the source-code of my test:

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Level;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.infra.Blackhole;

@State(Scope.Thread)
@BenchmarkMode(Mode.SingleShotTime)
@Measurement(batchSize = 100000, iterations = 20)
@Warmup(batchSize = 100000, iterations = 10)
@Fork(5)
public class MyBenchmark {

	private String database;
	private String retentionPolicy;
	private String consistency;
	private String tags;
	private String points;

	@Setup(Level.Iteration)
	public void setup() {
		database = "database";
		retentionPolicy = "retentionPolicy";
		consistency = "consistency";
		tags = "tags";
		points = "points";
	}

	@Benchmark
	public void testAppendMultipleLines(Blackhole bh) {
		StringBuilder builder = new StringBuilder();
		builder.append("BatchPoints [database=");
		builder.append(this.database);
		builder.append(", retentionPolicy=");
		builder.append(this.retentionPolicy);
		builder.append(", consistency=");
		builder.append(this.consistency);
		builder.append(", tags=");
		builder.append(this.tags);
		builder.append(", points=");
		builder.append(this.points);
		builder.append("]");
		bh.consume(builder.toString());
	}

	@Benchmark
	public void testAppendChained(Blackhole bh) {
		StringBuilder builder = new StringBuilder()
			.append("BatchPoints [database=")
			.append(this.database)
			.append(", retentionPolicy=")
			.append(this.retentionPolicy)
			.append(", consistency=")
			.append(this.consistency)
			.append(", tags=")
			.append(this.tags)
			.append(", points=")
			.append(this.points)
			.append("]");
		bh.consume(builder.toString());
	}

	@Benchmark
	public void testSimpleStringConcat(Blackhole bh) {
		bh.consume("BatchPoints [database="
			+ this.database
			+ ", retentionPolicy="
			+ this.retentionPolicy
			+ ", consistency="
			+ this.consistency
			+ ", tags="
			+ this.tags
			+ ", points="
			+ this.points
			+ "]");
	}
}

@majst01
Copy link
Collaborator

majst01 commented Mar 10, 2018

Nice, so we should transform to chained StringBuilder calls at least in the hot path.

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 a pull request may close this issue.

3 participants