Merge pull request #279 from dcbw/avoid-multi-level-map-access

cache: avoid repeated map lookups

cache/cache.go

@@ -127,12 +127,12 @@ func (r *RowCache) RowByModel(m model.Model) model.Model {
 	if uuid.(string) != "" {
 		return r.rowByUUID(uuid.(string))
 	}
-	for index := range r.indexes {
+	for index, vals := range r.indexes {
 		val, err := valueFromIndex(info, index)
 		if err != nil {
 			continue
 		}
-		if uuid, ok := r.indexes[index][val]; ok {
+		if uuid, ok := vals[val]; ok {
 			return r.rowByUUID(uuid)
 		}
 	}
@@ -154,13 +154,13 @@ func (r *RowCache) Create(uuid string, m model.Model, checkIndexes bool) error {
 		return err
 	}
 	newIndexes := newColumnToValue(r.dbModel.Schema.Table(r.name).Indexes)
-	for index := range r.indexes {
+	for index, vals := range r.indexes {
 		val, err := valueFromIndex(info, index)
 		if err != nil {
 			return err
 		}
 
-		if existing, ok := r.indexes[index][val]; ok && checkIndexes {
+		if existing, ok := vals[val]; ok && checkIndexes {
 			return NewIndexExistsError(r.name, val, string(index), uuid, existing)
 		}
 
@@ -169,8 +169,9 @@ func (r *RowCache) Create(uuid string, m model.Model, checkIndexes bool) error {
 
 	// write indexes
 	for k1, v1 := range newIndexes {
+		vals := r.indexes[k1]
 		for k2, v2 := range v1 {
-			r.indexes[k1][k2] = v2
+			vals[k2] = v2
 		}
 	}
 	r.cache[uuid] = model.Clone(m)
@@ -197,7 +198,7 @@ func (r *RowCache) Update(uuid string, m model.Model, checkIndexes bool) error {
 	newIndexes := newColumnToValue(indexes)
 	oldIndexes := newColumnToValue(indexes)
 	var errs []error
-	for index := range r.indexes {
+	for index, vals := range r.indexes {
 		var err error
 		oldVal, err := valueFromIndex(oldInfo, index)
 		if err != nil {
@@ -215,7 +216,7 @@ func (r *RowCache) Update(uuid string, m model.Model, checkIndexes bool) error {
 		// old and new values are NOT the same
 
 		// check that there are no conflicts
-		if conflict, ok := r.indexes[index][newVal]; ok && checkIndexes && conflict != uuid {
+		if conflict, ok := vals[newVal]; ok && checkIndexes && conflict != uuid {
 			errs = append(errs, NewIndexExistsError(
 				r.name,
 				newVal,
@@ -233,14 +234,16 @@ func (r *RowCache) Update(uuid string, m model.Model, checkIndexes bool) error {
 	}
 	// write indexes
 	for k1, v1 := range newIndexes {
+		vals := r.indexes[k1]
 		for k2, v2 := range v1 {
-			r.indexes[k1][k2] = v2
+			vals[k2] = v2
 		}
 	}
 	// delete old indexes
 	for k1, v1 := range oldIndexes {
+		vals := r.indexes[k1]
 		for k2 := range v1 {
-			delete(r.indexes[k1], k2)
+			delete(vals, k2)
 		}
 	}
 	r.cache[uuid] = model.Clone(m)
@@ -256,12 +259,12 @@ func (r *RowCache) IndexExists(row model.Model) error {
 	if err != nil {
 		return nil
 	}
-	for index := range r.indexes {
+	for index, vals := range r.indexes {
 		val, err := valueFromIndex(info, index)
 		if err != nil {
 			continue
 		}
-		if existing, ok := r.indexes[index][val]; ok && existing != uuid.(string) {
+		if existing, ok := vals[val]; ok && existing != uuid.(string) {
 			return NewIndexExistsError(
 				r.name,
 				val,
@@ -286,16 +289,16 @@ func (r *RowCache) Delete(uuid string) error {
 	if err != nil {
 		return err
 	}
-	for index := range r.indexes {
+	for index, vals := range r.indexes {
 		oldVal, err := valueFromIndex(oldInfo, index)
 		if err != nil {
 			return err
 		}
 		// only remove the index if it is pointing to this uuid
 		// otherwise we can cause a consistency issue if we've processed
 		// updates out of order
-		if r.indexes[index][oldVal] == uuid {
-			delete(r.indexes[index], oldVal)
+		if vals[oldVal] == uuid {
+			delete(vals, oldVal)
 		}
 	}
 	delete(r.cache, uuid)
@@ -470,8 +473,9 @@ func NewTableCache(dbModel model.DatabaseModel, data Data, logger *logr.Logger)
 		if _, ok := dbModel.Schema.Tables[table]; !ok {
 			return nil, fmt.Errorf("table %s is not in schema", table)
 		}
+		rowCache := cache[table]
 		for uuid, row := range rowData {
-			if err := cache[table].Create(uuid, row, true); err != nil {
+			if err := rowCache.Create(uuid, row, true); err != nil {
 				return nil, err
 			}
 		}

cache/cache_test.go

@@ -2,6 +2,7 @@ package cache
 
 import (
 	"encoding/json"
+	"fmt"
 	"reflect"
 	"testing"
 
@@ -1200,7 +1201,7 @@ func TestIndex(t *testing.T) {
 	})
 }
 
-func TestTableCacheRowByModelSingleIndex(t *testing.T) {
+func setupRowByModelSingleIndex(t require.TestingT) (*testModel, *TableCache) {
 	var schema ovsdb.DatabaseSchema
 	db, err := model.NewClientDBModel("Open_vSwitch", map[string]model.Model{"Open_vSwitch": &testModel{}})
 	require.Nil(t, err)
@@ -1214,8 +1215,8 @@ func TestTableCacheRowByModelSingleIndex(t *testing.T) {
 			  "type": "string"
 			},
 			"bar": {
-				"type": "string"
-			  }
+			  "type": "string"
+			}
 		      }
 		    }
 		 }
@@ -1234,6 +1235,12 @@ func TestTableCacheRowByModelSingleIndex(t *testing.T) {
 	tc, err := NewTableCache(dbModel, testData, nil)
 	require.NoError(t, err)
 
+	return myFoo, tc
+}
+
+func TestTableCacheRowByModelSingleIndex(t *testing.T) {
+	myFoo, tc := setupRowByModelSingleIndex(t)
+
 	t.Run("get foo by index", func(t *testing.T) {
 		foo := tc.Table("Open_vSwitch").RowByModel(&testModel{Foo: "foo"})
 		assert.NotNil(t, foo)
@@ -1251,6 +1258,67 @@ func TestTableCacheRowByModelSingleIndex(t *testing.T) {
 	})
 }
 
+func benchmarkDoCreate(b *testing.B, numRows int) *RowCache {
+	_, tc := setupRowByModelSingleIndex(b)
+
+	rc := tc.Table("Open_vSwitch")
+	for i := 0; i < numRows; i++ {
+		uuid := fmt.Sprintf("%d", i)
+		model := &testModel{Foo: uuid}
+		err := rc.Create(uuid, model, true)
+		require.NoError(b, err)
+	}
+
+	return rc
+}
+
+const numRows int = 10000
+
+func BenchmarkSingleIndexCreate(b *testing.B) {
+	for n := 0; n < b.N; n++ {
+		_ = benchmarkDoCreate(b, numRows)
+	}
+}
+
+func BenchmarkSingleIndexUpdate(b *testing.B) {
+	rc := benchmarkDoCreate(b, numRows)
+
+	b.ResetTimer()
+	for n := 0; n < b.N; n++ {
+		for i := 0; i < numRows; i++ {
+			uuid := fmt.Sprintf("%d", i)
+			model := &testModel{Foo: fmt.Sprintf("%d-%d", n, i)}
+			err := rc.Update(uuid, model, true)
+			require.NoError(b, err)
+		}
+	}
+}
+
+func BenchmarkSingleIndexDelete(b *testing.B) {
+	for n := 0; n < b.N; n++ {
+		rc := benchmarkDoCreate(b, numRows)
+		for i := 0; i < numRows; i++ {
+			uuid := fmt.Sprintf("%d", i)
+			err := rc.Delete(uuid)
+			require.NoError(b, err)
+		}
+	}
+}
+
+func BenchmarkIndexExists(b *testing.B) {
+	rc := benchmarkDoCreate(b, numRows)
+
+	b.ResetTimer()
+	for n := 0; n < b.N; n++ {
+		for i := 0; i < numRows; i++ {
+			uuid := fmt.Sprintf("%d", i)
+			model := &testModel{UUID: uuid, Foo: uuid}
+			err := rc.IndexExists(model)
+			require.NoError(b, err)
+		}
+	}
+}
+
 func TestTableCacheRowByModelTwoIndexes(t *testing.T) {
 	var schema ovsdb.DatabaseSchema
 	db, err := model.NewClientDBModel("Open_vSwitch", map[string]model.Model{"Open_vSwitch": &testModel{}})