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

[WIP] [experimental] replace ring gogoproto with csproto #637

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

francoposa
Copy link
Member

What this PR does:

Which issue(s) this PR fixes:

Fixes #

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

csproto gen in Makefile; update & gen ring.proto; go mod tidy and vendor
…rs for getting & setting InstanceDesc map with values
…s as pointers; opt to copy pointer to value when any external or public methods require value
… and basic_lifecycler_test.go; replaced direct map access with value getters & setters
…or pointer usage, fix compilation errors in lifecycler_test.go and merge_test.go; pass changed basic_lifecycler_test.go to use pointers; opt for all pointer access; pass all mentioned tests with race check
…sFor in model.go; pass model_test.go with race check
…ng_test.go; pass all mentioned tests with race check
…irect map access with value getters & setters in getCachedShuffledSubring and getCachedShuffledSubringWithLookback to eliminate race conditions and maintain spirit of original code; pass all mentioned tests with race check
… token_range_test.go, and util_test.go; opt for value getters in spread_minimizing_token_generator_test.go for compatibility with public CanJoin interface; pass all mentioned tests with race check
@francoposa
Copy link
Member Author

A quick run of benchmarking the Ring Clone since we didn't have a benchmark for it:

func BenchmarkRing_Clone(b *testing.B) {
	for _, numInstances := range []int{50} {
		for _, numZones := range []int{1, 3} {
			for _, shardSize := range []int{0, 3, 10, 30} {
				b.Run(fmt.Sprintf("num instances = %d, num zones = %d, shard size = %d", numInstances, numZones, shardSize), func(b *testing.B) {
					benchmarkClone(b, numInstances, numZones, 128, shardSize, false)
				})
			}
		}
	}
}

func benchmarkClone(b *testing.B, numInstances, numZones, numTokens, shardSize int, cache bool) {
	// Initialise the ring.
	ringDesc := &Desc{Ingesters: generateRingInstances(initTokenGenerator(b), numInstances, numZones, numTokens)}

	b.ResetTimer()

	for n := 0; n < b.N; n++ {
		ringDesc.Clone()
	}
}

output-ring-test-bench-ring-clone-edition2023-implicit.txt
output-ring-test-bench-ring-clone-main.txt
ring-clone-benchstat.txt

@francoposa
Copy link
Member Author

Benchmark stats for all benchmarks in ring/ring_test.go - no particular effort put into optimization.

I also added a branch for vanilla proto edition 2023 using implicit presence to be compatible with proto3.
The generated code is different than doing proto3 codegen + csproto fastmarshal, but it doesn't make much difference performance-wise vs the proto3 + csproto version because none of these benchmarks cover marshaling/unmarshaling

output-ring-test-bench-ring-test-all-csproto-locked.txt
output-ring-test-bench-ring-test-all-edition2023-implicit.txt
output-ring-test-bench-ring-test-all-main.txt
ring-test-all-benchstat.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant