refactor(sumologicextension): use bytes slices and strings.Builder to decrease allocations #530

pmalek-sumo · 2022-04-05T13:23:27Z

$ benchcmp main.txt pr.txt
benchmark                                          old ns/op     new ns/op     delta
Benchmark_PrometheusFormatter_Metric2String-16     66901         53862         -19.49%
Benchmark_PrometheusFormatter_Metric2String-16     65607         53773         -18.04%
Benchmark_PrometheusFormatter_Metric2String-16     65205         54047         -17.11%

benchmark                                          old allocs     new allocs     delta
Benchmark_PrometheusFormatter_Metric2String-16     696            512            -26.44%
Benchmark_PrometheusFormatter_Metric2String-16     696            512            -26.44%
Benchmark_PrometheusFormatter_Metric2String-16     696            512            -26.44%

benchmark                                          old bytes     new bytes     delta
Benchmark_PrometheusFormatter_Metric2String-16     20326         15793         -22.30%
Benchmark_PrometheusFormatter_Metric2String-16     20341         15785         -22.40%
Benchmark_PrometheusFormatter_Metric2String-16     20315         15811         -22.17%

swiatekm · 2022-04-05T15:36:54Z

Have you benchmarked how much of the improvement comes from introducing StringBuilder vs the other changes? The StringBuilder code is much uglier, and intuitively it shouldn't be that much faster than fmt.Sprintf for short strings.

pmalek-sumo · 2022-04-05T16:33:02Z

Have you benchmarked how much of the improvement comes from introducing StringBuilder vs the other changes? The StringBuilder code is much uglier, and intuitively it shouldn't be that much faster than fmt.Sprintf for short strings.

Just those 2 changes introducing strings.Builder in tags2String() contribute ~~20-25% of allocs and similarly in time:

benchmark                                          old ns/op     new ns/op     delta
Benchmark_PrometheusFormatter_Metric2String-16     49478         59523         +20.30%
Benchmark_PrometheusFormatter_Metric2String-16     50142         64352         +28.34%
Benchmark_PrometheusFormatter_Metric2String-16     49223         59849         +21.59%

benchmark                                          old allocs     new allocs     delta
Benchmark_PrometheusFormatter_Metric2String-16     538            674            +25.28%
Benchmark_PrometheusFormatter_Metric2String-16     538            674            +25.28%
Benchmark_PrometheusFormatter_Metric2String-16     538            674            +25.28%

benchmark                                          old bytes     new bytes     delta
Benchmark_PrometheusFormatter_Metric2String-16     16950         19635         +15.84%
Benchmark_PrometheusFormatter_Metric2String-16     16923         19645         +16.08%
Benchmark_PrometheusFormatter_Metric2String-16     16938         19654         +16.03%

pmalek-sumo · 2022-04-06T08:56:34Z

Another comparison:

	returnValue := make([]string, 0, length)
	mergedAttributes.Range(func(k string, v pdata.AttributeValue) bool {
		key := f.sanitizeKeyBytes([]byte(k))
		value := f.sanitizeValue(v.AsString())

		returnValue = append(
			returnValue,
			fmt.Sprintf(
				`%s="%s"`,
				key, value,
			),
		)
		return true
	})

	return prometheusTags(fmt.Sprintf("{%s}", strings.Join(returnValue, ",")))

goos: darwin
goarch: amd64
pkg: github.com/SumoLogic/sumologic-otel-collector/pkg/exporter/sumologicexporter
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
Benchmark_PrometheusFormatter_Metric2String
Benchmark_PrometheusFormatter_Metric2String-16             20386             58591 ns/op           19630 B/op        674 allocs/op
Benchmark_PrometheusFormatter_Metric2String-16             20182             59102 ns/op           19668 B/op        674 allocs/op
Benchmark_PrometheusFormatter_Metric2String-16             19946             61476 ns/op           19647 B/op        674 allocs/op
PASS
ok      github.com/SumoLogic/sumologic-otel-collector/pkg/exporter/sumologicexporter    5.821s

using strings.Builder

	returnValue := make([]string, 0, length)
	mergedAttributes.Range(func(k string, v pdata.AttributeValue) bool {
		key := f.sanitizeKeyBytes([]byte(k))
		value := f.sanitizeValue(v.AsString())

		sb := strings.Builder{}
		sb.Grow(len(key) + len(value) + 1 + 2)
		sb.Write(key)
		sb.WriteRune('=')
		sb.WriteRune('"')
		sb.WriteString(value)
		sb.WriteRune('"')

		returnValue = append(
			returnValue,
			fmt.Sprintf(
				"%s", sb.String(),
			),
		)
		return true
	})

	ret := strings.Join(returnValue, ",")
	sb := strings.Builder{}
	sb.Grow(2 + len(ret))
	sb.WriteRune('{')
	sb.WriteString(ret)
	sb.WriteRune('}')

	return prometheusTags(sb.String())

goos: darwin
goarch: amd64
pkg: github.com/SumoLogic/sumologic-otel-collector/pkg/exporter/sumologicexporter
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
Benchmark_PrometheusFormatter_Metric2String
Benchmark_PrometheusFormatter_Metric2String-16             21086             55722 ns/op           18930 B/op        658 allocs/op
Benchmark_PrometheusFormatter_Metric2String-16             21694             57132 ns/op           18978 B/op        658 allocs/op
Benchmark_PrometheusFormatter_Metric2String-16             21644             58168 ns/op           18953 B/op        658 allocs/op
PASS
ok      github.com/SumoLogic/sumologic-otel-collector/pkg/exporter/sumologicexporter    5.771s

using strings.Builder and no fmt

	returnValue := make([]string, 0, length)
	mergedAttributes.Range(func(k string, v pdata.AttributeValue) bool {
		key := f.sanitizeKeyBytes([]byte(k))
		value := f.sanitizeValue(v.AsString())

		sb := strings.Builder{}
		sb.Grow(len(key) + len(value) + 1 + 2)
		sb.Write(key)
		sb.WriteRune('=')
		sb.WriteRune('"')
		sb.WriteString(value)
		sb.WriteRune('"')

		returnValue = append(
			returnValue,
			sb.String(),
		)
		return true
	})

	ret := strings.Join(returnValue, ",")
	sb := strings.Builder{}
	sb.Grow(2 + len(ret))
	sb.WriteRune('{')
	sb.WriteString(ret)
	sb.WriteRune('}')

	return prometheusTags(sb.String())

goos: darwin
goarch: amd64
pkg: github.com/SumoLogic/sumologic-otel-collector/pkg/exporter/sumologicexporter
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
Benchmark_PrometheusFormatter_Metric2String
Benchmark_PrometheusFormatter_Metric2String-16             24160             51110 ns/op           16954 B/op        538 allocs/op
Benchmark_PrometheusFormatter_Metric2String-16             23751             50131 ns/op           16942 B/op        538 allocs/op
Benchmark_PrometheusFormatter_Metric2String-16             24018             49481 ns/op           16944 B/op        538 allocs/op
PASS
ok      github.com/SumoLogic/sumologic-otel-collector/pkg/exporter/sumologicexporter    5.389s

swiatekm · 2022-04-06T09:03:02Z

Pretty significant difference after all, then. Ok, I'm in favor of using the StringBuilders, let's just make it clear why we're doing it this way with comments and possibly moving the key=value formatting to a separate function.

pmalek-sumo · 2022-04-06T09:22:00Z

PTAL @swiatekm-sumo

swiatekm

LGTM. Do you reckon we should add separate unit tests for stringsJoinAndSurround or are we sufficiently covered by existing ones?

pmalek-sumo · 2022-04-06T11:14:25Z

LGTM. Do you reckon we should add separate unit tests for stringsJoinAndSurround or are we sufficiently covered by existing ones?

We could in theory. OTOH it's tested indirectly in other tests.

pdelewski · 2022-04-07T11:31:09Z

pkg/exporter/sumologicexporter/prometheus_formatter.go

+	// allocations.
+	sb := strings.Builder{}
+	// We preallocate space for key, value, equal sign and quotes.
+	sb.Grow(len(key) + len(value) + 1 + 2)


I think that it would be more readable to not use hardcoded values in this case
const equalSign string = "="
const quoteSign string = "\""
sb.Grow(len(key) + len(value) + len(equalSign) + len(quoteSign) + len(quoteSign)
This still does not guarantee consistency of allocated buffer and real space used to contain all characters.
One example of fixing it, would be, hiding string builder under higher level builder api with at least two functions for one allocating space and second for building string which would check (at runtime) consistency between allocated space and really used one.

What would be the outcome of that check in your view @pdelewski? Let's say you try to write more data to the buffer than what is allocated, StringBuilder will automatically allocate more space. Should we then panic, print a warning, or something else, instead?

It depends if that preallocation is critical for us and if that's the case we can return error or panic, however I don't want to over engineer that, so maybe it would be good to check what is benefit of preallocating buffer

We can benchmark this with and without the preallocation if we really want to find the performance difference between the 2 approaches but @swiatekm-sumo has a point that strings.Builder does re-allocate when needed and it's its feature so there's really no point to work around that I'd say.

To address your suggestion @pdelewski we could make those consts but then what type should they be? If we stick to runes then they are always of length 1 and you cannot take rune's length i.e.

r := '[' l := len(r) // <- this won't compile

In that case we'd need to resort to strings but then we'd have to measure if it makes sense from performance standpoint (probably it does).

Do we need to use runes? Are they better, faster in this particular case?

It looks like it's basically the same:

This PR:

goos: darwin goarch: amd64 pkg: github.com/SumoLogic/sumologic-otel-collector/pkg/exporter/sumologicexporter cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz Benchmark_PrometheusFormatter_Metric2String Benchmark_PrometheusFormatter_Metric2String-16 22712 51177 ns/op 15821 B/op 512 allocs/op Benchmark_PrometheusFormatter_Metric2String-16 23038 50360 ns/op 15791 B/op 512 allocs/op Benchmark_PrometheusFormatter_Metric2String-16 23560 51604 ns/op 15810 B/op 512 allocs/op PASS ok github.com/SumoLogic/sumologic-otel-collector/pkg/exporter/sumologicexporter 5.578s

This approach:

sb.WriteString("=") sb.WriteString(`"`) sb.WriteString(value) sb.WriteString(`"`)

goos: darwin goarch: amd64 pkg: github.com/SumoLogic/sumologic-otel-collector/pkg/exporter/sumologicexporter cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz Benchmark_PrometheusFormatter_Metric2String Benchmark_PrometheusFormatter_Metric2String-16 23334 50641 ns/op 15804 B/op 512 allocs/op Benchmark_PrometheusFormatter_Metric2String-16 24022 51114 ns/op 15809 B/op 512 allocs/op Benchmark_PrometheusFormatter_Metric2String-16 24036 49831 ns/op 15811 B/op 512 allocs/op

@pdelewski Please take a look now.

I would stick to string if you don't mind (it's simpler to read as we don't need to mix different types and we avoid questions why runes are there)

Please post your review then :) 👍 / 👎

…enchmark

pkg/exporter/sumologicexporter/prometheus_formatter.go

… decrease allocations

… decrease allocations (#530) * refactor(sumologicextension): add prometheus formatter metrc2string benchmark * refactor(sumologicextension): use bytes slices and strings.Builder to decrease allocations

pmalek-sumo self-assigned this Apr 5, 2022

github-actions bot added the go label Apr 5, 2022

pmalek-sumo force-pushed the optimize-prometheus-formatter branch from cc9da42 to 331d639 Compare April 5, 2022 14:06

pmalek-sumo marked this pull request as ready for review April 5, 2022 14:06

pmalek-sumo requested a review from a team as a code owner April 5, 2022 14:06

pmalek-sumo force-pushed the optimize-prometheus-formatter branch from 331d639 to ec60339 Compare April 5, 2022 14:34

pmalek-sumo force-pushed the optimize-prometheus-formatter branch from ec60339 to b3fc6e9 Compare April 5, 2022 16:33

pmalek-sumo force-pushed the optimize-prometheus-formatter branch 2 times, most recently from c73f7b6 to 46df176 Compare April 6, 2022 09:16

swiatekm approved these changes Apr 6, 2022

View reviewed changes

pmalek-sumo force-pushed the optimize-prometheus-formatter branch from 46df176 to d8fe2f6 Compare April 7, 2022 09:05

sumo-drosiek approved these changes Apr 7, 2022

View reviewed changes

pdelewski reviewed Apr 7, 2022

View reviewed changes

refactor(sumologicextension): add prometheus formatter metrc2string b…

582ace4

…enchmark

pmalek-sumo force-pushed the optimize-prometheus-formatter branch from d8fe2f6 to 35271f1 Compare April 7, 2022 13:39

pdelewski reviewed Apr 7, 2022

View reviewed changes

pkg/exporter/sumologicexporter/prometheus_formatter.go Outdated Show resolved Hide resolved

refactor(sumologicextension): use bytes slices and strings.Builder to…

7371688

… decrease allocations

pmalek-sumo force-pushed the optimize-prometheus-formatter branch from 35271f1 to 7371688 Compare April 7, 2022 14:31

pdelewski approved these changes Apr 7, 2022

View reviewed changes

pmalek-sumo merged commit 7e4d3df into main Apr 7, 2022

pmalek-sumo deleted the optimize-prometheus-formatter branch April 7, 2022 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(sumologicextension): use bytes slices and strings.Builder to decrease allocations #530

refactor(sumologicextension): use bytes slices and strings.Builder to decrease allocations #530

pmalek-sumo commented Apr 5, 2022 •

edited

Loading

swiatekm commented Apr 5, 2022

pmalek-sumo commented Apr 5, 2022

pmalek-sumo commented Apr 6, 2022

swiatekm commented Apr 6, 2022

pmalek-sumo commented Apr 6, 2022

swiatekm left a comment

pmalek-sumo commented Apr 6, 2022

pdelewski Apr 7, 2022

swiatekm Apr 7, 2022

pdelewski Apr 7, 2022 •

edited

Loading

pmalek-sumo Apr 7, 2022

pdelewski Apr 7, 2022

pmalek-sumo Apr 7, 2022

pmalek-sumo Apr 7, 2022

pdelewski Apr 7, 2022

pmalek-sumo Apr 7, 2022

refactor(sumologicextension): use bytes slices and strings.Builder to decrease allocations #530

refactor(sumologicextension): use bytes slices and strings.Builder to decrease allocations #530

Conversation

pmalek-sumo commented Apr 5, 2022 • edited Loading

swiatekm commented Apr 5, 2022

pmalek-sumo commented Apr 5, 2022

pmalek-sumo commented Apr 6, 2022

swiatekm commented Apr 6, 2022

pmalek-sumo commented Apr 6, 2022

swiatekm left a comment

Choose a reason for hiding this comment

pmalek-sumo commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdelewski Apr 7, 2022 • 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

pmalek-sumo commented Apr 5, 2022 •

edited

Loading

pdelewski Apr 7, 2022 •

edited

Loading