Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance of generatings IDs from tags in M3Coordinator #1000

Merged
merged 14 commits into from
Oct 2, 2018
Merged
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ More information:

```
# to build a local m3dbnode process
make m3dbnode
make m3dbnode (note that we currently require at least Go 1.10 or higher)

# run it with the sample configuration
./bin/m3dbnode -f ./src/dbnode/config/m3dbnode-local-etcd.yml
Expand Down
34 changes: 32 additions & 2 deletions src/query/models/tag.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"hash/fnv"
"regexp"
"sort"
"strings"
)

const (
Expand Down Expand Up @@ -147,15 +148,44 @@ func (m Matchers) ToTags() (Tags, error) {

// ID returns a string representation of the tags
func (t Tags) ID() string {
b := make([]byte, 0, len(t))
var (
idLen = t.IDLen()
strBuilder = strings.Builder{}
Copy link
Contributor

Choose a reason for hiding this comment

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

is this 1.10+ only feature ? If yes, maybe we should figure out if we are still planning to support 1.9

Copy link
Collaborator

Choose a reason for hiding this comment

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

1.9 is over a year old at this point, 1.10 is over 6 months old. 1.11 just came out.

Any reason you want to support 1.9?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably ok to drop support, since we're only supporting built binaries really. We would like most people to use all this software as docker images or as binaries built from the tagged releases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, i agree that we should drop 1.9 support. Even prom only supports 1.10+ prometheus/prometheus#3856. We should call it out in our README though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yup, @richardartoul want to update the README.md (in a minimal way, we can update formatting/etc of it later) that we only support 1.10 at this time?

)

strBuilder.Grow(idLen)
for _, tag := range t {
strBuilder.WriteString(tag.Name)
strBuilder.WriteByte(eq)
strBuilder.WriteString(tag.Value)
strBuilder.WriteByte(sep)
}

return strBuilder.String()
}

// IDMarshalTo writes out the ID representation
// of the tags into the provided buffer.
func (t Tags) IDMarshalTo(b []byte) []byte {
for _, tag := range t {
b = append(b, tag.Name...)
b = append(b, eq)
b = append(b, tag.Value...)
b = append(b, sep)
}

return string(b)
return b
}

// IDLen returns the length of the ID that would be
// generated from the tags.
func (t Tags) IDLen() int {
idLen := 2 * len(t) // account for eq and sep
for _, tag := range t {
idLen += len(tag.Name)
idLen += len(tag.Value)
}
return idLen
}

// IDWithExcludes returns a string representation of the tags excluding some tag keys
Expand Down
12 changes: 11 additions & 1 deletion src/query/models/tag_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,17 @@ func createTags(withName bool) Tags {

func TestTagID(t *testing.T) {
tags := createTags(false)
assert.Equal(t, tags.ID(), "t1=v1,t2=v2,")
assert.Equal(t, "t1=v1,t2=v2,", tags.ID())
assert.Equal(t, tags.IDLen(), len(tags.ID()))
}

func TestTagIDMarshalTo(t *testing.T) {
var (
tags = createTags(false)
b = tags.IDMarshalTo([]byte{})
)
assert.Equal(t, []byte("t1=v1,t2=v2,"), b)
assert.Equal(t, tags.IDLen(), len(b))
}

func TestWithoutName(t *testing.T) {
Expand Down
14 changes: 9 additions & 5 deletions src/query/storage/m3/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -269,11 +269,15 @@ func (s *m3storage) Write(
return errors.ErrNilWriteQuery
}

id := query.Tags.ID()
// TODO: Consider caching id -> identID
identID := ident.StringID(id)
var (
// TODO: Pool this once an ident pool is setup. We will have
// to stop calling NoFinalize() below if we do that.
buf = make([]byte, 0, query.Tags.IDLen())
idBuf = query.Tags.IDMarshalTo(buf)
id = ident.BytesID(idBuf)
)
// Set id to NoFinalize to avoid cloning it in write operations
identID.NoFinalize()
id.NoFinalize()
tagIterator := storage.TagsToIdentTagIterator(query.Tags)

var (
Expand All @@ -287,7 +291,7 @@ func (s *m3storage) Write(
datapoint := datapoint
wg.Add(1)
s.writeWorkerPool.Go(func() {
if err := s.writeSingle(ctx, query, datapoint, identID, tagIter); err != nil {
if err := s.writeSingle(ctx, query, datapoint, id, tagIter); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add a special case here for when len(query.Datapoints) == 1 to just return s.writeSingle(...) as discussed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

multiErr.add(err)
}

Expand Down