Optimize ListEquals and Map.Equals, Add List.Range

Author: jpbetz

fieldpath/fromvalue.go

@@ -55,11 +55,13 @@ func (w *objectWalker) walk() {
 	case w.value.IsList():
 		// If the list were atomic, we'd break here, but we don't have
 		// a schema, so we can't tell.
-		list := w.value.AsList()
-		for i := 0; i < list.Length(); i++ {
+		iter := w.value.AsList().Range()
+		defer iter.Recycle()
+		for iter.Next() {
+			i, value := iter.Item()
 			w2 := *w
-			w2.path = append(w.path, GuessBestListPathElement(i, list.At(i)))
-			w2.value = list.At(i)
+			w2.path = append(w.path, GuessBestListPathElement(i, value))
+			w2.value = value
 			w2.walk()
 		}
 		return

typed/remove.go

@@ -50,8 +50,10 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) {
 	}
 
 	var newItems []interface{}
-	for i := 0; i < l.Length(); i++ {
-		item := l.At(i)
+	iter := l.Range()
+	defer iter.Recycle()
+	for iter.Next() {
+		i, item := iter.Item()
 		// Ignore error because we have already validated this list
 		pe, _ := listItemToPathElement(t, i, item)
 		path, _ := fieldpath.MakePath(pe)

value/list.go

@@ -23,6 +23,22 @@ type List interface {
 	// At returns the item at the given position in the map. It will
 	// panic if the index is out of range.
 	At(int) Value
+	// Range returns a ListRange for iterating over the items in the list.
+	Range() ListRange
+}
+
+// ListRange represents a single iteration across the items of a list.
+type ListRange interface {
+	// Next increments to the next item in the range, if there is one, and returns true, or returns false if there are no more items.
+	Next() bool
+	// Item returns the index and value of the current item in the range. or panics if there is no current item.
+	// For efficiency, Item may reuse the values returned by previous Item calls. Callers should be careful avoid holding
+	// pointers to the value returned by Item() that escape the iteration loop since they become invalid once either
+	// Item() or Recycle() is called.
+	Item() (index int, value Value)
+	// Recycle returns a ListRange that is no longer needed. The value returned by Item() becomes invalid once this is
+	// called.
+	Recycle()
 }
 
 // ListEquals compares two lists lexically.
@@ -31,16 +47,17 @@ func ListEquals(lhs, rhs List) bool {
 		return false
 	}
 
-	for i := 0; i < lhs.Length(); i++ {
-		lv := lhs.At(i)
-		rv := rhs.At(i)
+	lhsRange := lhs.Range()
+	defer lhsRange.Recycle()
+	rhsRange := rhs.Range()
+	defer rhsRange.Recycle()
+
+	for lhsRange.Next() && rhsRange.Next() {
+		_, lv := lhsRange.Item()
+		_, rv := rhsRange.Item()
 		if !Equals(lv, rv) {
-			lv.Recycle()
-			rv.Recycle()
 			return false
 		}
-		lv.Recycle()
-		rv.Recycle()
 	}
 	return true
 }
@@ -53,28 +70,31 @@ func ListLess(lhs, rhs List) bool {
 // ListCompare compares two lists lexically. The result will be 0 if l==rhs, -1
 // if l < rhs, and +1 if l > rhs.
 func ListCompare(lhs, rhs List) int {
-	i := 0
+	lhsRange := lhs.Range()
+	defer lhsRange.Recycle()
+	rhsRange := rhs.Range()
+	defer rhsRange.Recycle()
+
 	for {
-		if i >= lhs.Length() && i >= rhs.Length() {
+		lhsOk := lhsRange.Next()
+		rhsOk := rhsRange.Next()
+		if !lhsOk && !rhsOk {
 			// Lists are the same length and all items are equal.
 			return 0
 		}
-		if i >= lhs.Length() {
+		if !lhsOk {
 			// LHS is shorter.
 			return -1
 		}
-		if i >= rhs.Length() {
+		if !rhsOk {
 			// RHS is shorter.
 			return 1
 		}
-		lv := lhs.At(i)
-		rv := rhs.At(i)
+		_, lv := lhsRange.Item()
+		_, rv := rhsRange.Item()
 		if c := Compare(lv, rv); c != 0 {
 			return c
 		}
-		lv.Recycle()
-		rv.Recycle()
 		// The items are equal; continue.
-		i++
 	}
 }

value/listreflect.go

@@ -40,3 +40,33 @@ func (r listReflect) Unstructured() interface{} {
 	}
 	return result
 }
+
+func (r listReflect) Range() ListRange {
+	return &listReflectRange{r.Value, newTempValuePooler(), -1, r.Value.Len()}
+}
+
+type listReflectRange struct {
+	val    reflect.Value
+	pooler *tempValuePooler
+	i      int
+	length int
+}
+
+func (r *listReflectRange) Next() bool {
+	r.i += 1
+	return r.i < r.length
+}
+
+func (r *listReflectRange) Item() (index int, value Value) {
+	if r.i < 0 {
+		panic("Item() called before first calling Next()")
+	}
+	if r.i >= r.length {
+		panic("Item() called on ListRange with no more items")
+	}
+	return r.i, r.pooler.NewValueReflect(r.val.Index(r.i))
+}
+
+func (r *listReflectRange) Recycle() {
+	r.pooler.Recycle()
+}

value/listunstructured.go

@@ -25,3 +25,41 @@ func (l listUnstructured) Length() int {
 func (l listUnstructured) At(i int) Value {
 	return NewValueInterface(l[i])
 }
+
+func (l listUnstructured) Range() ListRange {
+	if len(l) == 0 {
+		return &listUnstructuredRange{l, nil, -1, 0}
+	}
+	vv := viPool.Get().(*valueUnstructured)
+	return &listUnstructuredRange{l, vv, -1, len(l)}
+}
+
+type listUnstructuredRange struct {
+	list   listUnstructured
+	vv     *valueUnstructured
+	i      int
+	length int
+}
+
+func (r *listUnstructuredRange) Next() bool {
+	r.i += 1
+	return r.i < r.length
+}
+
+func (r *listUnstructuredRange) Item() (index int, value Value) {
+	if r.i < 0 {
+		panic("Item() called before first calling Next()")
+	}
+	if r.i >= r.length {
+		panic("Item() called on ListRange with no more items")
+	}
+
+	r.vv.Value = r.list[r.i]
+	return r.i, r.vv
+}
+
+func (r *listUnstructuredRange) Recycle() {
+	if r.vv != nil {
+		r.vv.Recycle()
+	}
+}

value/map.go

@@ -99,6 +99,7 @@ func MapCompare(lhs, rhs Map) int {
 // MapEquals returns true if lhs == rhs, false otherwise. This function
 // acts on generic types and should not be used by callers, but can help
 // implement Map.Equals.
+// WARN: This is a naive implementation, calling lhs.Equals(rhs) is typically far more efficient.
 func MapEquals(lhs, rhs Map) bool {
 	if lhs.Length() != rhs.Length() {
 		return false

value/mapreflect.go

@@ -30,12 +30,17 @@ func (r mapReflect) Length() int {
 }
 
 func (r mapReflect) Get(key string) (Value, bool) {
-	mapKey := r.toMapKey(key)
-	val := r.Value.MapIndex(mapKey)
-	if !val.IsValid() {
+	k, v, ok := r.get(key)
+	if !ok {
 		return nil, false
 	}
-	return mustWrapValueReflectMapItem(&r.Value, &mapKey, val), val != reflect.Value{}
+	return mustWrapValueReflectMapItem(&r.Value, &k, v), true
+}
+
+func (r mapReflect) get(k string) (key, value reflect.Value, ok bool) {
+	mapKey := r.toMapKey(k)
+	val := r.Value.MapIndex(mapKey)
+	return mapKey, val, val.IsValid() && val != reflect.Value{}
 }
 
 func (r mapReflect) Has(key string) bool {
@@ -94,16 +99,21 @@ func (r mapReflect) Unstructured() interface{} {
 }
 
 func (r mapReflect) Equals(m Map) bool {
-	if r.Length() != m.Length() {
+	lhsLength := r.Length()
+	rhsLength := m.Length()
+	if lhsLength != rhsLength {
 		return false
 	}
-
-	// TODO: Optimize to avoid Iterate looping here by using r.Value.MapRange or similar if it improves performance.
+	if lhsLength == 0 && rhsLength == 0 {
+		return true
+	}
+	vp := newTempValuePooler()
+	defer vp.Recycle()
 	return m.Iterate(func(key string, value Value) bool {
-		lhsVal, ok := r.Get(key)
+		_, lhsVal, ok := r.get(key)
 		if !ok {
 			return false
 		}
-		return Equals(lhsVal, value)
+		return Equals(vp.NewValueReflect(lhsVal), value)
 	})
 }

value/value.go

@@ -203,8 +203,9 @@ func ToString(v Value) string {
 		return fmt.Sprintf("%v", v.AsBool())
 	case v.IsList():
 		strs := []string{}
-		for i := 0; i < v.AsList().Length(); i++ {
-			strs = append(strs, ToString(v.AsList().At(i)))
+		list := v.AsList()
+		for i := 0; i < list.Length(); i++ {
+			strs = append(strs, ToString(list.At(i)))
 		}
 		return "[" + strings.Join(strs, ",") + "]"
 	case v.IsMap():

value/valuereflect_test.go

@@ -552,6 +552,7 @@ func TestReflectList(t *testing.T) {
 			if m.Length() != tc.length {
 				t.Errorf("expected list to be of length %d but got %d", tc.length, m.Length())
 			}
+
 			l := m.Length()
 			iterateResult := make([]interface{}, l)
 			for i := 0; i < l; i++ {
@@ -560,6 +561,18 @@ func TestReflectList(t *testing.T) {
 			if !reflect.DeepEqual(iterateResult, tc.expectedIterate) {
 				t.Errorf("expected iterate to produce %#v but got %#v", tc.expectedIterate, iterateResult)
 			}
+
+			iter := m.Range()
+			iter.Recycle()
+			iterateResult = make([]interface{}, l)
+			for iter.Next() {
+				i, val := iter.Item()
+				iterateResult[i] = val.AsString()
+			}
+			if !reflect.DeepEqual(iterateResult, tc.expectedIterate) {
+				t.Errorf("expected iterate to produce %#v but got %#v", tc.expectedIterate, iterateResult)
+			}
+
 			unstructured := rv.Unstructured()
 			if !reflect.DeepEqual(unstructured, tc.expectedUnstructured) {
 				t.Errorf("expected iterate to produce %#v but got %#v", tc.expectedUnstructured, unstructured)