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

refactor(contrib/registry/etcd): use service ID as Registry.ctxMap key instead of service pointer #3509

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

1911860538
Copy link
Contributor

The code modification is based on two points:

  • It is not recommended to use pointer as key in Go map.
  • Changing the service ID to be the key in ctxMap offers better performance. Detailed performance tests will be provided below.

test_kratos_registry.go

package test_kratos_registry

import (
	"context"
	"testing"
	"time"
)

type ServiceInstance struct {
	ID        string            `json:"id"`
	Name      string            `json:"name"`
	Version   string            `json:"version"`
	Metadata  map[string]string `json:"metadata"`
	Endpoints []string          `json:"endpoints"`
}

type options struct {
	ctx       context.Context
	namespace string
	ttl       time.Duration
	maxRetry  int
}

type Registry struct {
	opts   *options
	ctxMap map[*ServiceInstance]context.CancelFunc
}

func (r *Registry) Register(ctx context.Context, service *ServiceInstance) {
	_, cancel := context.WithCancel(r.opts.ctx)
	r.ctxMap[service] = cancel
}

func (r *Registry) Deregister(ctx context.Context, service *ServiceInstance) {
	if cancel, ok := r.ctxMap[service]; ok {
		cancel()
		delete(r.ctxMap, service)
	}
}

type serviceCancel struct {
	svc    *ServiceInstance
	cancel context.CancelFunc
}

type RegistryNew struct {
	opts   *options
	ctxMap map[string]*serviceCancel
}

func (r *RegistryNew) Register(ctx context.Context, service *ServiceInstance) {
	_, cancel := context.WithCancel(r.opts.ctx)
	r.ctxMap[service.ID] = &serviceCancel{
		svc:    service,
		cancel: cancel,
	}
}

func (r *RegistryNew) Deregister(ctx context.Context, service *ServiceInstance) {
	if svcCancel, ok := r.ctxMap[service.ID]; ok {
		svcCancel.cancel()
		delete(r.ctxMap, service.ID)
	}
}

func newServiceInstances() []*ServiceInstance {
	return []*ServiceInstance{
		{
			ID:        "1",
			Name:      "service1",
			Version:   "v1",
			Metadata:  map[string]string{"key1": "value1"},
			Endpoints: []string{"http://addr1:port1"},
		},
		{
			ID:        "2",
			Name:      "service2",
			Version:   "v2",
			Metadata:  map[string]string{"key2": "value2"},
			Endpoints: []string{"http://addr2:port2"},
		},
		{
			ID:        "3",
			Name:      "service3",
			Version:   "v3",
			Metadata:  map[string]string{"key3": "value3"},
			Endpoints: []string{"http://addr3:port3"},
		},
		{
			ID:        "4",
			Name:      "service4",
			Version:   "v4",
			Metadata:  map[string]string{"key4": "value4"},
			Endpoints: []string{"http://addr4:port4"},
		},
	}
}

func BenchmarkRegisterRegister(b *testing.B) {
	r := &Registry{
		opts: &options{
			ctx: context.Background(),
		},
		ctxMap: make(map[*ServiceInstance]context.CancelFunc),
	}
	b.ResetTimer()
	b.ReportAllocs()

	for i := 0; i < b.N; i++ {
		for _, svc := range newServiceInstances() {
			r.Register(context.Background(), svc)
			//break
		}
	}
}

func BenchmarkRegisterNewRegister(b *testing.B) {
	r := &RegistryNew{
		opts: &options{
			ctx: context.Background(),
		},
		ctxMap: make(map[string]*serviceCancel),
	}
	b.ResetTimer()
	b.ReportAllocs()

	for i := 0; i < b.N; i++ {
		for _, svc := range newServiceInstances() {
			r.Register(context.Background(), svc)
			//break
		}
	}
}

func BenchmarkRegisterDeregister(b *testing.B) {
	r := &Registry{
		opts: &options{
			ctx: context.Background(),
		},
		ctxMap: make(map[*ServiceInstance]context.CancelFunc),
	}
	b.ResetTimer()
	b.ReportAllocs()

	for i := 0; i < b.N; i++ {
		for _, svc := range newServiceInstances() {
			r.Deregister(context.Background(), svc)
			//break
		}
	}
}

func BenchmarkRegisterNewDeregister(b *testing.B) {
	r := &RegistryNew{
		opts: &options{
			ctx: context.Background(),
		},
		ctxMap: make(map[string]*serviceCancel),
	}
	b.ResetTimer()
	b.ReportAllocs()

	for i := 0; i < b.N; i++ {
		for _, svc := range newServiceInstances() {
			r.Deregister(context.Background(), svc)
			//break
		}
	}
}

benchmark output:

goos: darwin
goarch: arm64
pkg: test_code/test_kratos_registry
cpu: Apple M3 Pro
BenchmarkRegisterRegister
BenchmarkRegisterRegister-11         	 1000000	      1814 ns/op	    2454 B/op	      24 allocs/op
BenchmarkRegisterNewRegister
BenchmarkRegisterNewRegister-11      	 1783624	       764.8 ns/op	    2176 B/op	      28 allocs/op
BenchmarkRegisterDeregister
BenchmarkRegisterDeregister-11       	16407162	        71.50 ns/op	       0 B/op	       0 allocs/op
BenchmarkRegisterNewDeregister
BenchmarkRegisterNewDeregister-11    	17581944	        69.60 ns/op	       0 B/op	       0 allocs/op
PASS

Process finished with the exit code 0

…y and encapsulate cancel function in struct serviceCancel
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Dec 27, 2024
@dosubot dosubot bot added the LGTM label Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants