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.

Sign up for GitHub

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

Jump to bottom

Ring: reduce allocations and copies when Merge()-ing states. #77

Merged
merged 5 commits into from
Nov 26, 2021
Merged

Ring: reduce allocations and copies when Merge()-ing states. #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4b6d1b3
Ring: reduce allocations and copies when Merge()-ing states.
stevesg Nov 24, 2021
f11fe6a
Review comments.
stevesg Nov 24, 2021
4298eb4
Review comments.
stevesg Nov 25, 2021
5620d91
Review comments.
stevesg Nov 26, 2021
f59a6bb
Changelog.
stevesg Nov 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,5 @@
* [ENHANCEMENT] Replace go-kit/kit/log with go-kit/log. #52
* [ENHANCEMENT] Add spanlogger package. #42
* [ENHANCEMENT] Add runutil.CloseWithLogOnErr function. #58
* [ENHANCEMENT] Optimise memberlist receive path when used as a backing store for rings with a large number of members. #76
* [ENHANCEMENT] Optimise memberlist receive path when used as a backing store for rings with a large number of members. #76 #77
* [BUGFIX] spanlogger: Support multiple tenant IDs. #59
2 changes: 2 additions & 0 deletions kv/memberlist/mergeable.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ type Mergeable interface {
// Merge with other value in place. Returns change, that can be sent to other clients.
// If merge doesn't result in any change, returns nil.
// Error can be returned if merging with given 'other' value is not possible.
// Implementors of this method are permitted to modify the other parameter, as the
// memberlist-based KV store will not use the same "other" parameter in multiple Merge calls.
//
// In order for state merging to work correctly, Merge function must have some properties. When talking about the
// result of the merge in the following text, we don't mean the return value ("change"), but the
Expand Down
27 changes: 13 additions & 14 deletions ring/model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,8 @@ func (i *InstanceDesc) IsReady(now time.Time, heartbeatTimeout time.Duration) er
// (see resolveConflicts).
//
// This method is part of memberlist.Mergeable interface, and is only used by gossiping ring.
//
// Note: This method modifies d and mergeable to reduce allocations and copies.
stevesg marked this conversation as resolved.
Show resolved Hide resolved
func (d *Desc) Merge(mergeable memberlist.Mergeable, localCAS bool) (memberlist.Mergeable, error) {
return d.mergeWithTime(mergeable, localCAS, time.Now())
}
Expand All @@ -192,8 +194,11 @@ func (d *Desc) mergeWithTime(mergeable memberlist.Mergeable, localCAS bool, now
return nil, nil
}

thisIngesterMap := buildNormalizedIngestersMap(d)
otherIngesterMap := buildNormalizedIngestersMap(other)
normalizeIngestersMap(d)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the current value which we retrieved from KV.store already be normalized? If so, is it necessary to normalize d again, or could we assume that d is already normalized? I haven't checked how big of a performance difference this would be, might not be significant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a decent win, I have numbers somewhere. It's just a bit hard to reason about so it's on the riskier end of the optimization spectrum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

name                             old time/op    new time/op    delta
MemberlistReceiveWithRingDesc-6    4.12ms ±13%    3.58ms ± 8%  -13.11%  (p=0.000 n=10+9)

name                             old alloc/op   new alloc/op   delta
MemberlistReceiveWithRingDesc-6    51.6kB ±20%    32.0kB ±15%  -37.95%  (p=0.000 n=10+10)

name                             old allocs/op  new allocs/op  delta
MemberlistReceiveWithRingDesc-6       681 ± 2%        75 ± 7%  -89.03%  (p=0.000 n=10+10)

normalizeIngestersMap(other)
stevesg marked this conversation as resolved.
Show resolved Hide resolved

thisIngesterMap := d.Ingesters
otherIngesterMap := other.Ingesters

var updated []string

Expand Down Expand Up @@ -261,22 +266,18 @@ func (d *Desc) MergeContent() []string {
return result
}

// buildNormalizedIngestersMap will do the following:
// normalizeIngestersMap will do the following:
// - sorts tokens and removes duplicates (only within single ingester)
// - it doesn't modify input ring
func buildNormalizedIngestersMap(inputRing *Desc) map[string]InstanceDesc {
out := map[string]InstanceDesc{}

// - modifies the input ring
func normalizeIngestersMap(inputRing *Desc) {
// Make sure LEFT ingesters have no tokens
for n, ing := range inputRing.Ingesters {
if ing.State == LEFT {
ing.Tokens = nil
inputRing.Ingesters[n] = ing
}
out[n] = ing
}

// Sort tokens, and remove duplicates
for name, ing := range out {
// Sort tokens, and remove duplicates
if len(ing.Tokens) == 0 {
continue
}
Expand All @@ -297,10 +298,8 @@ func buildNormalizedIngestersMap(inputRing *Desc) map[string]InstanceDesc {
}

// write updated value back to map
out[name] = ing
inputRing.Ingesters[n] = ing
}

return out
}

func conflictingTokensExist(normalizedIngesters map[string]InstanceDesc) bool {
Expand Down