Improve performance of SetTags #1158

shanson7 · 2018-12-01T20:16:56Z

While debugging slowness in groupByTags, the performance of SetTags stood out. I don't believe this was the primary source of slowness in my case, but it certainly could be a contributing factor, since this is called for every series that is processed via render. Aside from replacing SplitN with IndexByte, I also only allocate a new map if the existing one is nil. I ran the benchmark in two modes, one where I leave the Tags and one where I nil them out.

Here's the benchcmp when s.Tags == nil

benchmark                             old ns/op     new ns/op     delta
BenchmarkSetTags_00tags_00chars-8     272           193           -29.04%
BenchmarkSetTags_20tags_32chars-8     3725          1798          -51.73%

benchmark                             old MB/s     new MB/s     speedup
BenchmarkSetTags_00tags_00chars-8     51.36        72.48        1.41x
BenchmarkSetTags_20tags_32chars-8     358.05       741.88       2.07x

benchmark                             old allocs     new allocs     delta
BenchmarkSetTags_00tags_00chars-8     3              2              -33.33%
BenchmarkSetTags_20tags_32chars-8     23             2              -91.30%

benchmark                             old bytes     new bytes     delta
BenchmarkSetTags_00tags_00chars-8     352           336           -4.55%
BenchmarkSetTags_20tags_32chars-8     2256          1264          -43.97%

and here is when we reuse the Tags:

benchmark                             old ns/op     new ns/op     delta
BenchmarkSetTags_00tags_00chars-8     272           99.3          -63.49%
BenchmarkSetTags_20tags_32chars-8     3725          2081          -44.13%

benchmark                             old MB/s     new MB/s     speedup
BenchmarkSetTags_00tags_00chars-8     51.36        140.92       2.74x
BenchmarkSetTags_20tags_32chars-8     358.05       640.78       1.79x

benchmark                             old allocs     new allocs     delta
BenchmarkSetTags_00tags_00chars-8     3              0              -100.00%
BenchmarkSetTags_20tags_32chars-8     23             0              -100.00%

benchmark                             old bytes     new bytes     delta
BenchmarkSetTags_00tags_00chars-8     352           0             -100.00%
BenchmarkSetTags_20tags_32chars-8     2256          0             -100.00%

With a lot of tags, the cost of clearing the map is higher than reallocating, but this was go 1.10.3 and I know go 1.11 has optimized map clearing.

Dieterbe · 2018-12-17T12:32:50Z

api/models/series_test.go

+		{
+			in: Series{
+				Target: "a;biglongtagkeyhere=andithasabiglongtagvaluetoo;c=d",
+			},


Dieterbe · 2018-12-17T12:35:27Z

api/models/series_test.go

+		in.Target = in.Target + randString(tagValueLength)
+	}
+
+	b.ReportAllocs()


interesting. everywhere else we assume if the user wants this, they use the -test.benchmem flag.
i'm hesitant of introducing this here and creating inconsistency with other tests.
is there a clear benefit?

Well, it allows benchmarks that know that allocations are something useful to benchmark to enable them independently of other benchmarks that may run that don't need them. I can remove it though.

i suppose it's a fair argument. but "allocations being useful to measure", doesn't that go for most benchmark code, other than perhaps very computationally expensive or io bound stuff maybe.
and i just use the alloc measures typically as a way to give color on changes in the time taken.

Right. I like allocation benchmarks on certain code, because it's less subject to noisy neighbor with local benchmarks. I removed it in my last commit though.

fyi re noisy neighbours i use cpu isolation.
see https://gist.github.com/Dieterbe/a52c95a9603507670eb39274544ee1a8

Dieterbe · 2018-12-17T12:38:04Z

api/models/series.go

+
+	if s.Tags == nil {
+		// +1 for the name tag
+		s.Tags = make(map[string]string, numTags)


don't need to use numTags+1 per the comment?

Good catch. Must have gotten lost during some of my benchmark rounds

Dieterbe · 2018-12-17T12:41:54Z

With a lot of tags, the cost of clearing the map is higher than reallocating, but this was go 1.10.3 and I know go 1.11 has optimized map clearing.

correct. https://go-review.googlesource.com/c/go/+/110055 should pay off here.

Dieterbe · 2018-12-17T12:43:38Z

api/models/series.go

+	}
+
+	index := strings.IndexByte(s.Target, ';')
+	name := s.Target[0:index]


hmm. surprised our qa gofmt script doesn't complain about the needless 0:

Dieterbe · 2018-12-17T12:55:23Z

api/models/series.go

-	s.Tags["name"] = tagSplits[0]
+
+	// Do this last to overwrite any invalid "name" tag that might be preset
+	s.Tags["name"] = name


what do you mean with invalid? one specified by the user? maybe we should refer to those as 'illegal' or even clarify as overwrite any "name" tag that may have been illegally specified in the series tags. or actually drop the 'illegal' because i don't think it's written anywhere that it's illegal.

Dieterbe · 2018-12-17T13:02:57Z

api/models/series_test.go

+	for i := 0; i < numTags; i++ {
+		in.Target = in.Target + ";" + randString(tagKeyLength)
+		in.Target = in.Target + "="
+		in.Target = in.Target + randString(tagValueLength)


why not simplify this to

in.Target += ";" + randString(tagKeyLength) + "=" + randString(tagValueLength)

also, we could use https://golang.org/pkg/strings/#Builder here

@shanson7 is it deliberate that you are still doing in.Target = in.Target + ... rather then += ?

Dieterbe · 2018-12-17T13:04:55Z

I ran the benchmark in two modes, one where I leave the Tags and one where I nil them out.

did you do this by uncommenting the nil setting line? instead of that, perhaps we should just have 2 separate benchmarks? to simplify reproducing the stats

Dieterbe

looks good, but needs some tweaks.

shanson7 · 2018-12-17T15:55:59Z

Upgraded my local golang to 1.11.4 and now the map clearing approach shows it's benefit:

BenchmarkSetTags_20tags_32chars-8          	10000000	      1666 ns/op	 800.46 MB/s	    1264 B/op	       2 allocs/op
BenchmarkSetTags_20tags_32chars_reused-8   	20000000	      1096 ns/op	1216.87 MB/s	     288 B/op	       1 allocs/op

shanson7 mentioned this pull request Dec 6, 2018

groupByTags Performance improvements + fix setting consolidator per group #1165

Merged

shanson7 added 2 commits December 6, 2018 09:42

Add tests and benchmarks for SetTags

7387d22

SetTags - Replace SplitN calls with explicit tokenization

832ccf3

shanson7 force-pushed the improveSetTags branch from 978c8b5 to 832ccf3 Compare December 6, 2018 14:42

Dieterbe reviewed Dec 17, 2018

View reviewed changes

api/models/series_test.go

{

in: Series{

Target: "a;biglongtagkeyhere=andithasabiglongtagvaluetoo;c=d",

},

Copy link

Contributor

Dieterbe Dec 17, 2018

Choose a reason for hiding this comment

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

👍

Dieterbe reviewed Dec 17, 2018

View reviewed changes

Dieterbe suggested changes Dec 17, 2018

View reviewed changes

shanson7 added 2 commits December 17, 2018 10:47

Minor cleanup of SetTags

b637429

Add flag for SetTags reusability

35b1e5d

Use += to build test Target

b6a848d

Dieterbe approved these changes Dec 20, 2018

View reviewed changes

Dieterbe merged commit 36065b4 into grafana:master Dec 20, 2018

Dieterbe added this to the vnext milestone Feb 11, 2019

shanson7 deleted the improveSetTags branch March 6, 2019 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of SetTags #1158

Improve performance of SetTags #1158

shanson7 commented Dec 1, 2018

Dieterbe Dec 17, 2018

Dieterbe Dec 17, 2018

shanson7 Dec 17, 2018

Dieterbe Dec 17, 2018

shanson7 Dec 17, 2018

Dieterbe Dec 17, 2018

Dieterbe Dec 17, 2018

Dieterbe Dec 17, 2018

shanson7 Dec 17, 2018

Dieterbe commented Dec 17, 2018

Dieterbe Dec 17, 2018

Dieterbe Dec 17, 2018

Dieterbe Dec 17, 2018

Dieterbe Dec 19, 2018

shanson7 Dec 19, 2018

Dieterbe commented Dec 17, 2018

Dieterbe left a comment

shanson7 commented Dec 17, 2018

Improve performance of SetTags #1158

Improve performance of SetTags #1158

Conversation

shanson7 commented Dec 1, 2018

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

Dieterbe commented Dec 17, 2018

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

Dieterbe commented Dec 17, 2018

Dieterbe left a comment

Choose a reason for hiding this comment

shanson7 commented Dec 17, 2018