Skip to content

Commit 5382ed4

Browse files
authored
Merge pull request #151 from jpbetz/avoid-sync-pool
Optimize sync.Pool usage and value.Equals implementations
2 parents 0fb62c1 + 84dea53 commit 5382ed4

13 files changed

+297
-110
lines changed

fieldpath/fromvalue.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,13 @@ func (w *objectWalker) walk() {
5555
case w.value.IsList():
5656
// If the list were atomic, we'd break here, but we don't have
5757
// a schema, so we can't tell.
58-
list := w.value.AsList()
59-
for i := 0; i < list.Length(); i++ {
58+
iter := w.value.AsList().Range()
59+
defer iter.Recycle()
60+
for iter.Next() {
61+
i, value := iter.Item()
6062
w2 := *w
61-
w2.path = append(w.path, GuessBestListPathElement(i, list.At(i)))
62-
w2.value = list.At(i)
63+
w2.path = append(w.path, GuessBestListPathElement(i, value))
64+
w2.value = value
6365
w2.walk()
6466
}
6567
return

typed/remove.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,10 @@ func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) {
5050
}
5151

5252
var newItems []interface{}
53-
for i := 0; i < l.Length(); i++ {
54-
item := l.At(i)
53+
iter := l.Range()
54+
defer iter.Recycle()
55+
for iter.Next() {
56+
i, item := iter.Item()
5557
// Ignore error because we have already validated this list
5658
pe, _ := listItemToPathElement(t, i, item)
5759
path, _ := fieldpath.MakePath(pe)

value/list.go

Lines changed: 50 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -23,24 +23,55 @@ type List interface {
2323
// At returns the item at the given position in the map. It will
2424
// panic if the index is out of range.
2525
At(int) Value
26+
// Range returns a ListRange for iterating over the items in the list.
27+
Range() ListRange
2628
}
2729

30+
// ListRange represents a single iteration across the items of a list.
31+
type ListRange interface {
32+
// 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.
33+
Next() bool
34+
// Item returns the index and value of the current item in the range. or panics if there is no current item.
35+
// For efficiency, Item may reuse the values returned by previous Item calls. Callers should be careful avoid holding
36+
// pointers to the value returned by Item() that escape the iteration loop since they become invalid once either
37+
// Item() or Recycle() is called.
38+
Item() (index int, value Value)
39+
// Recycle returns a ListRange that is no longer needed. The value returned by Item() becomes invalid once this is
40+
// called.
41+
Recycle()
42+
}
43+
44+
var EmptyRange = &emptyRange{}
45+
46+
type emptyRange struct{}
47+
48+
func (_ *emptyRange) Next() bool {
49+
return false
50+
}
51+
52+
func (_ *emptyRange) Item() (index int, value Value) {
53+
panic("Item called on empty ListRange")
54+
}
55+
56+
func (_ *emptyRange) Recycle() {}
57+
2858
// ListEquals compares two lists lexically.
2959
func ListEquals(lhs, rhs List) bool {
3060
if lhs.Length() != rhs.Length() {
3161
return false
3262
}
3363

34-
for i := 0; i < lhs.Length(); i++ {
35-
lv := lhs.At(i)
36-
rv := rhs.At(i)
64+
lhsRange := lhs.Range()
65+
defer lhsRange.Recycle()
66+
rhsRange := rhs.Range()
67+
defer rhsRange.Recycle()
68+
69+
for lhsRange.Next() && rhsRange.Next() {
70+
_, lv := lhsRange.Item()
71+
_, rv := rhsRange.Item()
3772
if !Equals(lv, rv) {
38-
lv.Recycle()
39-
rv.Recycle()
4073
return false
4174
}
42-
lv.Recycle()
43-
rv.Recycle()
4475
}
4576
return true
4677
}
@@ -53,28 +84,31 @@ func ListLess(lhs, rhs List) bool {
5384
// ListCompare compares two lists lexically. The result will be 0 if l==rhs, -1
5485
// if l < rhs, and +1 if l > rhs.
5586
func ListCompare(lhs, rhs List) int {
56-
i := 0
87+
lhsRange := lhs.Range()
88+
defer lhsRange.Recycle()
89+
rhsRange := rhs.Range()
90+
defer rhsRange.Recycle()
91+
5792
for {
58-
if i >= lhs.Length() && i >= rhs.Length() {
93+
lhsOk := lhsRange.Next()
94+
rhsOk := rhsRange.Next()
95+
if !lhsOk && !rhsOk {
5996
// Lists are the same length and all items are equal.
6097
return 0
6198
}
62-
if i >= lhs.Length() {
99+
if !lhsOk {
63100
// LHS is shorter.
64101
return -1
65102
}
66-
if i >= rhs.Length() {
103+
if !rhsOk {
67104
// RHS is shorter.
68105
return 1
69106
}
70-
lv := lhs.At(i)
71-
rv := rhs.At(i)
107+
_, lv := lhsRange.Item()
108+
_, rv := rhsRange.Item()
72109
if c := Compare(lv, rv); c != 0 {
73110
return c
74111
}
75-
lv.Recycle()
76-
rv.Recycle()
77112
// The items are equal; continue.
78-
i++
79113
}
80114
}

value/listreflect.go

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,10 @@ limitations under the License.
1616

1717
package value
1818

19-
import "reflect"
19+
import (
20+
"reflect"
21+
"sync"
22+
)
2023

2124
type listReflect struct {
2225
Value reflect.Value
@@ -40,3 +43,45 @@ func (r listReflect) Unstructured() interface{} {
4043
}
4144
return result
4245
}
46+
47+
var lrrPool = sync.Pool{
48+
New: func() interface{} {
49+
return &listReflectRange{vr: &valueReflect{}}
50+
},
51+
}
52+
53+
func (r listReflect) Range() ListRange {
54+
length := r.Value.Len()
55+
if length == 0 {
56+
return EmptyRange
57+
}
58+
rr := lrrPool.Get().(*listReflectRange)
59+
rr.list = r.Value
60+
rr.i = -1
61+
return rr
62+
}
63+
64+
type listReflectRange struct {
65+
list reflect.Value
66+
vr *valueReflect
67+
i int
68+
}
69+
70+
func (r *listReflectRange) Next() bool {
71+
r.i += 1
72+
return r.i < r.list.Len()
73+
}
74+
75+
func (r *listReflectRange) Item() (index int, value Value) {
76+
if r.i < 0 {
77+
panic("Item() called before first calling Next()")
78+
}
79+
if r.i >= r.list.Len() {
80+
panic("Item() called on ListRange with no more items")
81+
}
82+
return r.i, r.vr.reuse(r.list.Index(r.i))
83+
}
84+
85+
func (r *listReflectRange) Recycle() {
86+
lrrPool.Put(r)
87+
}

value/listunstructured.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ limitations under the License.
1616

1717
package value
1818

19+
import "sync"
20+
1921
type listUnstructured []interface{}
2022

2123
func (l listUnstructured) Length() int {
@@ -25,3 +27,44 @@ func (l listUnstructured) Length() int {
2527
func (l listUnstructured) At(i int) Value {
2628
return NewValueInterface(l[i])
2729
}
30+
31+
var lurPool = sync.Pool{
32+
New: func() interface{} {
33+
return &listUnstructuredRange{vv: &valueUnstructured{}}
34+
},
35+
}
36+
37+
func (l listUnstructured) Range() ListRange {
38+
if len(l) == 0 {
39+
return EmptyRange
40+
}
41+
r := lurPool.Get().(*listUnstructuredRange)
42+
r.list = l
43+
r.i = -1
44+
return r
45+
}
46+
47+
type listUnstructuredRange struct {
48+
list listUnstructured
49+
vv *valueUnstructured
50+
i int
51+
}
52+
53+
func (r *listUnstructuredRange) Next() bool {
54+
r.i += 1
55+
return r.i < len(r.list)
56+
}
57+
58+
func (r *listUnstructuredRange) Item() (index int, value Value) {
59+
if r.i < 0 {
60+
panic("Item() called before first calling Next()")
61+
}
62+
if r.i >= len(r.list) {
63+
panic("Item() called on ListRange with no more items")
64+
}
65+
return r.i, r.vv.reuse(r.list[r.i])
66+
}
67+
68+
func (r *listUnstructuredRange) Recycle() {
69+
lurPool.Put(r)
70+
}

value/map.go

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -81,11 +81,16 @@ func MapCompare(lhs, rhs Map) int {
8181
}
8282
litem, _ := lhs.Get(lorder[i])
8383
ritem, _ := rhs.Get(rorder[i])
84-
if c := Compare(litem, ritem); c != 0 {
84+
c := Compare(litem, ritem)
85+
if litem != nil {
86+
litem.Recycle()
87+
}
88+
if ritem != nil {
89+
ritem.Recycle()
90+
}
91+
if c != 0 {
8592
return c
8693
}
87-
litem.Recycle()
88-
ritem.Recycle()
8994
// The items are equal; continue.
9095
i++
9196
}
@@ -94,6 +99,7 @@ func MapCompare(lhs, rhs Map) int {
9499
// MapEquals returns true if lhs == rhs, false otherwise. This function
95100
// acts on generic types and should not be used by callers, but can help
96101
// implement Map.Equals.
102+
// WARN: This is a naive implementation, calling lhs.Equals(rhs) is typically far more efficient.
97103
func MapEquals(lhs, rhs Map) bool {
98104
if lhs.Length() != rhs.Length() {
99105
return false
@@ -103,11 +109,8 @@ func MapEquals(lhs, rhs Map) bool {
103109
if !ok {
104110
return false
105111
}
106-
if !Equals(v, vo) {
107-
vo.Recycle()
108-
return false
109-
}
112+
equal := Equals(v, vo)
110113
vo.Recycle()
111-
return true
114+
return equal
112115
})
113116
}

value/mapreflect.go

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@ limitations under the License.
1616

1717
package value
1818

19-
import "reflect"
19+
import (
20+
"reflect"
21+
)
2022

2123
type mapReflect struct {
2224
valueReflect
@@ -28,12 +30,17 @@ func (r mapReflect) Length() int {
2830
}
2931

3032
func (r mapReflect) Get(key string) (Value, bool) {
31-
mapKey := r.toMapKey(key)
32-
val := r.Value.MapIndex(mapKey)
33-
if !val.IsValid() {
33+
k, v, ok := r.get(key)
34+
if !ok {
3435
return nil, false
3536
}
36-
return mustWrapValueReflectMapItem(&r.Value, &mapKey, val), val != reflect.Value{}
37+
return mustWrapValueReflectMapItem(&r.Value, &k, v), true
38+
}
39+
40+
func (r mapReflect) get(k string) (key, value reflect.Value, ok bool) {
41+
mapKey := r.toMapKey(k)
42+
val := r.Value.MapIndex(mapKey)
43+
return mapKey, val, val.IsValid() && val != reflect.Value{}
3744
}
3845

3946
func (r mapReflect) Has(key string) bool {
@@ -61,10 +68,10 @@ func (r mapReflect) toMapKey(key string) reflect.Value {
6168
}
6269

6370
func (r mapReflect) Iterate(fn func(string, Value) bool) bool {
71+
vr := reflectPool.Get().(*valueReflect)
72+
defer vr.Recycle()
6473
return eachMapEntry(r.Value, func(s string, value reflect.Value) bool {
65-
mapVal := mustWrapValueReflect(value)
66-
defer mapVal.Recycle()
67-
return fn(s, mapVal)
74+
return fn(s, vr.reuse(value))
6875
})
6976
}
7077

@@ -92,16 +99,21 @@ func (r mapReflect) Unstructured() interface{} {
9299
}
93100

94101
func (r mapReflect) Equals(m Map) bool {
95-
if r.Length() != m.Length() {
102+
lhsLength := r.Length()
103+
rhsLength := m.Length()
104+
if lhsLength != rhsLength {
96105
return false
97106
}
98-
99-
// TODO: Optimize to avoid Iterate looping here by using r.Value.MapRange or similar if it improves performance.
107+
if lhsLength == 0 {
108+
return true
109+
}
110+
vr := reflectPool.Get().(*valueReflect)
111+
defer vr.Recycle()
100112
return m.Iterate(func(key string, value Value) bool {
101-
lhsVal, ok := r.Get(key)
113+
_, lhsVal, ok := r.get(key)
102114
if !ok {
103115
return false
104116
}
105-
return Equals(lhsVal, value)
117+
return Equals(vr.reuse(lhsVal), value)
106118
})
107119
}

0 commit comments

Comments
 (0)