Optimize SeriesGrouper & aggregators.merge #8391

phemmer · 2020-11-11T14:10:57Z

This introduces some optimizations for use with the aggregators.merge plugin. In my current usage of telegraf, it's heavily bottle-necking on this plugin.

The previous implementation of SeriesGrouper required breaking a metric object apart into its constituents, converting tags and keys into unoptimized maps, only to have it put them back together into another metric object. This resulted in a significant performance overhead. This overhead was further compounded when the number of fields was large.

This change adds a new AddMetric method to SeriesGrouper which preserves the metric object and removes the back-and-forth conversion.

Additionlly the method used for calculating the metric's hash was switched to use maphash, which is optimized for this case.

Benchmarks

Before:

BenchmarkMergeOne-16          106012	     11790 ns/op
BenchmarkMergeTwo-16           48529	     24819 ns/op
BenchmarkGroupID-16           780018	      1608 ns/op

After:

BenchmarkMergeOne-16          907093	      1173 ns/op
BenchmarkMergeTwo-16          508321	      2168 ns/op
BenchmarkGroupID-16         11217788	      99.4 ns/op

Note the BenchmarkGroupID isn't a direct apples-to-apples comparison, since the old method had the sort code present, and the new one does not. However earlier in my development, switching just the hash, while keeping all the other code resulted in about a 2x performance increase (831 ns/op).

Also since the vast majority of metrics are likely not to result in any mergers, a further optimization might be to utilize copy-on-write. Meaning use the input metric without copying it, and if a second metric is merged into it, then copy it. However the current change is a good first step, and that can be investigated later.

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

phemmer · 2020-11-11T15:03:24Z

I haven't a clue whats up with the test failure.

macdeps:

github.com/influxdata/telegraf/metric imports
	hash/maphash: malformed module path "hash/maphash": missing dot in first path element

... No it's not

reimda · 2020-11-11T21:42:40Z

I took a look at the macdeps failure but I haven't figured out yet why it's failing. First I reran it and it failed again. I noticed when rerunning it that macdeps installs go version 1.13.6 when it intends to install the latest go package from homebrew. Maybe 'go mod tidy' fails only on go 1.13.6? (The other CI steps use 1.14 or 1.15.)

phemmer · 2020-11-11T21:57:42Z

Ah, that would explain it. maphash was introduced in 1.14.

Given that go.mod has the version set to 1.15, should we not even be testing against lower versions?

reimda · 2020-11-11T22:08:34Z

I just noticed that hash/maphash is new in 1.14 too.
https://golang.org/doc/go1.14#hash/maphash

Telegraf is currently supporting go 1.14 and 1.15 even though go.mod has the version set to 1.15. Fortunately the 1.14 CI steps are working even with the go.mod version set to 1.15.

To fix this failure we'll have to fix the macdeps job to use a newer golang version. I think it's just a matter of updating .circleci/config.yml to run 'brew update' before running 'brew install go'. Could you make that change and try it out in this PR?

reimda · 2020-11-11T22:39:56Z

It looks like that worked. Unfortunately 'brew update' took over four minutes to run. We may need to look for a faster way to get an up to date golang install, but you don't need to worry about that for this PR. Thanks for your help debugging CI!

phemmer · 2020-11-11T22:46:53Z

Yay.

We may need to look for a faster way to get an up to date golang install

Maybe it should use the official package/archive. I know I've seen issues on the golang issue tracker where they won't support people using the homebrew package, and tell these people to go reproduce issues using the official package. I'm unfamiliar with the background, but some googling suggests that homebrew does janky things with the package.

metric/series_grouper.go

srebhan

Code looks good. Maybe we could move the seed init to the NewSeriesGrouper() call? This would get rid of the global variable and generates an individual seed per grouper if I'm not mistaken...

Could we also move the CircleCI fix out here? It's just a minor thing, but nobody will afterwards search this PR for that... :-)

reimda · 2020-12-04T23:56:38Z

I updated the golang version for mac ci in #8516. Could you remove the 'brew update' from .circleci/config.yml and rebase this PR? Thanks!

srebhan · 2020-12-18T15:13:38Z

@phemmer any news?

The previous implementation of SeriesGrouper required breaking a metric object apart into its constituents, converting tags and keys into unoptimized maps, only to have it put them back together into another metric object. This resulted in a significant performance overhead. This overhead was further compounded when the number of fields was large. This change adds a new AddMetric method to SeriesGrouper which preserves the metric object and removes the back-and-forth conversion. Additionlly the method used for calculating the metric's hash was switched to use maphash, which is optimized for this case. ---- Benchmarks Before: BenchmarkMergeOne-16 106012 11790 ns/op BenchmarkMergeTwo-16 48529 24819 ns/op BenchmarkGroupID-16 780018 1608 ns/op After: BenchmarkMergeOne-16 907093 1173 ns/op BenchmarkMergeTwo-16 508321 2168 ns/op BenchmarkGroupID-16 11217788 99.4 ns/op

phemmer · 2020-12-21T17:53:14Z

Updated to address comments.

srebhan

Looks good to me.

ssoroka · 2021-01-07T16:38:35Z

Great work, thanks!

The previous implementation of SeriesGrouper required breaking a metric object apart into its constituents, converting tags and keys into unoptimized maps, only to have it put them back together into another metric object. This resulted in a significant performance overhead. This overhead was further compounded when the number of fields was large. This change adds a new AddMetric method to SeriesGrouper which preserves the metric object and removes the back-and-forth conversion. Additionlly the method used for calculating the metric's hash was switched to use maphash, which is optimized for this case. ---- Benchmarks Before: BenchmarkMergeOne-16 106012 11790 ns/op BenchmarkMergeTwo-16 48529 24819 ns/op BenchmarkGroupID-16 780018 1608 ns/op After: BenchmarkMergeOne-16 907093 1173 ns/op BenchmarkMergeTwo-16 508321 2168 ns/op BenchmarkGroupID-16 11217788 99.4 ns/op (cherry picked from commit 910b726)

The previous implementation of SeriesGrouper required breaking a metric object apart into its constituents, converting tags and keys into unoptimized maps, only to have it put them back together into another metric object. This resulted in a significant performance overhead. This overhead was further compounded when the number of fields was large. This change adds a new AddMetric method to SeriesGrouper which preserves the metric object and removes the back-and-forth conversion. Additionlly the method used for calculating the metric's hash was switched to use maphash, which is optimized for this case. ---- Benchmarks Before: BenchmarkMergeOne-16 106012 11790 ns/op BenchmarkMergeTwo-16 48529 24819 ns/op BenchmarkGroupID-16 780018 1608 ns/op After: BenchmarkMergeOne-16 907093 1173 ns/op BenchmarkMergeTwo-16 508321 2168 ns/op BenchmarkGroupID-16 11217788 99.4 ns/op

phemmer force-pushed the optimize-series-grouper branch 5 times, most recently from 266b36a to dc0648c Compare November 11, 2020 14:57

phemmer mentioned this pull request Nov 11, 2020

PostgreSQL output plugin #3428

Closed

3 tasks

phemmer force-pushed the optimize-series-grouper branch from dc0648c to de28732 Compare November 11, 2020 21:51

sjwang90 added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Nov 24, 2020

srebhan reviewed Nov 30, 2020

View reviewed changes

metric/series_grouper.go Outdated Show resolved Hide resolved

srebhan reviewed Nov 30, 2020

View reviewed changes

srebhan self-assigned this Nov 30, 2020

p-zak mentioned this pull request Dec 2, 2020

Increase build verbosity with go mod download -x #8496

Merged

3 tasks

srebhan mentioned this pull request Dec 2, 2020

Add node groups to opcua input plugin #8389

Merged

reimda mentioned this pull request Dec 4, 2020

Update circleci mac golang version #8516

Merged

phemmer force-pushed the optimize-series-grouper branch from 90cd3e8 to 3e1a2a7 Compare December 21, 2020 17:53

srebhan approved these changes Dec 23, 2020

View reviewed changes

srebhan requested a review from ssoroka December 23, 2020 21:59

srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jan 6, 2021

ssoroka approved these changes Jan 7, 2021

View reviewed changes

ssoroka merged commit 910b726 into influxdata:master Jan 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize SeriesGrouper & aggregators.merge #8391

Optimize SeriesGrouper & aggregators.merge #8391

phemmer commented Nov 11, 2020 •

edited

Loading

phemmer commented Nov 11, 2020

reimda commented Nov 11, 2020

phemmer commented Nov 11, 2020

reimda commented Nov 11, 2020

reimda commented Nov 11, 2020

phemmer commented Nov 11, 2020

srebhan left a comment

reimda commented Dec 4, 2020

srebhan commented Dec 18, 2020

phemmer commented Dec 21, 2020

srebhan left a comment

ssoroka commented Jan 7, 2021

Optimize SeriesGrouper & aggregators.merge #8391

Optimize SeriesGrouper & aggregators.merge #8391

Conversation

phemmer commented Nov 11, 2020 • edited Loading

Required for all PRs:

phemmer commented Nov 11, 2020

reimda commented Nov 11, 2020

phemmer commented Nov 11, 2020

reimda commented Nov 11, 2020

reimda commented Nov 11, 2020

phemmer commented Nov 11, 2020

srebhan left a comment

Choose a reason for hiding this comment

reimda commented Dec 4, 2020

srebhan commented Dec 18, 2020

phemmer commented Dec 21, 2020

srebhan left a comment

Choose a reason for hiding this comment

ssoroka commented Jan 7, 2021

phemmer commented Nov 11, 2020 •

edited

Loading