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

Performance: Escape fields and keys more efficiently #424

Merged
merged 3 commits into from
Mar 10, 2018

Conversation

mschaefers
Copy link

Under heavy load a profiler shows that calling String.replace() in FIELD_ESCAPER or KEY_ESCAPER is significantly slower than escaping manually by writing directly into the current StringBuffer.

This pull request does not change behavior but only execution speed

@codecov-io
Copy link

codecov-io commented Mar 9, 2018

Codecov Report

Merging #424 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #424      +/-   ##
============================================
+ Coverage     84.86%   84.98%   +0.11%     
- Complexity      270      274       +4     
============================================
  Files            19       19              
  Lines          1183     1192       +9     
  Branches        119      121       +2     
============================================
+ Hits           1004     1013       +9     
  Misses          118      118              
  Partials         61       61
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/influxdb/dto/Point.java 82.82% <100%> (+1%) 31 <6> (+4) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be3b8b6...458d4ea. Read the comment docs.

@majst01
Copy link
Collaborator

majst01 commented Mar 9, 2018

Any numbers about performance improvements ?

@mschaefers
Copy link
Author

I used jvmtop (https://github.com/patric-r/jvmtop) as a simple profiler. Profiling our application resulted in 14-15% of CPU time to be used in closure KEY_ESCAPER. With this patch the CPU percentage of escapeKey was reported with 1-2%.

@mschaefers
Copy link
Author

Another very simple test can be done by executing the following testcase in PointTest.java

It creates 2M different lineProtocol Strings in a loop of 1M, single threaded, having to escape 7M keys and 2M values

Execution time on my laptop without this merge request: ~7.2sec vs. with merge request: ~1.8sec.

@org.junit.Test
public void lineProtocolPerformanceTest() {

	String[] value = new String[2];
	final long start = System.currentTimeMillis();
	IntStream.range(1, 1000000).forEach((i) -> {
		Map<String, Object> fields1 = Collections.singletonMap("foo ", "bar\""+i);
		Map<String, Object> fields2 = Collections.singletonMap("foo,"+i, "baz\"");

		String measurement = "measurement"+i;

		TimeUnit precision = TimeUnit.NANOSECONDS;

		Map<String, String> tags = Collections.singletonMap("bar \"\\"+i, "b\"a\"z"+i);

		Long time = System.currentTimeMillis();

		Point p1 = new Point();
		p1.setFields(fields1);
		p1.setMeasurement(measurement);
		p1.setPrecision(precision);
		p1.setTags(tags);
		p1.setTime(time);

		Point p2 = new Point();
		p2.setFields(fields2);
		p2.setMeasurement(measurement);
		p2.setPrecision(precision);
		p2.setTags(tags);
		p2.setTime(time);

		value[0] = p1.lineProtocol();
		value[1] = p2.lineProtocol();
	});
	System.out.println(System.currentTimeMillis() - start);
	System.out.println(value[0]);
	System.out.println(value[1]);
}

Copy link
Collaborator

@majst01 majst01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, just two formatting changes required.

@@ -182,7 +176,7 @@ public Builder fields(final Map<String, Object> fieldsToAdd) {
/**
* Add a time to this point.
*
* @param timeToSet the time for this point
* @param timeToSet the time for this point
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reset this formatting

@@ -205,8 +199,8 @@ public Point build() {
point.setFields(this.fields);
point.setMeasurement(this.measurement);
if (this.time != null) {
point.setTime(this.time);
point.setPrecision(this.precision);
point.setTime(this.time);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@majst01 majst01 merged commit 90f02ea into influxdata:master Mar 10, 2018
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 this pull request may close these issues.

3 participants