Microoptimise matching #2005

redbaron · 2016-09-18T22:26:27Z

Series of patches with minor matching optimizations.

ab65905 - adds BenchmarkQueryRange to measure effect of future changes
e222d40 - avoids extra iteration over possible results, run matchers checks before adding to a result instead
1121364 - extracts logic which prepares candidate FPs into a separate method
9b15a3b - adds fpsForLabelMatchers and seriesForLabelMatchers methods and uses them where appropriate

Before (as of ab65905):

go test -bench BenchmarkQueryRange -run none -benchtime=10s -benchmem
    1000      14723524 ns/op     5013534 B/op      83059 allocs/op

After:

go test -bench BenchmarkQueryRange -run none -benchtime=10s -benchmem
    1000      11553489 ns/op     4688423 B/op      82596 allocs/op

redbaron · 2016-09-19T16:23:33Z

rebased

redbaron · 2016-09-20T10:09:44Z

Can someone have a look please? It is up to 30% speedup on QueryRange benchmark

beorn7 · 2016-09-20T10:18:34Z

It's on my review queue, I'm just totally overloaded, as usual...

beorn7

Thanks for doing this.

I have mostly just style nits.

beorn7 · 2016-09-22T17:16:06Z

storage/local/storage.go

@@ -417,11 +417,11 @@ func (s *MemorySeriesStorage) WaitForIndexing() {
 func (s *MemorySeriesStorage) LastSampleForLabelMatchers(_ context.Context, cutoff model.Time, matcherSets ...metric.LabelMatchers) (model.Vector, error) {
 	fps := map[model.Fingerprint]struct{}{}
 	for _, matchers := range matcherSets {
-		fpToMetric, err := s.metricsForLabelMatchers(cutoff, model.Latest, matchers...)
+		fps1, err := s.fpsForLabelMatchers(cutoff, model.Latest, matchers...)


Please use a more descriptive name than fps1. For example, you could call this fps, and the original fps above mergedFPs or something.

beorn7 · 2016-09-22T17:20:31Z

storage/local/storage.go

-	for fp := range fpToMetric {
-		it := s.preloadChunksForRange(fp, from, through)
+	iterators := make([]SeriesIterator, 0, len(fpSeriesPairs))
+	for _, v := range fpSeriesPairs {


How about pair or p instead of v?

beorn7 · 2016-09-22T17:20:57Z

storage/local/storage.go

-	for fp := range fpToMetric {
-		it := s.preloadChunksForInstant(fp, from, through)
+	iterators := make([]SeriesIterator, 0, len(fpSeriesPairs))
+	for _, v := range fpSeriesPairs {


beorn7 · 2016-09-22T17:21:42Z

storage/local/storage.go

@@ -563,55 +563,55 @@ func (s *MemorySeriesStorage) MetricsForLabelMatchers(
 	return metrics, nil
 }

-func (s *MemorySeriesStorage) metricsForLabelMatchers(
-	from, through model.Time,
+// returns candidate FPs for given matchers and remaining matchers to be checked


Start with "candidateFPsForLabelMatchers" and end with a period.

beorn7 · 2016-09-22T22:19:49Z

storage/local/storage.go

+		return nil, err
+	}
+
+FP_LOOP:


With my comment above, this comment finally makes sense. Use FPLoop:

beorn7 · 2016-09-22T22:30:29Z

storage/local/storage.go

 	if series == nil {
 		return nopIter
 	}
+
+	s.fpLocker.Lock(fp)


As discussed in the other PR, the performance gain of not going with defer is probably negligible in this context, and will go away with Go1.8 anyway. Better use the cleaner code here (i.e. unlock with defer as before).

beorn7 · 2016-09-22T22:31:03Z

storage/local/storage.go

 	if series == nil {
 		return nopIter
 	}
+
+	s.fpLocker.Lock(fp)


Same as above, unlock with defer.

beorn7 · 2016-09-22T22:31:53Z

storage/local/storage.go

@@ -888,40 +963,42 @@ func (s *MemorySeriesStorage) seriesForRange(
 }

 func (s *MemorySeriesStorage) preloadChunksForRange(
-	fp model.Fingerprint,
+	fpsPair fingerprintSeriesPair,


fpsPair is misleading. (Pair of two FPs?). I'd just call it pair.

beorn7 · 2016-09-22T22:32:04Z

storage/local/storage.go

 	return iter
 }

 func (s *MemorySeriesStorage) preloadChunksForInstant(
-	fp model.Fingerprint,
+	fpsPair fingerprintSeriesPair,


beorn7 · 2016-09-22T22:33:08Z

storage/local/test_helpers.go

@@ -65,3 +65,7 @@ func NewTestStorage(t testutil.T, encoding chunkEncoding) (*MemorySeriesStorage,

 	return storage, closer
 }
+
+func mkFpSeriesPair(s *MemorySeriesStorage, fp model.Fingerprint) fingerprintSeriesPair {


Capitalization nit: mkFPSeriesPair. But given the name of the returned type (where FP is not abbreviated), and given that "make" is rarle abrreviated "mk" in typical Go constructors, why not call it makeFingerprintSeriesPair after all?

beorn7 · 2016-09-28T11:43:05Z

@redbaron Ping?

redbaron · 2016-09-28T11:59:47Z

@beorn7 will be able to do sometime this week

Should be marginally faster and somewhat more GC friendly

…chers method No functional changes otherwise

redbaron · 2016-10-02T16:48:53Z

@beorn7 , made suggested changes

redbaron · 2016-10-04T08:33:51Z

sorry, I didn't know this PR is holding a release back. I'll keep this tab open and expect to be able to make fixes if needed within an hour.

beorn7 · 2016-10-05T13:44:33Z

storage/local/storage.go

+	}
+
+	result := []fingerprintSeriesPair{}
+FP_LOOP:


My comment here got lost: Don't use snake case in Go. Use something like FPLoop:

beorn7 · 2016-10-05T13:45:41Z

Thanks. The only missing nit is the one about the loop labels. See comments above. After that, we can merge.

beorn7 · 2016-10-05T14:12:03Z

Sorry for the nit, but ALLCAPS is also not very Go idiomatic. Please use FPLoop as label name.

These more specific methods have replaced `metricForLabelMatchers` in cases where its `map[fingerprint]metric` result type was not necessary or was used as an intermediate step Avoids duplicated calls to `seriesForRange` from `QueryRange` and `QueryInstant` methods.

redbaron · 2016-10-05T14:16:25Z

fixed, I wasn't reading careful enough :)

beorn7 · 2016-10-05T15:26:52Z

Thank you. Merging now.

redbaron · 2016-10-31T12:45:34Z

Do you still keep authors.md up to date?

beorn7 · 2016-10-31T15:40:47Z

In principle, yes. It definitely needs a major update. I had a script for it somewhere…

redbaron force-pushed the microoptimise-matching branch from 580cc3a to 7d660ce Compare September 19, 2016 16:23

redbaron force-pushed the microoptimise-matching branch from 7d660ce to 9b15a3b Compare September 20, 2016 10:23

beorn7 requested changes Sep 22, 2016

View reviewed changes

redbaron added 3 commits October 2, 2016 17:35

Added BenchmarkQueryRange

bedc0ed

Add metrics to result after checking all matchers

c048a0c

Should be marginally faster and somewhat more GC friendly

Extract initial FP candidate build logic into candidateFPsForLabelMat…

4978a65

…chers method No functional changes otherwise

redbaron force-pushed the microoptimise-matching branch 2 times, most recently from 4f50e9b to 0a16958 Compare October 2, 2016 16:43

beorn7 mentioned this pull request Oct 3, 2016

rules/manager.go: Fix race between reload and stop #2016

Merged

beorn7 reviewed Oct 5, 2016

View reviewed changes

redbaron force-pushed the microoptimise-matching branch from 0a16958 to fc50365 Compare October 5, 2016 13:51

redbaron force-pushed the microoptimise-matching branch from fc50365 to e6db9f8 Compare October 5, 2016 14:16

beorn7 merged commit 1e2f03f into prometheus:master Oct 5, 2016

redbaron deleted the microoptimise-matching branch October 5, 2016 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Microoptimise matching #2005

Microoptimise matching #2005

redbaron commented Sep 18, 2016 •

edited

Loading

redbaron commented Sep 19, 2016

redbaron commented Sep 20, 2016

beorn7 commented Sep 20, 2016

beorn7 left a comment

beorn7 Sep 22, 2016

beorn7 Sep 22, 2016

beorn7 Sep 22, 2016

beorn7 Sep 22, 2016

beorn7 Sep 22, 2016

beorn7 Oct 5, 2016

beorn7 Sep 22, 2016

beorn7 Sep 22, 2016

beorn7 Sep 22, 2016

beorn7 Sep 22, 2016

beorn7 Sep 22, 2016 •

edited

Loading

beorn7 commented Sep 28, 2016

redbaron commented Sep 28, 2016

redbaron commented Oct 2, 2016

redbaron commented Oct 4, 2016

beorn7 Oct 5, 2016

redbaron Oct 5, 2016

beorn7 commented Oct 5, 2016

beorn7 commented Oct 5, 2016

redbaron commented Oct 5, 2016

beorn7 commented Oct 5, 2016

redbaron commented Oct 31, 2016

beorn7 commented Oct 31, 2016

Microoptimise matching #2005

Microoptimise matching #2005

Conversation

redbaron commented Sep 18, 2016 • edited Loading

redbaron commented Sep 19, 2016

redbaron commented Sep 20, 2016

beorn7 commented Sep 20, 2016

beorn7 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

beorn7 commented Sep 28, 2016

redbaron commented Sep 28, 2016

redbaron commented Oct 2, 2016

redbaron commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented Oct 5, 2016

beorn7 commented Oct 5, 2016

redbaron commented Oct 5, 2016

beorn7 commented Oct 5, 2016

redbaron commented Oct 31, 2016

beorn7 commented Oct 31, 2016

redbaron commented Sep 18, 2016 •

edited

Loading

beorn7 Sep 22, 2016 •

edited

Loading