wavefront/serializer: improve performance by ~30% #5842

charlievieth · 2019-05-12T01:53:20Z

This PR improves the performance and memory consumption of the Wavefront serializer by roughly 30%.

benchmark                 old ns/op     new ns/op     delta
BenchmarkSerialize-16     1168          778           -33.39%

benchmark                 old allocs     new allocs     delta
BenchmarkSerialize-16     16             8              -50.00%

benchmark                 old bytes     new bytes     delta
BenchmarkSerialize-16     1116          800           -28.32%

Required for all PRs:

[ x ] Signed CLA.
[ x ] Associated README.md updated.
[ x ] Has appropriate unit tests.

This PR improves the performance and memory consumption of the Wavefront serializer by roughly 30%. ``` benchmark old ns/op new ns/op delta BenchmarkSerialize-16 1168 778 -33.39% benchmark old allocs new allocs delta BenchmarkSerialize-16 16 8 -50.00% benchmark old bytes new bytes delta BenchmarkSerialize-16 1116 800 -28.32% ```

charlievieth · 2019-05-13T19:43:44Z

Thanks for approving!

danielnelson · 2019-05-14T02:23:44Z

plugins/serializers/wavefront/wavefront.go

@@ -39,6 +41,12 @@ var tagValueReplacer = strings.NewReplacer("\"", "\\\"", "*", "-")

 var pathReplacer = strings.NewReplacer("_", ".")

+var bufPool = sync.Pool{


You could add a reused buffer to the serializer, it isn't required for a Serializer to be thread safe.

This is a notable difference from the Parser interface which is currently required to be thread-safe, though I do think this should be changed in the future. I will update the documentation for these interfaces.

I still think there is value to using a sync.Pool that is global to the package. The sync.Pool will pin the current goroutine to a pool, which should improve locality of reference. Additionally, it does not make the WavefrontSerializer unsafe to use concurrently and is dynamic in size.

There isn't any reason for it to be dynamic though because it will never need more than a single buffer. Another issue with the pool is that you can't return the buffer to it unless you copy the data out of the buffer, I believe the current use in this pull request is unsafe even in a single threaded environment.

Another issue with the pool is that you can't return the buffer to it unless you copy the data out of the buffer

We do that. The contents of pooled buffers are copied to the out buffer (note: out will never alias buf, example). This code is thread-safe.

Personally, I would prefer to not add a mutable field to the WavefrontSerializer, but I understand the argument for simplicity and will make the change.

What are your thoughts on adding a mutex to protect the WavefrontSerializer's scratch buffer? From you comment and a quick scan of the code it appears there is no need for it to be thread-safe, but if that were to change it would be beneficial to have it as a guard against this code being overlooked during a refactor.

I missed this, but this pool was added by accident the one we actually use is pbFree. That said, I'm still removing the pools in favor of a shared buffer - as requested.

Don't add anything to make it thread safe. I agree it there is a risk of code sneaking in that use these wrong, especially with the way the SerializerOutput interface works (sending in a single instance), but I'd like to move the other way with parsers/serializers.

Cool, I made WavefrontSerializer thread-safe.

danielnelson · 2019-05-15T23:17:37Z

plugins/serializers/wavefront/wavefront.go

-	metricSeparator := "."
+	const metricSeparator = "."
+	var out []byte
+	buf := pbFree.Get().(*buffer)


It probably makes sense to just create a single bytes.Buffer and then pass this into the various functions, the out slice would be removed and then there is no reason to append. I don't see a reason we need both of these.

Agreed, will change.

Expect something from me tomorrow or Friday as I'm pretty short on bandwidth right now.

danielnelson · 2019-05-15T23:18:58Z

@puckpuck Can you review? I assume any changes would also need to go into the wavefront-sdk.

Note this removes use of `sync.Pool` and since this is no longer thread-safe the parallel benchmark is also removed.

charlievieth · 2019-05-17T01:14:30Z

plugins/serializers/wavefront/wavefront.go

-		}
-
-		metricValue, buildError := buildValue(value, metric.Metric)
+		metricValue, buildError := buildValue(value, name)


We always ignore this error - is there any point returning it from buildValue()? This was the case before this PR was made.

Seems to be used to determine if the value should be skipped or not.

Typically you log on error from the caller, but we log inside the buildValue function instead to avoid noisy log outputs when the value is of type string (which is expected to not work).

Thanks for the insight, I just changed this to return a bool instead of an error since we used the error as a bool (nil / not-nil) and this saves an alloc.

puckpuck · 2019-05-17T17:11:54Z

plugins/serializers/wavefront/wavefront.go

+	*b = append(*b, c)
+}
+
+func (b *buffer) WriteUnit64(val uint64) {


function name has a typo in it, which can be misleading (Unit vs Uint)

🤦‍♂ thanks - fixed

puckpuck · 2019-05-17T17:11:59Z

plugins/serializers/wavefront/wavefront.go

+	return *b
+}
+
+// Use a fast and simple buffer for constructing statsd messages


I'm guessing this comment is a left over from something else?

kinda, and good catch - will remove.

Since the buffer we use is now a field of the WavefrontSerializer we need to protect it against concurrent access with a mutex.

The error it previously returned was treated as a bool - so we should just return a bool instead.

glinton approved these changes May 13, 2019

View reviewed changes

glinton added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label May 13, 2019

danielnelson suggested changes May 15, 2019

View reviewed changes

wavefront/serializer: add scratch buffer to WavefrontSerializer

96f2a2d

Note this removes use of `sync.Pool` and since this is no longer thread-safe the parallel benchmark is also removed.

charlievieth commented May 17, 2019

View reviewed changes

wavefront/serializer: remove unused buffer.Write() method

6fc8173

puckpuck reviewed May 17, 2019

View reviewed changes

Charlie Vieth added 4 commits May 24, 2019 16:54

wavefront/serializer: remove comment

f63ab04

wavefront/serializer: fix misspelling WriteUnit64 => WriteUint64

0b6a6b7

wavefront/serializer: make WavefrontSerializer thread-safe

cb722eb

Since the buffer we use is now a field of the WavefrontSerializer we need to protect it against concurrent access with a mutex.

wavefront/serializer: return bool from buildValue

f4f19c0

The error it previously returned was treated as a bool - so we should just return a bool instead.

danielnelson added this to the 1.12.0 milestone Jun 12, 2019

danielnelson added the performance problems with decreased performance or enhancements that improve performance label Jun 12, 2019

danielnelson approved these changes Jun 12, 2019

View reviewed changes

danielnelson merged commit 1a647fb into influxdata:master Jun 12, 2019

hwaastad pushed a commit to hwaastad/telegraf that referenced this pull request Jun 13, 2019

Improve performance of wavefront serializer (influxdata#5842)

40c90eb

bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019

Improve performance of wavefront serializer (influxdata#5842)

6a9dcaa

athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020

Improve performance of wavefront serializer (influxdata#5842)

b2f130a

idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020

Improve performance of wavefront serializer (influxdata#5842)

4a9cdb7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wavefront/serializer: improve performance by ~30% #5842

wavefront/serializer: improve performance by ~30% #5842

charlievieth commented May 12, 2019

charlievieth commented May 13, 2019 •

edited

Loading

danielnelson May 14, 2019

charlievieth May 16, 2019

danielnelson May 16, 2019

charlievieth May 17, 2019

charlievieth May 17, 2019

danielnelson May 17, 2019

charlievieth May 24, 2019

danielnelson May 15, 2019

charlievieth May 16, 2019

charlievieth May 16, 2019

danielnelson commented May 15, 2019

charlievieth May 17, 2019 •

edited

Loading

glinton May 17, 2019

puckpuck May 17, 2019

charlievieth May 24, 2019

puckpuck May 17, 2019

charlievieth May 24, 2019

puckpuck May 17, 2019

charlievieth May 24, 2019

		@@ -39,6 +41,12 @@ var tagValueReplacer = strings.NewReplacer("\"", "\\\"", "*", "-")

		var pathReplacer = strings.NewReplacer("_", ".")

		var bufPool = sync.Pool{

wavefront/serializer: improve performance by ~30% #5842

wavefront/serializer: improve performance by ~30% #5842

Conversation

charlievieth commented May 12, 2019

Required for all PRs:

charlievieth commented May 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnelson commented May 15, 2019

charlievieth May 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlievieth commented May 13, 2019 •

edited

Loading

charlievieth May 17, 2019 •

edited

Loading