Skip to content

Commit

Permalink
perf: remove expensive reflection from xDS hot path
Browse files Browse the repository at this point in the history
Backport of #14934 to 1.13 release series.

Replaces the reflection-based implementation of proxycfg's
ConfigSnapshot.Clone with code generated by deep-copy.
  • Loading branch information
boxofrad committed Feb 15, 2023
1 parent 5b252a4 commit 54138dc
Show file tree
Hide file tree
Showing 17 changed files with 1,639 additions and 60 deletions.
21 changes: 21 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,26 @@ jobs:
name: "Protobuf Lint"
command: make proto-lint

check-generated-deep-copy:
docker:
- image: *GOLANG_IMAGE
environment:
<<: *ENVIRONMENT
# tput complains if this isn't set to something.
TERM: ansi
steps:
- checkout
- run:
name: Install deep-copy
command: make codegen-tools
- run:
command: make --always-make deep-copy
- run: |
if ! git diff --exit-code; then
echo "Generated code was not updated correctly"
exit 1
fi
go-test-arm64:
machine:
image: *UBUNTU_CI_IMAGE
Expand Down Expand Up @@ -1050,6 +1070,7 @@ workflows:
- /^backport\/docs\/.*/
- /^backport\/ui\/.*/
- check-generated-protobuf: *filter-ignore-non-go-branches
- check-generated-deep-copy: *filter-ignore-non-go-branches
- lint-enums: *filter-ignore-non-go-branches
- lint-consul-retry: *filter-ignore-non-go-branches
- lint: *filter-ignore-non-go-branches
Expand Down
11 changes: 11 additions & 0 deletions GNUmakefile
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ PROTOC_GEN_GO_GRPC_VERSION="v1.2.0"
MOG_VERSION='v0.3.0'
PROTOC_GO_INJECT_TAG_VERSION='v1.3.0'
PROTOC_GEN_GO_BINARY_VERSION="v0.0.1"
DEEP_COPY_VERSION='bc3f5aa5735d8a54961580a3a24422c308c831c2'

GOTAGS ?=
GOPATH=$(shell go env GOPATH)
Expand Down Expand Up @@ -321,6 +322,16 @@ lint-tools:
proto-tools:
@$(SHELL) $(CURDIR)/build-support/scripts/devtools.sh -protobuf

.PHONY: codegen-tools
codegen-tools:
@$(SHELL) $(CURDIR)/build-support/scripts/devtools.sh -codegen

.PHONY: deep-copy
deep-copy:
@$(SHELL) $(CURDIR)/agent/structs/deep-copy.sh
@$(SHELL) $(CURDIR)/proto/pbpeering/deep-copy.sh
@$(SHELL) $(CURDIR)/agent/proxycfg/deep-copy.sh

version:
@echo -n "Version: "
@$(SHELL) $(CURDIR)/build-support/scripts/version.sh
Expand Down
15 changes: 15 additions & 0 deletions agent/proxycfg/deep-copy.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#!/usr/bin/env bash

readonly PACKAGE_DIR="$(dirname "${BASH_SOURCE[0]}")"
cd $PACKAGE_DIR

# Uses: https://github.com/globusdigital/deep-copy
deep-copy -pointer-receiver \
-o ./proxycfg.deepcopy.go \
-type ConfigSnapshot \
-type ConfigSnapshotUpstreams \
-type configSnapshotConnectProxy \
-type configSnapshotIngressGateway \
-type configSnapshotMeshGateway \
-type configSnapshotTerminatingGateway \
./
27 changes: 25 additions & 2 deletions agent/proxycfg/internal/watch/watchmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,18 @@ package watch

import "context"

// DeepCopyable describes a type that implements the DeepCopy
// method to get a copy of itself that is safe to pass around
// without worrying about receivers modifying the original.
type DeepCopyable[T any] interface {
DeepCopy() T
}

// Map safely stores and retrieves values by validating that
// there is a live watch for a key. InitWatch must be called
// to associate a key with its cancel function before any
// Set's are called.
type Map[K comparable, V any] struct {
type Map[K comparable, V DeepCopyable[V]] struct {
M map[K]watchedVal[V]
}

Expand All @@ -20,10 +27,26 @@ type watchedVal[V any] struct {
cancel context.CancelFunc
}

func NewMap[K comparable, V any]() Map[K, V] {
func NewMap[K comparable, V DeepCopyable[V]]() Map[K, V] {
return Map[K, V]{M: make(map[K]watchedVal[V])}
}

// DeepCopy returns a copy of the Map that is safe to be passed
// around without worrying about receivers modifying the original
// or canceling its watches.
func (m Map[K, V]) DeepCopy() Map[K, V] {
dup := make(map[K]watchedVal[V], len(m.M))
for k, v := range m.M {
var val *V
if v.Val != nil {
dc := (*v.Val).DeepCopy()
val = &dc
}
dup[k] = watchedVal[V]{Val: val}
}
return Map[K, V]{M: dup}
}

// InitWatch associates a cancel function with a key,
// allowing Set to be called for the key. The cancel
// function is allowed to be nil.
Expand Down
56 changes: 36 additions & 20 deletions agent/proxycfg/internal/watch/watchmap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
)

func TestMap(t *testing.T) {
m := NewMap[string, string]()
m := NewMap[string, testVal]()

// Set without init is a no-op
{
Expand Down Expand Up @@ -51,7 +51,7 @@ func TestMap(t *testing.T) {
{
got, ok := m.Get("hello")
require.True(t, ok)
require.Equal(t, "world", got)
require.Equal(t, "world", string(got))
}

// CancelWatch successful
Expand All @@ -76,15 +76,11 @@ func TestMap(t *testing.T) {
}

func TestMap_ForEach(t *testing.T) {
type testType struct {
s string
}

m := NewMap[string, any]()
inputs := map[string]any{
"hello": 13,
"foo": struct{}{},
"bar": &testType{s: "wow"},
m := NewMap[string, testVal]()
inputs := map[string]testVal{
"hello": "world",
"foo": "bar",
"baz": "bat",
}
for k, v := range inputs {
m.InitWatch(k, nil)
Expand Down Expand Up @@ -114,15 +110,11 @@ func TestMap_ForEach(t *testing.T) {
}

func TestMap_ForEachE(t *testing.T) {
type testType struct {
s string
}

m := NewMap[string, any]()
inputs := map[string]any{
"hello": 13,
"foo": struct{}{},
"bar": &testType{s: "wow"},
m := NewMap[string, testVal]()
inputs := map[string]testVal{
"hello": "world",
"foo": "bar",
"baz": "bat",
}
for k, v := range inputs {
m.InitWatch(k, nil)
Expand Down Expand Up @@ -152,3 +144,27 @@ func TestMap_ForEachE(t *testing.T) {
require.Errorf(t, err, "boo")
}
}

func TestMap_DeepCopy(t *testing.T) {
orig := NewMap[string, testVal]()
inputs := map[string]testVal{
"hello": "world",
"foo": "bar",
"baz": "bat",
}
for k, v := range inputs {
orig.InitWatch(k, nil)
orig.Set(k, v)
}
require.Equal(t, 3, orig.Len())

clone := orig.DeepCopy()
require.Equal(t, 3, clone.Len())

orig.CancelWatch("hello")
require.NotEqual(t, orig.Len(), clone.Len())
}

type testVal string

func (tv testVal) DeepCopy() testVal { return tv }
10 changes: 3 additions & 7 deletions agent/proxycfg/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"testing"
"time"

"github.com/mitchellh/copystructure"
"github.com/stretchr/testify/require"

"github.com/hashicorp/consul/acl"
Expand Down Expand Up @@ -324,17 +323,14 @@ func TestManager_BasicLifecycle(t *testing.T) {
dataSources.ConfigEntry.Set(meshConfigReq, &structs.ConfigEntryResponse{Entry: nil})
tt.setup(t, dataSources)

expectSnapCopy, err := tt.expectSnap.Clone()
require.NoError(t, err)

webProxyCopy, err := copystructure.Copy(webProxy)
require.NoError(t, err)
expectSnapCopy := tt.expectSnap.Clone()
webProxyCopy := webProxy.DeepCopy()

testManager_BasicLifecycle(t,
dataSources,
rootsReq, leafReq,
roots,
webProxyCopy.(*structs.NodeService),
webProxyCopy,
expectSnapCopy,
)
})
Expand Down
Loading

0 comments on commit 54138dc

Please sign in to comment.