client: Speed up row access by index

This adds a new api to the cache - RowByMdoel which will calculate the will look up by the UUID of the model, or by the matching the model against the table indexes. This is much faster as it doesn't need to iterate across the entire cache. The Get API has been updated to use this new function Fixes: ovn-org#150 Signed-off-by: Dave Tucker <[email protected]>
dave-tucker · Jun 24, 2021 · 1f085bb · 1f085bb
1 parent d0d9dbf
commit 1f085bb
Show file tree

Hide file tree

Showing 3 changed files with 186 additions and 27 deletions.
diff --git a/cache/cache.go b/cache/cache.go
@@ -92,6 +92,37 @@ func (r *RowCache) row(uuid string) model.Model {
 	return nil
 }
 
+// RowByModel searches the cache using a the indexes for a provided model
+func (r *RowCache) RowByModel(m model.Model) model.Model {
+	r.mutex.RLock()
+	defer r.mutex.RUnlock()
+	return r.rowByModel(m)
+}
+
+func (r *RowCache) rowByModel(m model.Model) model.Model {
+	if reflect.TypeOf(m) != r.dataType {
+		return nil
+	}
+	info, _ := mapper.NewInfo(&r.schema, m)
+	uuid, err := info.FieldByColumn("_uuid")
+	if err != nil {
+		return nil
+	}
+	if uuid.(string) != "" {
+		return r.row(uuid.(string))
+	}
+	for index := range r.indexes {
+		val, err := valueFromIndex(info, index)
+		if err != nil {
+			continue
+		}
+		if uuid, ok := r.indexes[index][val]; ok {
+			return r.row(uuid)
+		}
+	}
+	return nil
+}
+
 // Create writes the provided content to the cache
 func (r *RowCache) Create(uuid string, m model.Model) error {
 	r.mutex.Lock()

diff --git a/cache/cache_test.go b/cache/cache_test.go
@@ -841,3 +841,149 @@ func TestIndex(t *testing.T) {
 		assert.Equal(t, idx.columns(), []string{"foo", "bar"})
 	})
 }
+
+func TestTableCacheRowByModelSingleIndex(t *testing.T) {
+	var schema ovsdb.DatabaseSchema
+	db, err := model.NewDBModel("Open_vSwitch", map[string]model.Model{"Open_vSwitch": &testModel{}})
+	require.Nil(t, err)
+	err = json.Unmarshal([]byte(`
+		 {"name": "TestDB",
+		  "tables": {
+		    "Open_vSwitch": {
+			  "indexes": [["foo"]],
+		      "columns": {
+		        "foo": {
+			  "type": "string"
+			},
+			"bar": {
+				"type": "string"
+			  }
+		      }
+		    }
+		 }
+	     }
+	`), &schema)
+	require.NoError(t, err)
+	myFoo := &testModel{Foo: "foo", Bar: "foo"}
+	testData := Data{
+		"Open_vSwitch": map[string]model.Model{
+			"foo": myFoo,
+			"bar": &testModel{Foo: "bar", Bar: "bar"},
+		},
+	}
+	tc, err := NewTableCache(&schema, db, testData)
+	require.NoError(t, err)
+
+	t.Run("get foo by index", func(t *testing.T) {
+		foo := tc.Table("Open_vSwitch").RowByModel(&testModel{Foo: "foo"})
+		assert.NotNil(t, foo)
+		assert.Equal(t, myFoo, foo)
+	})
+
+	t.Run("get non-existent item by index", func(t *testing.T) {
+		baz := tc.Table("Open_vSwitch").RowByModel(&testModel{Foo: "baz"})
+		assert.Nil(t, baz)
+	})
+
+	t.Run("no index data", func(t *testing.T) {
+		foo := tc.Table("Open_vSwitch").RowByModel(&testModel{Bar: "foo"})
+		assert.Nil(t, foo)
+	})
+}
+
+func TestTableCacheRowByModelTwoIndexes(t *testing.T) {
+	var schema ovsdb.DatabaseSchema
+	db, err := model.NewDBModel("Open_vSwitch", map[string]model.Model{"Open_vSwitch": &testModel{}})
+	require.Nil(t, err)
+	err = json.Unmarshal([]byte(`
+		 {"name": "TestDB",
+		  "tables": {
+		    "Open_vSwitch": {
+			  "indexes": [["foo"], ["bar"]],
+		      "columns": {
+		        "foo": {
+			  "type": "string"
+			},
+			"bar": {
+				"type": "string"
+			  }
+		      }
+		    }
+		 }
+	     }
+	`), &schema)
+	require.NoError(t, err)
+	myFoo := &testModel{Foo: "foo", Bar: "foo"}
+	testData := Data{
+		"Open_vSwitch": map[string]model.Model{
+			"foo": myFoo,
+			"bar": &testModel{Foo: "bar", Bar: "bar"},
+		},
+	}
+	tc, err := NewTableCache(&schema, db, testData)
+	require.NoError(t, err)
+
+	t.Run("get foo by Foo index", func(t *testing.T) {
+		foo := tc.Table("Open_vSwitch").RowByModel(&testModel{Foo: "foo"})
+		assert.NotNil(t, foo)
+		assert.Equal(t, myFoo, foo)
+	})
+
+	t.Run("get foo by Bar index", func(t *testing.T) {
+		foo := tc.Table("Open_vSwitch").RowByModel(&testModel{Bar: "foo"})
+		assert.NotNil(t, foo)
+		assert.Equal(t, myFoo, foo)
+	})
+
+	t.Run("get non-existent item by index", func(t *testing.T) {
+		baz := tc.Table("Open_vSwitch").RowByModel(&testModel{Foo: "baz"})
+		assert.Nil(t, baz)
+	})
+
+}
+
+func TestTableCacheRowByModelMultiIndex(t *testing.T) {
+	var schema ovsdb.DatabaseSchema
+	db, err := model.NewDBModel("Open_vSwitch", map[string]model.Model{"Open_vSwitch": &testModel{}})
+	require.Nil(t, err)
+	err = json.Unmarshal([]byte(`
+		 {"name": "TestDB",
+		  "tables": {
+		    "Open_vSwitch": {
+			  "indexes": [["foo", "bar"]],
+		      "columns": {
+		        "foo": {
+			  "type": "string"
+			},
+			"bar": {
+				"type": "string"
+			  }
+		      }
+		    }
+		 }
+	     }
+	`), &schema)
+	require.NoError(t, err)
+	myFoo := &testModel{Foo: "foo", Bar: "foo"}
+	testData := Data{
+		"Open_vSwitch": map[string]model.Model{"foo": myFoo, "bar": &testModel{Foo: "bar", Bar: "bar"}},
+	}
+	tc, err := NewTableCache(&schema, db, testData)
+	require.NoError(t, err)
+
+	t.Run("incomplete index", func(t *testing.T) {
+		foo := tc.Table("Open_vSwitch").RowByModel(&testModel{Foo: "foo"})
+		assert.Nil(t, foo)
+	})
+
+	t.Run("get foo by index", func(t *testing.T) {
+		foo := tc.Table("Open_vSwitch").RowByModel(&testModel{Foo: "foo", Bar: "foo"})
+		assert.NotNil(t, foo)
+		assert.Equal(t, myFoo, foo)
+	})
+
+	t.Run("get non-existent item by index", func(t *testing.T) {
+		baz := tc.Table("Open_vSwitch").RowByModel(&testModel{Foo: "baz", Bar: "baz"})
+		assert.Nil(t, baz)
+	})
+}
diff --git a/client/api.go b/client/api.go
@@ -3,6 +3,7 @@ package client
 import (
 	"errors"
 	"fmt"
+	"log"
 	"reflect"
 
 	"github.com/ovn-org/libovsdb/cache"
@@ -210,7 +211,7 @@ func (a api) conditionFromModel(any bool, model model.Model, cond ...model.Condi
 // a instance of any row in the cache.
 // 'result' must be a pointer to an Model that exists in the DBModel
 //
-// The way the cache is search depends on the fields already populated in 'result'
+// The way the cache is searched depends on the fields already populated in 'result'
 // Any table index (including _uuid) will be used for comparison
 func (a api) Get(m model.Model) error {
 	table, err := a.getTableFromModel(m)
@@ -220,36 +221,17 @@ func (a api) Get(m model.Model) error {
 
 	tableCache := a.cache.Table(table)
 	if tableCache == nil {
+		log.Print("no table")
 		return ErrNotFound
 	}
 
-	// If model contains _uuid value, we can access it via cache index
-	mapperInfo, err := mapper.NewInfo(a.cache.Mapper().Schema.Table(table), m)
-	if err != nil {
-		return err
-	}
-	if uuid, err := mapperInfo.FieldByColumn("_uuid"); err != nil && uuid != nil {
-		found := tableCache.Row(uuid.(string))
-		if found == nil {
-			return ErrNotFound
-		}
-		reflect.ValueOf(m).Elem().Set(reflect.Indirect(reflect.ValueOf(found)))
-		return nil
-	}
-
-	// Look across the entire cache for table index equality
-	for _, row := range tableCache.Rows() {
-		elem := tableCache.Row(row)
-		equal, err := a.cache.Mapper().EqualFields(table, m, elem.(model.Model))
-		if err != nil {
-			return err
-		}
-		if equal {
-			reflect.ValueOf(m).Elem().Set(reflect.Indirect(reflect.ValueOf(elem)))
-			return nil
-		}
+	found := tableCache.RowByModel(m)
+	if found == nil {
+		log.Print("not found")
+		return ErrNotFound
 	}
-	return ErrNotFound
+	reflect.ValueOf(m).Elem().Set(reflect.Indirect(reflect.ValueOf(found)))
+	return nil
 }
 
 // Create is a generic function capable of creating any row in the DB