Skip to content

Commit

Permalink
refactor WhereCache func to import performance
Browse files Browse the repository at this point in the history
  • Loading branch information
@halfcrazy
halfcrazy committed Sep 19, 2023
1 parent 8883b97 commit bbfce10
Show file tree
Hide file tree
Showing 8 changed files with 89 additions and 111 deletions.
42 changes: 8 additions & 34 deletions client/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type API interface {
// Create a Conditional API from a Function that is used to filter cached data
// The function must accept a Model implementation and return a boolean. E.g:
// ConditionFromFunc(func(l *LogicalSwitch) bool { return l.Enabled })
WhereCache(predicate interface{}) ConditionalAPI
WhereCache(model.Model, func(model.Model) bool) ConditionalAPI

// Create a ConditionalAPI from a Model's index data, where operations
// apply to elements that match the values provided in one or more
Expand Down Expand Up @@ -121,12 +121,12 @@ func (a api) List(ctx context.Context, result interface{}) error {
var appendValue func(reflect.Value)
var m model.Model
if resultVal.Type().Elem().Kind() == reflect.Ptr {
m = reflect.New(resultVal.Type().Elem().Elem()).Interface()
m = reflect.New(resultVal.Type().Elem().Elem()).Interface().(model.Model)
appendValue = func(v reflect.Value) {
resultVal.Set(reflect.Append(resultVal, v))
}
} else {
m = reflect.New(resultVal.Type().Elem()).Interface()
m = reflect.New(resultVal.Type().Elem()).Interface().(model.Model)
appendValue = func(v reflect.Value) {
resultVal.Set(reflect.Append(resultVal, reflect.Indirect(v)))
}
Expand Down Expand Up @@ -194,14 +194,14 @@ func (a api) WhereAll(m model.Model, cond ...model.Condition) ConditionalAPI {
}

// WhereCache returns a conditionalAPI based a Predicate
func (a api) WhereCache(predicate interface{}) ConditionalAPI {
return newConditionalAPI(a.cache, a.conditionFromFunc(predicate), a.logger)
func (a api) WhereCache(m model.Model, predicate func(model.Model) bool) ConditionalAPI {
return newConditionalAPI(a.cache, a.conditionFromFunc(m, predicate), a.logger)
}

// Conditional interface implementation
// FromFunc returns a Condition from a function
func (a api) conditionFromFunc(predicate interface{}) Conditional {
table, err := a.getTableFromFunc(predicate)
func (a api) conditionFromFunc(m model.Model, predicate func(model.Model) bool) Conditional {
table, err := a.getTableFromModel(m)
if err != nil {
return newErrorConditional(err)
}
Expand Down Expand Up @@ -542,39 +542,13 @@ func (a api) getTableFromModel(m interface{}) (string, error) {
if _, ok := m.(model.Model); !ok {
return "", &ErrWrongType{reflect.TypeOf(m), "Type does not implement Model interface"}
}
table := a.cache.DatabaseModel().FindTable(reflect.TypeOf(m))
table := m.(model.Model).Table()
if table == "" {
return "", &ErrWrongType{reflect.TypeOf(m), "Model not found in Database Model"}
}
return table, nil
}

// getTableFromModel returns the table name from a the predicate after performing
// type verifications
func (a api) getTableFromFunc(predicate interface{}) (string, error) {
predType := reflect.TypeOf(predicate)
if predType == nil || predType.Kind() != reflect.Func {
return "", &ErrWrongType{predType, "Expected function"}
}
if predType.NumIn() != 1 || predType.NumOut() != 1 || predType.Out(0).Kind() != reflect.Bool {
return "", &ErrWrongType{predType, "Expected func(Model) bool"}
}

modelInterface := reflect.TypeOf((*model.Model)(nil)).Elem()
modelType := predType.In(0)
if !modelType.Implements(modelInterface) {
return "", &ErrWrongType{predType,
fmt.Sprintf("Type %s does not implement Model interface", modelType.String())}
}

table := a.cache.DatabaseModel().FindTable(modelType)
if table == "" {
return "", &ErrWrongType{predType,
fmt.Sprintf("Model %s not found in Database Model", modelType.String())}
}
return table, nil
}

// newAPI returns a new API to interact with the database
func newAPI(cache *cache.TableCache, logger *logr.Logger) API {
return api{
Expand Down
98 changes: 40 additions & 58 deletions client/api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,52 +217,51 @@ func TestAPIListPredicate(t *testing.T) {

test := []struct {
name string
predicate interface{}
m model.Model
predicate func(model.Model) bool
content []model.Model
err bool
}{
{
name: "none",
predicate: func(t *testLogicalSwitch) bool {
m: &testLogicalSwitch{},
predicate: func(t model.Model) bool {
return false
},
content: []model.Model{},
err: false,
},
{
name: "all",
predicate: func(t *testLogicalSwitch) bool {
m: &testLogicalSwitch{},
predicate: func(t model.Model) bool {
return true
},
content: lscacheList,
err: false,
},
{
name: "nil function must fail",
m: &testLogicalSwitch{},
err: true,
},
{
name: "arbitrary condition",
predicate: func(t *testLogicalSwitch) bool {
m: &testLogicalSwitch{},
predicate: func(m model.Model) bool {
t := m.(*testLogicalSwitch)
return strings.HasPrefix(t.Name, "magic")
},
content: []model.Model{lscacheList[1], lscacheList[3]},
err: false,
},
{
name: "error wrong type",
predicate: func(t testLogicalSwitch) string {
return "foo"
},
err: true,
},
}

for _, tt := range test {
t.Run(fmt.Sprintf("ApiListPredicate: %s", tt.name), func(t *testing.T) {
var result []*testLogicalSwitch
api := newAPI(tcache, &discardLogger)
cond := api.WhereCache(tt.predicate)
cond := api.WhereCache(tt.m, tt.predicate)
err := cond.List(context.Background(), &result)
if tt.err {
assert.NotNil(t, err)
Expand Down Expand Up @@ -536,26 +535,14 @@ func TestAPIListMulti(t *testing.T) {
func TestConditionFromFunc(t *testing.T) {
test := []struct {
name string
arg interface{}
m model.Model
arg func(model.Model) bool
err bool
}{
{
name: "wrong function must fail",
arg: func(s string) bool {
return false
},
err: true,
},
{
name: "wrong function must fail2 ",
arg: func(t *testLogicalSwitch) string {
return "foo"
},
err: true,
},
{
name: "correct func should succeed",
arg: func(t *testLogicalSwitch) bool {
m: &testLogicalSwitch{},
arg: func(m model.Model) bool {
return true
},
err: false,
Expand All @@ -566,7 +553,7 @@ func TestConditionFromFunc(t *testing.T) {
t.Run(fmt.Sprintf("conditionFromFunc: %s", tt.name), func(t *testing.T) {
cache := apiTestCache(t, nil)
apiIface := newAPI(cache, &discardLogger)
condition := apiIface.(api).conditionFromFunc(tt.arg)
condition := apiIface.(api).conditionFromFunc(tt.m, tt.arg)
if tt.err {
assert.IsType(t, &errorConditional{}, condition)
} else {
Expand All @@ -584,23 +571,6 @@ func TestConditionFromModel(t *testing.T) {
conds []model.Condition
err bool
}{
{
name: "wrong model must fail",
models: []model.Model{
&struct{ a string }{},
},
err: true,
},
{
name: "wrong condition must fail",
models: []model.Model{
&struct {
a string `ovsdb:"_uuid"`
}{},
},
conds: []model.Condition{{Field: "foo"}},
err: true,
},
{
name: "correct model must succeed",
models: []model.Model{
Expand Down Expand Up @@ -1009,7 +979,8 @@ func TestAPIMutate(t *testing.T) {
{
name: "select single by predicate name insert element in map",
condition: func(a API) ConditionalAPI {
return a.WhereCache(func(lsp *testLogicalSwitchPort) bool {
return a.WhereCache(&testLogicalSwitchPort{}, func(m model.Model) bool {
lsp := m.(*testLogicalSwitchPort)
return lsp.Name == "lsp2"
})
},
Expand All @@ -1033,7 +1004,8 @@ func TestAPIMutate(t *testing.T) {
{
name: "select many by predicate name insert element in map",
condition: func(a API) ConditionalAPI {
return a.WhereCache(func(lsp *testLogicalSwitchPort) bool {
return a.WhereCache(&testLogicalSwitchPort{}, func(m model.Model) bool {
lsp := m.(*testLogicalSwitchPort)
return lsp.Type == "someType"
})
},
Expand Down Expand Up @@ -1063,7 +1035,8 @@ func TestAPIMutate(t *testing.T) {
{
name: "No mutations should error",
condition: func(a API) ConditionalAPI {
return a.WhereCache(func(lsp *testLogicalSwitchPort) bool {
return a.WhereCache(&testLogicalSwitchPort{}, func(m model.Model) bool {
lsp := m.(*testLogicalSwitchPort)
return lsp.Type == "someType"
})
},
Expand Down Expand Up @@ -1411,7 +1384,8 @@ func TestAPIUpdate(t *testing.T) {
{
name: "select multiple by predicate change multiple field",
condition: func(a API) ConditionalAPI {
return a.WhereCache(func(t *testLogicalSwitchPort) bool {
return a.WhereCache(&testLogicalSwitchPort{}, func(m model.Model) bool {
t := m.(*testLogicalSwitchPort)
return t.Enabled != nil && *t.Enabled == true
})
},
Expand Down Expand Up @@ -1645,7 +1619,8 @@ func TestAPIDelete(t *testing.T) {
{
name: "select multiple by predicate",
condition: func(a API) ConditionalAPI {
return a.WhereCache(func(t *testLogicalSwitchPort) bool {
return a.WhereCache(&testLogicalSwitchPort{}, func(m model.Model) bool {
t := m.(*testLogicalSwitchPort)
return t.Enabled != nil && *t.Enabled == true
})
},
Expand Down Expand Up @@ -1724,29 +1699,36 @@ func BenchmarkAPIList(b *testing.B) {

test := []struct {
name string
predicate interface{}
m model.Model
predicate func(model.Model) bool
}{
{
name: "predicate returns none",
predicate: func(t *testLogicalSwitchPort) bool {
m: &testLogicalSwitchPort{},
predicate: func(t model.Model) bool {
return false
},
},
{
name: "predicate returns all",
predicate: func(t *testLogicalSwitchPort) bool {
m: &testLogicalSwitchPort{},
predicate: func(t model.Model) bool {
return true
},
},
{
name: "predicate on an arbitrary condition",
predicate: func(t *testLogicalSwitchPort) bool {
m: &testLogicalSwitchPort{},
predicate: func(m model.Model) bool {
t := m.(*testLogicalSwitchPort)
return strings.HasPrefix(t.Name, "ls1")
},
},
{
name: "predicate matches name",
predicate: func(t *testLogicalSwitchPort) bool {
m: &testLogicalSwitchPort{},
predicate: func(m model.Model) bool {
t := m.(*testLogicalSwitchPort)
return t.Name == lscacheList[index].Name
},
},
Expand All @@ -1763,7 +1745,7 @@ func BenchmarkAPIList(b *testing.B) {
api := newAPI(tcache, &discardLogger)
var cond ConditionalAPI
if tt.predicate != nil {
cond = api.WhereCache(tt.predicate)
cond = api.WhereCache(tt.m, tt.predicate)
} else {
cond = api.Where(lscacheList[index])
}
Expand Down Expand Up @@ -1979,7 +1961,7 @@ func TestAPIWait(t *testing.T) {
{
name: "no operation",
condition: func(a API) ConditionalAPI {
return a.WhereCache(func(t *testLogicalSwitchPort) bool { return false })
return a.WhereCache(&testLogicalSwitchPort{}, func(t model.Model) bool { return false })
},
until: "==",
prepare: func() (model.Model, []interface{}) {
Expand Down
4 changes: 2 additions & 2 deletions client/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1475,6 +1475,6 @@ func (o *ovsdbClient) WhereAll(m model.Model, conditions ...model.Condition) Con
}

// WhereCache implements the API interface's WhereCache function
func (o *ovsdbClient) WhereCache(predicate interface{}) ConditionalAPI {
return o.primaryDB().api.WhereCache(predicate)
func (o *ovsdbClient) WhereCache(m model.Model, predicate func(model.Model) bool) ConditionalAPI {
return o.primaryDB().api.WhereCache(m, predicate)
}
8 changes: 8 additions & 0 deletions client/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ type Bridge struct {
STPEnable bool `ovsdb:"stp_enable"`
}

func (b *Bridge) Table() string {
return "Bridge"
}

// OpenvSwitch defines an object in Open_vSwitch table
type OpenvSwitch struct {
UUID string `ovsdb:"_uuid"`
Expand All @@ -103,6 +107,10 @@ type OpenvSwitch struct {
SystemVersion *string `ovsdb:"system_version"`
}

func (o *OpenvSwitch) Table() string {
return "Open_vSwitch"
}

var defDB, _ = model.NewClientDBModel("Open_vSwitch",
map[string]model.Model{
"Open_vSwitch": &OpenvSwitch{},
Expand Down
9 changes: 3 additions & 6 deletions client/condition.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ package client

import (
"fmt"
"reflect"

"github.com/ovn-org/libovsdb/cache"
"github.com/ovn-org/libovsdb/mapper"
"github.com/ovn-org/libovsdb/model"
Expand Down Expand Up @@ -176,7 +174,7 @@ func newExplicitConditional(table string, cache *cache.TableCache, matchAll bool
// to match on models.
type predicateConditional struct {
tableName string
predicate interface{}
predicate func(model.Model) bool
cache *cache.TableCache
}

Expand All @@ -192,8 +190,7 @@ func (c *predicateConditional) Matches() (map[string]model.Model, error) {
// run the predicate on a shallow copy of the models for speed and only
// clone the matches
for u, m := range tableCache.RowsShallow() {
ret := reflect.ValueOf(c.predicate).Call([]reflect.Value{reflect.ValueOf(m)})
if ret[0].Bool() {
if c.predicate(m) {
found[u] = model.Clone(m)
}
}
Expand All @@ -215,7 +212,7 @@ func (c *predicateConditional) Generate() ([][]ovsdb.Condition, error) {
}

// newPredicateConditional creates a new predicateConditional
func newPredicateConditional(table string, cache *cache.TableCache, predicate interface{}) (Conditional, error) {
func newPredicateConditional(table string, cache *cache.TableCache, predicate func(model.Model) bool) (Conditional, error) {
return &predicateConditional{
tableName: table,
predicate: predicate,
Expand Down
Loading

0 comments on commit bbfce10

Please sign in to comment.