Skip to content

Commit

Permalink
WIP: Demonstration of potential memberlist optimizations.
Browse files Browse the repository at this point in the history
This is to show posssible optimizations on the memberlist receive path. I
believe both these are correct but I need to spend a little time more double
checking. All unit tests pass with both these optimizations.

1) Remove Clone()-ing `ring.Desc`

    The state is cloned so that when the state is not changed, it is not put
    back into the kv structure. This is not necessary, as by definition, the
    state did not change. Additionally, there are no error paths where we
    depend in this cloning to be able to roll-back the state.

2) Remove copying when normalizing of the `Ingesters` map

    The costly part here is not normalizing the incoming update, but
    normalizing the currently held state, as this can be large. By doing this
    in-place we can avoid a lot of overhead.

Both these changes will significantly reduce GC load as both are in essence
cloning the ingesters map, only for it to be thrown away shortly after.

As a follow up, there might be possibility to avoid the normalization entirely,
assuming we can keep the current ring state normalized, but this needs some more
reasoning put into it.
  • Loading branch information
stevesg committed Nov 23, 2021
1 parent bd9af71 commit ec40ae1
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 2 deletions.
2 changes: 1 addition & 1 deletion kv/memberlist/memberlist_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -1184,7 +1184,7 @@ func (m *KV) mergeValueForKey(key string, incomingValue Mergeable, casVersion ui
m.storeMu.Lock()
defer m.storeMu.Unlock()

curr := m.store[key].Clone()
curr := m.store[key]
// if casVersion is 0, then there was no previous value, so we will just do normal merge, without localCAS flag set.
if casVersion > 0 && curr.version != casVersion {
return nil, 0, errVersionMismatch
Expand Down
38 changes: 37 additions & 1 deletion ring/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,8 @@ func (d *Desc) mergeWithTime(mergeable memberlist.Mergeable, localCAS bool, now
return nil, nil
}

thisIngesterMap := buildNormalizedIngestersMap(d)
normalizeIngestersMap(d)
thisIngesterMap := d.Ingesters
otherIngesterMap := buildNormalizedIngestersMap(other)

var updated []string
Expand Down Expand Up @@ -303,6 +304,41 @@ func buildNormalizedIngestersMap(inputRing *Desc) map[string]InstanceDesc {
return out
}

// normalizeIngestersMap will do the following:
// - sorts tokens and removes duplicates (only within single ingester)
// - modifies the input ring
func normalizeIngestersMap(inputRing *Desc) {
for n, ing := range inputRing.Ingesters {
// Make sure LEFT ingesters have no tokens
if ing.State == LEFT {
ing.Tokens = nil
}

// Sort tokens, and remove duplicates
if len(ing.Tokens) == 0 {
inputRing.Ingesters[n] = ing
continue
}

if !sort.IsSorted(Tokens(ing.Tokens)) {
sort.Sort(Tokens(ing.Tokens))
}

// tokens are sorted now, we can easily remove duplicates.
prev := ing.Tokens[0]
for ix := 1; ix < len(ing.Tokens); {
if ing.Tokens[ix] == prev {
ing.Tokens = append(ing.Tokens[:ix], ing.Tokens[ix+1:]...)
} else {
prev = ing.Tokens[ix]
ix++
}
}

inputRing.Ingesters[n] = ing
}
}

func conflictingTokensExist(normalizedIngesters map[string]InstanceDesc) bool {
count := 0
for _, ing := range normalizedIngesters {
Expand Down

0 comments on commit ec40ae1

Please sign in to comment.