From bbfce10f40516c3d3fd63d726704357c02396e6d Mon Sep 17 00:00:00 2001 From: Yan Zhu Date: Mon, 18 Sep 2023 18:38:45 +0800 Subject: [PATCH] refactor WhereCache func to import performance --- client/api.go | 42 ++++------------- client/api_test.go | 98 ++++++++++++++++------------------------ client/client.go | 4 +- client/client_test.go | 8 ++++ client/condition.go | 9 ++-- client/condition_test.go | 8 ++-- model/model.go | 19 ++++---- test/test_data.go | 12 +++++ 8 files changed, 89 insertions(+), 111 deletions(-) diff --git a/client/api.go b/client/api.go index 8d5a30ef..92faf4d3 100644 --- a/client/api.go +++ b/client/api.go @@ -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 @@ -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))) } @@ -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) } @@ -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{ diff --git a/client/api_test.go b/client/api_test.go index 7c768109..fea0d14a 100644 --- a/client/api_test.go +++ b/client/api_test.go @@ -217,13 +217,15 @@ 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{}, @@ -231,7 +233,8 @@ func TestAPIListPredicate(t *testing.T) { }, { name: "all", - predicate: func(t *testLogicalSwitch) bool { + m: &testLogicalSwitch{}, + predicate: func(t model.Model) bool { return true }, content: lscacheList, @@ -239,30 +242,26 @@ func TestAPIListPredicate(t *testing.T) { }, { 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) @@ -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, @@ -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 { @@ -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{ @@ -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" }) }, @@ -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" }) }, @@ -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" }) }, @@ -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 }) }, @@ -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 }) }, @@ -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 }, }, @@ -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]) } @@ -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{}) { diff --git a/client/client.go b/client/client.go index 10ea757e..7a526c27 100644 --- a/client/client.go +++ b/client/client.go @@ -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) } diff --git a/client/client_test.go b/client/client_test.go index 1f66651a..23ac2da9 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -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"` @@ -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{}, diff --git a/client/condition.go b/client/condition.go index 1dfabda0..ad973f29 100644 --- a/client/condition.go +++ b/client/condition.go @@ -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" @@ -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 } @@ -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) } } @@ -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, diff --git a/client/condition_test.go b/client/condition_test.go index 5bd63b33..00fa8eeb 100644 --- a/client/condition_test.go +++ b/client/condition_test.go @@ -204,14 +204,15 @@ func TestPredicateConditional(t *testing.T) { test := []struct { name string - predicate interface{} + predicate func(model.Model) bool condition [][]ovsdb.Condition matches map[string]model.Model err bool }{ { name: "simple value comparison", - predicate: func(lsp *testLogicalSwitchPort) bool { + predicate: func(m model.Model) bool { + lsp := m.(*testLogicalSwitchPort) return lsp.UUID == aUUID0 }, condition: [][]ovsdb.Condition{ @@ -225,7 +226,8 @@ func TestPredicateConditional(t *testing.T) { }, { name: "by random field", - predicate: func(lsp *testLogicalSwitchPort) bool { + predicate: func(m model.Model) bool { + lsp := m.(*testLogicalSwitchPort) return lsp.Enabled != nil && *lsp.Enabled == false }, condition: [][]ovsdb.Condition{ diff --git a/model/model.go b/model/model.go index c8575f5b..9e83dfcb 100644 --- a/model/model.go +++ b/model/model.go @@ -16,13 +16,16 @@ import ( // The struct may also have non-tagged fields (which will be ignored by the API calls) // The Model interface must be implemented by the pointer to such type // Example: -//type MyLogicalRouter struct { -// UUID string `ovsdb:"_uuid"` -// Name string `ovsdb:"name"` -// ExternalIDs map[string]string `ovsdb:"external_ids"` -// LoadBalancers []string `ovsdb:"load_balancer"` -//} -type Model interface{} +// +// type MyLogicalRouter struct { +// UUID string `ovsdb:"_uuid"` +// Name string `ovsdb:"name"` +// ExternalIDs map[string]string `ovsdb:"external_ids"` +// LoadBalancers []string `ovsdb:"load_balancer"` +// } +type Model interface { + Table() string +} type CloneableModel interface { CloneModel() Model @@ -43,7 +46,7 @@ func Clone(a Model) Model { b := reflect.New(val.Type()).Interface() aBytes, _ := json.Marshal(a) _ = json.Unmarshal(aBytes, b) - return b + return b.(Model) } // CloneInto deep copies a model into another one diff --git a/test/test_data.go b/test/test_data.go index 6be6b8b2..311eec92 100644 --- a/test/test_data.go +++ b/test/test_data.go @@ -144,12 +144,20 @@ type BridgeType struct { Status map[string]string `ovsdb:"status"` } +func (b *BridgeType) Table() string { + return "Bridge" +} + // OvsType is the simplified ORM model of the Bridge table type OvsType struct { UUID string `ovsdb:"_uuid"` Bridges []string `ovsdb:"bridges"` } +func (o *OvsType) Table() string { + return "OVS" +} + type FlowSampleCollectorSetType struct { UUID string `ovsdb:"_uuid"` Bridge string `ovsdb:"bridge"` @@ -158,6 +166,10 @@ type FlowSampleCollectorSetType struct { IPFIX *string // `ovsdb:"ipfix"` } +func (f *FlowSampleCollectorSetType) Table() string { + return "FlowSampleCollectorSet" +} + func GetModel() (model.DatabaseModel, error) { client, err := model.NewClientDBModel( "Open_vSwitch",