Skip to content

Commit 33fa6b4

Browse files
halfcrazyhalfcrazy
committed
refactor WhereCache func to import performance
Signed-off-by: Yan Zhu <[email protected]>
1 parent 89850f8 commit 33fa6b4

12 files changed

+291
-220
lines changed

‎cache/cache_test.go

+137-99
Large diffs are not rendered by default.

‎client/api.go

+18-34
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ type API interface {
2424
// Create a Conditional API from a Function that is used to filter cached data
2525
// The function must accept a Model implementation and return a boolean. E.g:
2626
// ConditionFromFunc(func(l *LogicalSwitch) bool { return l.Enabled })
27-
WhereCache(predicate interface{}) ConditionalAPI
27+
WhereCache(model.Model, func(model.Model) bool) ConditionalAPI
2828

2929
// Create a ConditionalAPI from a Model's index data, where operations
3030
// apply to elements that match the values provided in one or more
@@ -120,13 +120,20 @@ func (a api) List(ctx context.Context, result interface{}) error {
120120
// structs
121121
var appendValue func(reflect.Value)
122122
var m model.Model
123+
var ok bool
123124
if resultVal.Type().Elem().Kind() == reflect.Ptr {
124-
m = reflect.New(resultVal.Type().Elem().Elem()).Interface()
125+
m, ok = reflect.New(resultVal.Type().Elem().Elem()).Interface().(model.Model)
126+
if !ok {
127+
return &ErrWrongType{resultPtr.Type(), "Expected pointer to slice of valid Models"}
128+
}
125129
appendValue = func(v reflect.Value) {
126130
resultVal.Set(reflect.Append(resultVal, v))
127131
}
128132
} else {
129-
m = reflect.New(resultVal.Type().Elem()).Interface()
133+
m, ok = reflect.New(resultVal.Type().Elem()).Interface().(model.Model)
134+
if !ok {
135+
return &ErrWrongType{resultPtr.Type(), "Expected pointer to slice of valid Models"}
136+
}
130137
appendValue = func(v reflect.Value) {
131138
resultVal.Set(reflect.Append(resultVal, reflect.Indirect(v)))
132139
}
@@ -194,14 +201,17 @@ func (a api) WhereAll(m model.Model, cond ...model.Condition) ConditionalAPI {
194201
}
195202

196203
// WhereCache returns a conditionalAPI based a Predicate
197-
func (a api) WhereCache(predicate interface{}) ConditionalAPI {
198-
return newConditionalAPI(a.cache, a.conditionFromFunc(predicate), a.logger)
204+
func (a api) WhereCache(m model.Model, predicate func(model.Model) bool) ConditionalAPI {
205+
return newConditionalAPI(a.cache, a.conditionFromFunc(m, predicate), a.logger)
199206
}
200207

201208
// Conditional interface implementation
202209
// FromFunc returns a Condition from a function
203-
func (a api) conditionFromFunc(predicate interface{}) Conditional {
204-
table, err := a.getTableFromFunc(predicate)
210+
func (a api) conditionFromFunc(m model.Model, predicate func(model.Model) bool) Conditional {
211+
if predicate == nil {
212+
return newErrorConditional(fmt.Errorf("expect predicate as a function, got nil"))
213+
}
214+
table, err := a.getTableFromModel(m)
205215
if err != nil {
206216
return newErrorConditional(err)
207217
}
@@ -542,39 +552,13 @@ func (a api) getTableFromModel(m interface{}) (string, error) {
542552
if _, ok := m.(model.Model); !ok {
543553
return "", &ErrWrongType{reflect.TypeOf(m), "Type does not implement Model interface"}
544554
}
545-
table := a.cache.DatabaseModel().FindTable(reflect.TypeOf(m))
555+
table := m.(model.Model).Table()
546556
if table == "" {
547557
return "", &ErrWrongType{reflect.TypeOf(m), "Model not found in Database Model"}
548558
}
549559
return table, nil
550560
}
551561

552-
// getTableFromModel returns the table name from a the predicate after performing
553-
// type verifications
554-
func (a api) getTableFromFunc(predicate interface{}) (string, error) {
555-
predType := reflect.TypeOf(predicate)
556-
if predType == nil || predType.Kind() != reflect.Func {
557-
return "", &ErrWrongType{predType, "Expected function"}
558-
}
559-
if predType.NumIn() != 1 || predType.NumOut() != 1 || predType.Out(0).Kind() != reflect.Bool {
560-
return "", &ErrWrongType{predType, "Expected func(Model) bool"}
561-
}
562-
563-
modelInterface := reflect.TypeOf((*model.Model)(nil)).Elem()
564-
modelType := predType.In(0)
565-
if !modelType.Implements(modelInterface) {
566-
return "", &ErrWrongType{predType,
567-
fmt.Sprintf("Type %s does not implement Model interface", modelType.String())}
568-
}
569-
570-
table := a.cache.DatabaseModel().FindTable(modelType)
571-
if table == "" {
572-
return "", &ErrWrongType{predType,
573-
fmt.Sprintf("Model %s not found in Database Model", modelType.String())}
574-
}
575-
return table, nil
576-
}
577-
578562
// newAPI returns a new API to interact with the database
579563
func newAPI(cache *cache.TableCache, logger *logr.Logger) API {
580564
return api{

‎client/api_test.go

+40-58
Original file line numberDiff line numberDiff line change
@@ -217,52 +217,51 @@ func TestAPIListPredicate(t *testing.T) {
217217

218218
test := []struct {
219219
name string
220-
predicate interface{}
220+
m model.Model
221+
predicate func(model.Model) bool
221222
content []model.Model
222223
err bool
223224
}{
224225
{
225226
name: "none",
226-
predicate: func(t *testLogicalSwitch) bool {
227+
m: &testLogicalSwitch{},
228+
predicate: func(t model.Model) bool {
227229
return false
228230
},
229231
content: []model.Model{},
230232
err: false,
231233
},
232234
{
233235
name: "all",
234-
predicate: func(t *testLogicalSwitch) bool {
236+
m: &testLogicalSwitch{},
237+
predicate: func(t model.Model) bool {
235238
return true
236239
},
237240
content: lscacheList,
238241
err: false,
239242
},
240243
{
241244
name: "nil function must fail",
245+
m: &testLogicalSwitch{},
242246
err: true,
243247
},
244248
{
245249
name: "arbitrary condition",
246-
predicate: func(t *testLogicalSwitch) bool {
250+
m: &testLogicalSwitch{},
251+
predicate: func(m model.Model) bool {
252+
t := m.(*testLogicalSwitch)
247253
return strings.HasPrefix(t.Name, "magic")
248254
},
249255
content: []model.Model{lscacheList[1], lscacheList[3]},
250256
err: false,
251257
},
252-
{
253-
name: "error wrong type",
254-
predicate: func(t testLogicalSwitch) string {
255-
return "foo"
256-
},
257-
err: true,
258-
},
259258
}
260259

261260
for _, tt := range test {
262261
t.Run(fmt.Sprintf("ApiListPredicate: %s", tt.name), func(t *testing.T) {
263262
var result []*testLogicalSwitch
264263
api := newAPI(tcache, &discardLogger)
265-
cond := api.WhereCache(tt.predicate)
264+
cond := api.WhereCache(tt.m, tt.predicate)
266265
err := cond.List(context.Background(), &result)
267266
if tt.err {
268267
assert.NotNil(t, err)
@@ -536,26 +535,14 @@ func TestAPIListMulti(t *testing.T) {
536535
func TestConditionFromFunc(t *testing.T) {
537536
test := []struct {
538537
name string
539-
arg interface{}
538+
m model.Model
539+
arg func(model.Model) bool
540540
err bool
541541
}{
542-
{
543-
name: "wrong function must fail",
544-
arg: func(s string) bool {
545-
return false
546-
},
547-
err: true,
548-
},
549-
{
550-
name: "wrong function must fail2 ",
551-
arg: func(t *testLogicalSwitch) string {
552-
return "foo"
553-
},
554-
err: true,
555-
},
556542
{
557543
name: "correct func should succeed",
558-
arg: func(t *testLogicalSwitch) bool {
544+
m: &testLogicalSwitch{},
545+
arg: func(m model.Model) bool {
559546
return true
560547
},
561548
err: false,
@@ -566,7 +553,7 @@ func TestConditionFromFunc(t *testing.T) {
566553
t.Run(fmt.Sprintf("conditionFromFunc: %s", tt.name), func(t *testing.T) {
567554
cache := apiTestCache(t, nil)
568555
apiIface := newAPI(cache, &discardLogger)
569-
condition := apiIface.(api).conditionFromFunc(tt.arg)
556+
condition := apiIface.(api).conditionFromFunc(tt.m, tt.arg)
570557
if tt.err {
571558
assert.IsType(t, &errorConditional{}, condition)
572559
} else {
@@ -584,23 +571,6 @@ func TestConditionFromModel(t *testing.T) {
584571
conds []model.Condition
585572
err bool
586573
}{
587-
{
588-
name: "wrong model must fail",
589-
models: []model.Model{
590-
&struct{ a string }{},
591-
},
592-
err: true,
593-
},
594-
{
595-
name: "wrong condition must fail",
596-
models: []model.Model{
597-
&struct {
598-
a string `ovsdb:"_uuid"`
599-
}{},
600-
},
601-
conds: []model.Condition{{Field: "foo"}},
602-
err: true,
603-
},
604574
{
605575
name: "correct model must succeed",
606576
models: []model.Model{
@@ -1009,7 +979,8 @@ func TestAPIMutate(t *testing.T) {
1009979
{
1010980
name: "select single by predicate name insert element in map",
1011981
condition: func(a API) ConditionalAPI {
1012-
return a.WhereCache(func(lsp *testLogicalSwitchPort) bool {
982+
return a.WhereCache(&testLogicalSwitchPort{}, func(m model.Model) bool {
983+
lsp := m.(*testLogicalSwitchPort)
1013984
return lsp.Name == "lsp2"
1014985
})
1015986
},
@@ -1033,7 +1004,8 @@ func TestAPIMutate(t *testing.T) {
10331004
{
10341005
name: "select many by predicate name insert element in map",
10351006
condition: func(a API) ConditionalAPI {
1036-
return a.WhereCache(func(lsp *testLogicalSwitchPort) bool {
1007+
return a.WhereCache(&testLogicalSwitchPort{}, func(m model.Model) bool {
1008+
lsp := m.(*testLogicalSwitchPort)
10371009
return lsp.Type == "someType"
10381010
})
10391011
},
@@ -1063,7 +1035,8 @@ func TestAPIMutate(t *testing.T) {
10631035
{
10641036
name: "No mutations should error",
10651037
condition: func(a API) ConditionalAPI {
1066-
return a.WhereCache(func(lsp *testLogicalSwitchPort) bool {
1038+
return a.WhereCache(&testLogicalSwitchPort{}, func(m model.Model) bool {
1039+
lsp := m.(*testLogicalSwitchPort)
10671040
return lsp.Type == "someType"
10681041
})
10691042
},
@@ -1411,7 +1384,8 @@ func TestAPIUpdate(t *testing.T) {
14111384
{
14121385
name: "select multiple by predicate change multiple field",
14131386
condition: func(a API) ConditionalAPI {
1414-
return a.WhereCache(func(t *testLogicalSwitchPort) bool {
1387+
return a.WhereCache(&testLogicalSwitchPort{}, func(m model.Model) bool {
1388+
t := m.(*testLogicalSwitchPort)
14151389
return t.Enabled != nil && *t.Enabled == true
14161390
})
14171391
},
@@ -1645,7 +1619,8 @@ func TestAPIDelete(t *testing.T) {
16451619
{
16461620
name: "select multiple by predicate",
16471621
condition: func(a API) ConditionalAPI {
1648-
return a.WhereCache(func(t *testLogicalSwitchPort) bool {
1622+
return a.WhereCache(&testLogicalSwitchPort{}, func(m model.Model) bool {
1623+
t := m.(*testLogicalSwitchPort)
16491624
return t.Enabled != nil && *t.Enabled == true
16501625
})
16511626
},
@@ -1724,29 +1699,36 @@ func BenchmarkAPIList(b *testing.B) {
17241699

17251700
test := []struct {
17261701
name string
1727-
predicate interface{}
1702+
m model.Model
1703+
predicate func(model.Model) bool
17281704
}{
17291705
{
17301706
name: "predicate returns none",
1731-
predicate: func(t *testLogicalSwitchPort) bool {
1707+
m: &testLogicalSwitchPort{},
1708+
predicate: func(t model.Model) bool {
17321709
return false
17331710
},
17341711
},
17351712
{
17361713
name: "predicate returns all",
1737-
predicate: func(t *testLogicalSwitchPort) bool {
1714+
m: &testLogicalSwitchPort{},
1715+
predicate: func(t model.Model) bool {
17381716
return true
17391717
},
17401718
},
17411719
{
17421720
name: "predicate on an arbitrary condition",
1743-
predicate: func(t *testLogicalSwitchPort) bool {
1721+
m: &testLogicalSwitchPort{},
1722+
predicate: func(m model.Model) bool {
1723+
t := m.(*testLogicalSwitchPort)
17441724
return strings.HasPrefix(t.Name, "ls1")
17451725
},
17461726
},
17471727
{
17481728
name: "predicate matches name",
1749-
predicate: func(t *testLogicalSwitchPort) bool {
1729+
m: &testLogicalSwitchPort{},
1730+
predicate: func(m model.Model) bool {
1731+
t := m.(*testLogicalSwitchPort)
17501732
return t.Name == lscacheList[index].Name
17511733
},
17521734
},
@@ -1763,7 +1745,7 @@ func BenchmarkAPIList(b *testing.B) {
17631745
api := newAPI(tcache, &discardLogger)
17641746
var cond ConditionalAPI
17651747
if tt.predicate != nil {
1766-
cond = api.WhereCache(tt.predicate)
1748+
cond = api.WhereCache(tt.m, tt.predicate)
17671749
} else {
17681750
cond = api.Where(lscacheList[index])
17691751
}
@@ -1979,7 +1961,7 @@ func TestAPIWait(t *testing.T) {
19791961
{
19801962
name: "no operation",
19811963
condition: func(a API) ConditionalAPI {
1982-
return a.WhereCache(func(t *testLogicalSwitchPort) bool { return false })
1964+
return a.WhereCache(&testLogicalSwitchPort{}, func(t model.Model) bool { return false })
19831965
},
19841966
until: "==",
19851967
prepare: func() (model.Model, []interface{}) {

‎client/client.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -1475,6 +1475,6 @@ func (o *ovsdbClient) WhereAll(m model.Model, conditions ...model.Condition) Con
14751475
}
14761476

14771477
// WhereCache implements the API interface's WhereCache function
1478-
func (o *ovsdbClient) WhereCache(predicate interface{}) ConditionalAPI {
1479-
return o.primaryDB().api.WhereCache(predicate)
1478+
func (o *ovsdbClient) WhereCache(m model.Model, predicate func(model.Model) bool) ConditionalAPI {
1479+
return o.primaryDB().api.WhereCache(m, predicate)
14801480
}

‎client/client_test.go

+8
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,10 @@ type Bridge struct {
8181
STPEnable bool `ovsdb:"stp_enable"`
8282
}
8383

84+
func (b *Bridge) Table() string {
85+
return "Bridge"
86+
}
87+
8488
// OpenvSwitch defines an object in Open_vSwitch table
8589
type OpenvSwitch struct {
8690
UUID string `ovsdb:"_uuid"`
@@ -103,6 +107,10 @@ type OpenvSwitch struct {
103107
SystemVersion *string `ovsdb:"system_version"`
104108
}
105109

110+
func (o *OpenvSwitch) Table() string {
111+
return "Open_vSwitch"
112+
}
113+
106114
var defDB, _ = model.NewClientDBModel("Open_vSwitch",
107115
map[string]model.Model{
108116
"Open_vSwitch": &OpenvSwitch{},

‎client/condition.go

+3-5
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package client
22

33
import (
44
"fmt"
5-
"reflect"
65

76
"github.com/ovn-org/libovsdb/cache"
87
"github.com/ovn-org/libovsdb/mapper"
@@ -176,7 +175,7 @@ func newExplicitConditional(table string, cache *cache.TableCache, matchAll bool
176175
// to match on models.
177176
type predicateConditional struct {
178177
tableName string
179-
predicate interface{}
178+
predicate func(model.Model) bool
180179
cache *cache.TableCache
181180
}
182181

@@ -192,8 +191,7 @@ func (c *predicateConditional) Matches() (map[string]model.Model, error) {
192191
// run the predicate on a shallow copy of the models for speed and only
193192
// clone the matches
194193
for u, m := range tableCache.RowsShallow() {
195-
ret := reflect.ValueOf(c.predicate).Call([]reflect.Value{reflect.ValueOf(m)})
196-
if ret[0].Bool() {
194+
if c.predicate(m) {
197195
found[u] = model.Clone(m)
198196
}
199197
}
@@ -215,7 +213,7 @@ func (c *predicateConditional) Generate() ([][]ovsdb.Condition, error) {
215213
}
216214

217215
// newPredicateConditional creates a new predicateConditional
218-
func newPredicateConditional(table string, cache *cache.TableCache, predicate interface{}) (Conditional, error) {
216+
func newPredicateConditional(table string, cache *cache.TableCache, predicate func(model.Model) bool) (Conditional, error) {
219217
return &predicateConditional{
220218
tableName: table,
221219
predicate: predicate,

0 commit comments

Comments
 (0)