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 the 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. Instead of
    doing this, we should just ensure that when the state is updated, it is
    kept in "normalized" form. This might already be the case, but should be
    verified.

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.
  • Loading branch information
stevesg committed Nov 23, 2021
1 parent bd9af71 commit e482fc1
Show file tree
Hide file tree
Showing 2 changed files with 2 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] //.Clone()
// 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
2 changes: 1 addition & 1 deletion ring/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ func (d *Desc) mergeWithTime(mergeable memberlist.Mergeable, localCAS bool, now
return nil, nil
}

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

var updated []string
Expand Down

0 comments on commit e482fc1

Please sign in to comment.