From 4708915ac6ef3f1a8b7c348c6463c08f098a1e0a Mon Sep 17 00:00:00 2001 From: Brian Brazil Date: Wed, 1 Jan 2020 11:38:01 +0000 Subject: [PATCH] Replace StringTuples with []string Benchmarks show slight cpu/allocs improvements. benchmark old ns/op new ns/op delta BenchmarkPostingsForMatchers/Head/n="1"-4 269978625 235305110 -12.84% BenchmarkPostingsForMatchers/Head/n="1",j="foo"-4 129739974 121646193 -6.24% BenchmarkPostingsForMatchers/Head/j="foo",n="1"-4 123826274 122056253 -1.43% BenchmarkPostingsForMatchers/Head/n="1",j!="foo"-4 126962188 130038235 +2.42% BenchmarkPostingsForMatchers/Head/i=~".*"-4 6423653989 5991126455 -6.73% BenchmarkPostingsForMatchers/Head/i=~".+"-4 6934647521 7033370634 +1.42% BenchmarkPostingsForMatchers/Head/i=~""-4 1177781285 1121497736 -4.78% BenchmarkPostingsForMatchers/Head/i!=""-4 7033680256 7246094991 +3.02% BenchmarkPostingsForMatchers/Head/n="1",i=~".*",j="foo"-4 293702332 287440212 -2.13% BenchmarkPostingsForMatchers/Head/n="1",i=~".*",i!="2",j="foo"-4 307628268 307039964 -0.19% BenchmarkPostingsForMatchers/Head/n="1",i!=""-4 512247746 480003862 -6.29% BenchmarkPostingsForMatchers/Head/n="1",i!="",j="foo"-4 361199794 367066917 +1.62% BenchmarkPostingsForMatchers/Head/n="1",i=~".+",j="foo"-4 478863761 476037784 -0.59% BenchmarkPostingsForMatchers/Head/n="1",i=~"1.+",j="foo"-4 103394659 102902098 -0.48% BenchmarkPostingsForMatchers/Head/n="1",i=~".+",i!="2",j="foo"-4 482552781 475453903 -1.47% BenchmarkPostingsForMatchers/Head/n="1",i=~".+",i!~"2.*",j="foo"-4 559257389 589297047 +5.37% BenchmarkPostingsForMatchers/Block/n="1"-4 36492 37012 +1.42% BenchmarkPostingsForMatchers/Block/n="1",j="foo"-4 557788 611903 +9.70% BenchmarkPostingsForMatchers/Block/j="foo",n="1"-4 554443 573814 +3.49% BenchmarkPostingsForMatchers/Block/n="1",j!="foo"-4 553227 553826 +0.11% BenchmarkPostingsForMatchers/Block/i=~".*"-4 113855090 111707221 -1.89% BenchmarkPostingsForMatchers/Block/i=~".+"-4 133994674 136520728 +1.89% BenchmarkPostingsForMatchers/Block/i=~""-4 38138091 36299898 -4.82% BenchmarkPostingsForMatchers/Block/i!=""-4 28861213 27396723 -5.07% BenchmarkPostingsForMatchers/Block/n="1",i=~".*",j="foo"-4 112699941 110853868 -1.64% BenchmarkPostingsForMatchers/Block/n="1",i=~".*",i!="2",j="foo"-4 113198026 111389742 -1.60% BenchmarkPostingsForMatchers/Block/n="1",i!=""-4 28994069 27363804 -5.62% BenchmarkPostingsForMatchers/Block/n="1",i!="",j="foo"-4 29709406 28589223 -3.77% BenchmarkPostingsForMatchers/Block/n="1",i=~".+",j="foo"-4 134695119 135736971 +0.77% BenchmarkPostingsForMatchers/Block/n="1",i=~"1.+",j="foo"-4 26783286 25826928 -3.57% BenchmarkPostingsForMatchers/Block/n="1",i=~".+",i!="2",j="foo"-4 134733254 134116739 -0.46% BenchmarkPostingsForMatchers/Block/n="1",i=~".+",i!~"2.*",j="foo"-4 160713937 158802768 -1.19% benchmark old allocs new allocs delta BenchmarkPostingsForMatchers/Head/n="1"-4 36 36 +0.00% BenchmarkPostingsForMatchers/Head/n="1",j="foo"-4 38 38 +0.00% BenchmarkPostingsForMatchers/Head/j="foo",n="1"-4 38 38 +0.00% BenchmarkPostingsForMatchers/Head/n="1",j!="foo"-4 42 40 -4.76% BenchmarkPostingsForMatchers/Head/i=~".*"-4 61 59 -3.28% BenchmarkPostingsForMatchers/Head/i=~".+"-4 100088 100087 -0.00% BenchmarkPostingsForMatchers/Head/i=~""-4 100053 100051 -0.00% BenchmarkPostingsForMatchers/Head/i!=""-4 100087 100085 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i=~".*",j="foo"-4 44 42 -4.55% BenchmarkPostingsForMatchers/Head/n="1",i=~".*",i!="2",j="foo"-4 50 48 -4.00% BenchmarkPostingsForMatchers/Head/n="1",i!=""-4 100076 100074 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i!="",j="foo"-4 100077 100075 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i=~".+",j="foo"-4 100077 100074 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i=~"1.+",j="foo"-4 11167 11165 -0.02% BenchmarkPostingsForMatchers/Head/n="1",i=~".+",i!="2",j="foo"-4 100082 100080 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i=~".+",i!~"2.*",j="foo"-4 111265 111261 -0.00% BenchmarkPostingsForMatchers/Block/n="1"-4 6 6 +0.00% BenchmarkPostingsForMatchers/Block/n="1",j="foo"-4 11 11 +0.00% BenchmarkPostingsForMatchers/Block/j="foo",n="1"-4 11 11 +0.00% BenchmarkPostingsForMatchers/Block/n="1",j!="foo"-4 15 13 -13.33% BenchmarkPostingsForMatchers/Block/i=~".*"-4 12 10 -16.67% BenchmarkPostingsForMatchers/Block/i=~".+"-4 100040 100038 -0.00% BenchmarkPostingsForMatchers/Block/i=~""-4 100045 100043 -0.00% BenchmarkPostingsForMatchers/Block/i!=""-4 100041 100039 -0.00% BenchmarkPostingsForMatchers/Block/n="1",i=~".*",j="foo"-4 17 15 -11.76% BenchmarkPostingsForMatchers/Block/n="1",i=~".*",i!="2",j="foo"-4 23 21 -8.70% BenchmarkPostingsForMatchers/Block/n="1",i!=""-4 100046 100044 -0.00% BenchmarkPostingsForMatchers/Block/n="1",i!="",j="foo"-4 100050 100048 -0.00% BenchmarkPostingsForMatchers/Block/n="1",i=~".+",j="foo"-4 100049 100047 -0.00% BenchmarkPostingsForMatchers/Block/n="1",i=~"1.+",j="foo"-4 11150 11148 -0.02% BenchmarkPostingsForMatchers/Block/n="1",i=~".+",i!="2",j="foo"-4 100055 100053 -0.00% BenchmarkPostingsForMatchers/Block/n="1",i=~".+",i!~"2.*",j="foo"-4 111238 111234 -0.00% benchmark old bytes new bytes delta BenchmarkPostingsForMatchers/Head/n="1"-4 10887816 10887817 +0.00% BenchmarkPostingsForMatchers/Head/n="1",j="foo"-4 5456648 5456648 +0.00% BenchmarkPostingsForMatchers/Head/j="foo",n="1"-4 5456648 5456648 +0.00% BenchmarkPostingsForMatchers/Head/n="1",j!="foo"-4 5456792 5456712 -0.00% BenchmarkPostingsForMatchers/Head/i=~".*"-4 258254408 258254328 -0.00% BenchmarkPostingsForMatchers/Head/i=~".+"-4 273912888 273912904 +0.00% BenchmarkPostingsForMatchers/Head/i=~""-4 17266680 17266600 -0.00% BenchmarkPostingsForMatchers/Head/i!=""-4 273912416 273912336 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i=~".*",j="foo"-4 7062578 7062498 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i=~".*",i!="2",j="foo"-4 7062770 7062690 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i!=""-4 28152346 28152266 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i!="",j="foo"-4 22721178 22721098 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i=~".+",j="foo"-4 22721336 22721224 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i=~"1.+",j="foo"-4 3623804 3623733 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i=~".+",i!="2",j="foo"-4 22721480 22721400 -0.00% BenchmarkPostingsForMatchers/Head/n="1",i=~".+",i!~"2.*",j="foo"-4 24816652 24816444 -0.00% BenchmarkPostingsForMatchers/Block/n="1"-4 296 296 +0.00% BenchmarkPostingsForMatchers/Block/n="1",j="foo"-4 424 424 +0.00% BenchmarkPostingsForMatchers/Block/j="foo",n="1"-4 424 424 +0.00% BenchmarkPostingsForMatchers/Block/n="1",j!="foo"-4 1544 1464 -5.18% BenchmarkPostingsForMatchers/Block/i=~".*"-4 1606114 1606045 -0.00% BenchmarkPostingsForMatchers/Block/i=~".+"-4 17264709 17264629 -0.00% BenchmarkPostingsForMatchers/Block/i=~""-4 17264780 17264696 -0.00% BenchmarkPostingsForMatchers/Block/i!=""-4 17264680 17264600 -0.00% BenchmarkPostingsForMatchers/Block/n="1",i=~".*",j="foo"-4 1606253 1606165 -0.01% BenchmarkPostingsForMatchers/Block/n="1",i=~".*",i!="2",j="foo"-4 1606445 1606348 -0.01% BenchmarkPostingsForMatchers/Block/n="1",i!=""-4 17264808 17264728 -0.00% BenchmarkPostingsForMatchers/Block/n="1",i!="",j="foo"-4 17264936 17264856 -0.00% BenchmarkPostingsForMatchers/Block/n="1",i=~".+",j="foo"-4 17264965 17264885 -0.00% BenchmarkPostingsForMatchers/Block/n="1",i=~"1.+",j="foo"-4 3148262 3148182 -0.00% BenchmarkPostingsForMatchers/Block/n="1",i=~".+",i!="2",j="foo"-4 17265141 17265061 -0.00% BenchmarkPostingsForMatchers/Block/n="1",i=~".+",i!~"2.*",j="foo"-4 20416944 20416784 -0.00% Signed-off-by: Brian Brazil --- tsdb/block.go | 12 ++--------- tsdb/cmd/tsdb/main.go | 38 +++++++++++---------------------- tsdb/head.go | 5 ++--- tsdb/index/index.go | 46 ++++++---------------------------------- tsdb/index/index_test.go | 22 ++++++------------- tsdb/querier.go | 32 +++++----------------------- tsdb/querier_test.go | 5 +++-- 7 files changed, 37 insertions(+), 123 deletions(-) diff --git a/tsdb/block.go b/tsdb/block.go index d219995d7..48ebb348b 100644 --- a/tsdb/block.go +++ b/tsdb/block.go @@ -63,7 +63,7 @@ type IndexReader interface { Symbols() index.StringIter // LabelValues returns sorted possible label values. - LabelValues(name string) (index.StringTuples, error) + LabelValues(name string) ([]string, error) // Postings returns the postings list iterator for the label pairs. // The Postings here contain the offsets to the series inside the index. @@ -87,14 +87,6 @@ type IndexReader interface { Close() error } -// StringTuples provides access to a sorted list of string tuples. -type StringTuples interface { - // Total number of tuples in the list. - Len() int - // At returns the tuple at position i. - At(i int) ([]string, error) -} - // ChunkWriter serializes a time block of chunked series data. type ChunkWriter interface { // WriteChunks writes several chunks. The Chunk field of the ChunkMetas @@ -433,7 +425,7 @@ func (r blockIndexReader) Symbols() index.StringIter { return r.ir.Symbols() } -func (r blockIndexReader) LabelValues(name string) (index.StringTuples, error) { +func (r blockIndexReader) LabelValues(name string) ([]string, error) { st, err := r.ir.LabelValues(name) return st, errors.Wrapf(err, "block: %s", r.b.Meta().ULID) } diff --git a/tsdb/cmd/tsdb/main.go b/tsdb/cmd/tsdb/main.go index 0f8d8c00d..3b26493f6 100644 --- a/tsdb/cmd/tsdb/main.go +++ b/tsdb/cmd/tsdb/main.go @@ -559,17 +559,9 @@ func analyzeBlock(b tsdb.BlockReader, limit int) error { return err } var cumulativeLength uint64 - - for i := 0; i < values.Len(); i++ { - value, err := values.At(i) - if err != nil { - return err - } - for _, str := range value { - cumulativeLength += uint64(len(str)) - } + for _, str := range values { + cumulativeLength += uint64(len(str)) } - postingInfos = append(postingInfos, postingInfo{n, cumulativeLength}) } @@ -582,7 +574,7 @@ func analyzeBlock(b tsdb.BlockReader, limit int) error { if err != nil { return err } - postingInfos = append(postingInfos, postingInfo{n, uint64(lv.Len())}) + postingInfos = append(postingInfos, postingInfo{n, uint64(len(lv))}) } fmt.Printf("\nHighest cardinality labels:\n") printInfo(postingInfos) @@ -592,25 +584,19 @@ func analyzeBlock(b tsdb.BlockReader, limit int) error { if err != nil { return err } - for i := 0; i < lv.Len(); i++ { - names, err := lv.At(i) + for _, n := range lv { + postings, err := ir.Postings("__name__", n) if err != nil { return err } - for _, n := range names { - postings, err := ir.Postings("__name__", n) - if err != nil { - return err - } - count := 0 - for postings.Next() { - count++ - } - if postings.Err() != nil { - return postings.Err() - } - postingInfos = append(postingInfos, postingInfo{n, uint64(count)}) + count := 0 + for postings.Next() { + count++ } + if postings.Err() != nil { + return postings.Err() + } + postingInfos = append(postingInfos, postingInfo{n, uint64(count)}) } fmt.Printf("\nHighest cardinality metric names:\n") printInfo(postingInfos) diff --git a/tsdb/head.go b/tsdb/head.go index 78c713334..314048248 100644 --- a/tsdb/head.go +++ b/tsdb/head.go @@ -1352,7 +1352,7 @@ func (h *headIndexReader) Symbols() index.StringIter { } // LabelValues returns the possible label values -func (h *headIndexReader) LabelValues(name string) (index.StringTuples, error) { +func (h *headIndexReader) LabelValues(name string) ([]string, error) { h.head.symMtx.RLock() sl := make([]string, 0, len(h.head.values[name])) for s := range h.head.values[name] { @@ -1360,8 +1360,7 @@ func (h *headIndexReader) LabelValues(name string) (index.StringTuples, error) { } h.head.symMtx.RUnlock() sort.Strings(sl) - - return index.NewStringTuples(sl) + return sl, nil } // LabelNames returns all the unique label names present in the head. diff --git a/tsdb/index/index.go b/tsdb/index/index.go index eb04322c2..81d4a808b 100644 --- a/tsdb/index/index.go +++ b/tsdb/index/index.go @@ -993,14 +993,6 @@ func (w *Writer) Close() error { return ensureErr } -// StringTuples provides access to a sorted list of string tuples. -type StringTuples interface { - // Total number of tuples in the list. - Len() int - // At returns the tuple at position i. - At(i int) (string, error) -} - // StringIter iterates over a sorted list of strings. type StringIter interface { // Next advances the iterator and returns true if another value was found. @@ -1424,26 +1416,26 @@ func (r *Reader) SymbolTableSize() uint64 { // LabelValues returns value tuples that exist for the given label name. // It is not safe to use the return value beyond the lifetime of the byte slice // passed into the Reader. -func (r *Reader) LabelValues(name string) (StringTuples, error) { +func (r *Reader) LabelValues(name string) ([]string, error) { if r.version == FormatV1 { e, ok := r.postingsV1[name] if !ok { - return emptyStringTuples{}, nil + return nil, nil } values := make([]string, 0, len(e)) for k := range e { values = append(values, k) } sort.Strings(values) - return NewStringTuples(values, 1) + return values, nil } e, ok := r.postings[name] if !ok { - return emptyStringTuples{}, nil + return nil, nil } if len(e) == 0 { - return emptyStringTuples{}, nil + return nil, nil } values := make([]string, 0, len(e)*symbolFactor) @@ -1473,14 +1465,9 @@ func (r *Reader) LabelValues(name string) (StringTuples, error) { if d.Err() != nil { return nil, errors.Wrap(d.Err(), "get postings offset entry") } - return NewStringTuples(values) + return values, nil } -type emptyStringTuples struct{} - -func (emptyStringTuples) At(i int) (string, error) { return "", nil } -func (emptyStringTuples) Len() int { return 0 } - // Series reads the series with the given ID and writes its labels and chunks into lbls and chks. func (r *Reader) Series(id uint64, lbls *labels.Labels, chks *[]chunks.Meta) error { offset := id @@ -1621,27 +1608,6 @@ func (r *Reader) LabelNames() ([]string, error) { return labelNames, nil } -type stringTuples struct { - entries []string // flattened tuple entries -} - -func NewStringTuples(entries []string) (*stringTuples, error) { - return &stringTuples{ - entries: entries, - }, nil -} - -func (t *stringTuples) Len() int { return len(t.entries) } -func (t *stringTuples) At(i int) (string, error) { return t.entries[i], nil } - -func (t *stringTuples) Swap(i, j int) { - t.entries[i], t.entries[j] = t.entries[j], t.entries[i] -} - -func (t *stringTuples) Less(i, j int) bool { - return t.entries[i] < t.entries[j] -} - // NewStringListIterator returns a StringIter for the given sorted list of strings. func NewStringListIter(s []string) StringIter { return &stringListIter{l: s} diff --git a/tsdb/index/index_test.go b/tsdb/index/index_test.go index 8da2a41c9..5868184ac 100644 --- a/tsdb/index/index_test.go +++ b/tsdb/index/index_test.go @@ -85,14 +85,14 @@ func (m mockIndex) Close() error { return nil } -func (m mockIndex) LabelValues(name string) (StringTuples, error) { +func (m mockIndex) LabelValues(name string) ([]string, error) { values := []string{} for l := range m.postings { if l.Name == name { values = append(values, l.Value) } } - return NewStringTuples(values) + return values, nil } func (m mockIndex) Postings(name string, values ...string) (Postings, error) { @@ -451,21 +451,13 @@ func TestPersistence_index_e2e(t *testing.T) { } for k, v := range labelPairs { sort.Strings(v) - tplsExp, err := NewStringTuples(v) + + res, err := ir.LabelValues(k) testutil.Ok(t, err) - tplsRes, err := ir.LabelValues(k) - testutil.Ok(t, err) - - testutil.Equals(t, tplsExp.Len(), tplsRes.Len()) - for i := 0; i < tplsExp.Len(); i++ { - strsExp, err := tplsExp.At(i) - testutil.Ok(t, err) - - strsRes, err := tplsRes.At(i) - testutil.Ok(t, err) - - testutil.Equals(t, strsExp, strsRes) + testutil.Equals(t, len(v), len(res)) + for i := 0; i < len(v); i++ { + testutil.Equals(t, v[i], res[i]) } } diff --git a/tsdb/querier.go b/tsdb/querier.go index d8e35eae6..bd2fa9d13 100644 --- a/tsdb/querier.go +++ b/tsdb/querier.go @@ -218,20 +218,7 @@ func (q *blockQuerier) Select(ms ...*labels.Matcher) (SeriesSet, error) { } func (q *blockQuerier) LabelValues(name string) ([]string, error) { - tpls, err := q.index.LabelValues(name) - if err != nil { - return nil, err - } - res := make([]string, 0, tpls.Len()) - - for i := 0; i < tpls.Len(); i++ { - val, err := tpls.At(i) - if err != nil { - return nil, err - } - res = append(res, val) - } - return res, nil + return q.index.LabelValues(name) } func (q *blockQuerier) LabelNames() ([]string, error) { @@ -412,17 +399,13 @@ func postingsForMatcher(ix IndexReader, m *labels.Matcher) (index.Postings, erro } } - tpls, err := ix.LabelValues(m.Name) + vals, err := ix.LabelValues(m.Name) if err != nil { return nil, err } var res []string - for i := 0; i < tpls.Len(); i++ { - val, err := tpls.At(i) - if err != nil { - return nil, err - } + for _, val := range vals { if m.Matches(val) { res = append(res, val) } @@ -437,18 +420,13 @@ func postingsForMatcher(ix IndexReader, m *labels.Matcher) (index.Postings, erro // inversePostingsForMatcher returns the postings for the series with the label name set but not matching the matcher. func inversePostingsForMatcher(ix IndexReader, m *labels.Matcher) (index.Postings, error) { - tpls, err := ix.LabelValues(m.Name) + vals, err := ix.LabelValues(m.Name) if err != nil { return nil, err } var res []string - for i := 0; i < tpls.Len(); i++ { - val, err := tpls.At(i) - if err != nil { - return nil, err - } - + for _, val := range vals { if !m.Matches(val) { res = append(res, val) } diff --git a/tsdb/querier_test.go b/tsdb/querier_test.go index 7c7cf8c76..44782ce86 100644 --- a/tsdb/querier_test.go +++ b/tsdb/querier_test.go @@ -1356,14 +1356,15 @@ func (m mockIndex) Close() error { return nil } -func (m mockIndex) LabelValues(name string) (index.StringTuples, error) { +func (m mockIndex) LabelValues(name string) ([]string, error) { values := []string{} for l := range m.postings { if l.Name == name { values = append(values, l.Value) } } - return index.NewStringTuples(values) + sort.Strings(values) + return values, nil } func (m mockIndex) Postings(name string, values ...string) (index.Postings, error) {