Skip to content

Optimize benchmarks; permit serialization #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 19, 2019
Merged

Optimize benchmarks; permit serialization #88

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f33c13e
don't use maps for PathElementSet; saves a couple hundred allocations…
lavalamp Jul 16, 2019
dd13317
In PathElement, replace []Field with *Map for Key
lavalamp Jul 18, 2019
8b1ef26
Benchmarks shouldn't include test code
lavalamp Jul 17, 2019
03f010c
Add a micro benchmark for fieldpath.
lavalamp Jul 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 128 additions & 33 deletions fieldpath/element.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,54 @@ type PathElement struct {
Index *int
}

// Less provides an order for path elements.
func (e PathElement) Less(rhs PathElement) bool {
if e.FieldName != nil {
if rhs.FieldName == nil {
return true
}
return *e.FieldName < *rhs.FieldName
} else if rhs.FieldName != nil {
return false
}

if len(e.Key) != 0 {
if len(rhs.Key) == 0 {
return true
}
return value.FieldCollectionLess(e.Key, rhs.Key)
} else if len(rhs.Key) != 0 {
return false
}

if e.Value != nil {
if rhs.Value == nil {
return true
}
return e.Value.Less(*rhs.Value)
} else if rhs.Value != nil {
return false
}

if e.Index != nil {
if rhs.Index == nil {
return true
}
return *e.Index < *rhs.Index
} else if rhs.Index != nil {
// Yes, I know the next statement is the same. But this way
// the obvious way of extending the function wil be bug-free.
return false
}

return false
}

// Equals returns true if both path elements are equal.
func (e PathElement) Equals(rhs PathElement) bool {
return !e.Less(rhs) && !rhs.Less(e)
}

// String presents the path element as a human-readable string.
func (e PathElement) String() string {
switch {
Expand Down Expand Up @@ -92,62 +140,104 @@ func KeyByFields(nameValues ...interface{}) []value.Field {
// PathElementSet is a set of path elements.
// TODO: serialize as a list.
type PathElementSet struct {
// The strange construction is because there's no way to test
// PathElements for equality (it can't be used as a key for a map).
members map[string]PathElement
members sortedPathElements
}

type sortedPathElements []PathElement

// Implement the sort interface; this would permit bulk creation, which would
// be faster than doing it one at a time via Insert.
func (spe sortedPathElements) Len() int { return len(spe) }
func (spe sortedPathElements) Less(i, j int) bool { return spe[i].Less(spe[j]) }
func (spe sortedPathElements) Swap(i, j int) { spe[i], spe[j] = spe[j], spe[i] }

// Insert adds pe to the set.
func (s *PathElementSet) Insert(pe PathElement) {
serialized := pe.String()
if s.members == nil {
s.members = map[string]PathElement{
serialized: pe,
}
loc := sort.Search(len(s.members), func(i int) bool {
return !s.members[i].Less(pe)
})
if loc == len(s.members) {
s.members = append(s.members, pe)
return
}
if _, ok := s.members[serialized]; !ok {
s.members[serialized] = pe
if s.members[loc].Equals(pe) {
return
}
n := len(s.members) - 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly similar but maybe better: https://github.com/golang/go/wiki/SliceTricks#insert

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I meant to switch this to the thing they have listed as the "copy" variant; the other variant does an unnecessary allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#89

s.members = append(s.members, s.members[n])
for n > loc {
s.members[n] = s.members[n-1]
n--
}
s.members[loc] = pe
}

// Union returns a set containing elements that appear in either s or s2.
func (s *PathElementSet) Union(s2 *PathElementSet) *PathElementSet {
out := &PathElementSet{
members: map[string]PathElement{},
out := &PathElementSet{}

i, j := 0, 0
for i < len(s.members) && j < len(s2.members) {
if s.members[i].Less(s2.members[j]) {
out.members = append(out.members, s.members[i])
i++
} else {
out.members = append(out.members, s2.members[j])
if !s2.members[j].Less(s.members[i]) {
i++
}
j++
}
}
for k, v := range s.members {
out.members[k] = v

if i < len(s.members) {
out.members = append(out.members, s.members[i:]...)
}
for k, v := range s2.members {
out.members[k] = v
if j < len(s2.members) {
out.members = append(out.members, s2.members[j:]...)
}
return out
}

// Intersection returns a set containing elements which appear in both s and s2.
func (s *PathElementSet) Intersection(s2 *PathElementSet) *PathElementSet {
out := &PathElementSet{
members: map[string]PathElement{},
}
for k, v := range s.members {
if _, ok := s2.members[k]; ok {
out.members[k] = v
out := &PathElementSet{}

i, j := 0, 0
for i < len(s.members) && j < len(s2.members) {
if s.members[i].Less(s2.members[j]) {
i++
} else {
if !s2.members[j].Less(s.members[i]) {
out.members = append(out.members, s.members[i])
i++
}
j++
}
}

return out
}

// Difference returns a set containing elements which appear in s but not in s2.
func (s *PathElementSet) Difference(s2 *PathElementSet) *PathElementSet {
out := &PathElementSet{
members: map[string]PathElement{},
}
for k, v := range s.members {
if _, ok := s2.members[k]; !ok {
out.members[k] = v
out := &PathElementSet{}

i, j := 0, 0
for i < len(s.members) && j < len(s2.members) {
if s.members[i].Less(s2.members[j]) {
out.members = append(out.members, s.members[i])
i++
} else {
if !s2.members[j].Less(s.members[i]) {
i++
}
j++
}
}
if i < len(s.members) {
out.members = append(out.members, s.members[i:]...)
}
return out
}

Expand All @@ -156,11 +246,16 @@ func (s *PathElementSet) Size() int { return len(s.members) }

// Has returns true if pe is a member of the set.
func (s *PathElementSet) Has(pe PathElement) bool {
if s.members == nil {
loc := sort.Search(len(s.members), func(i int) bool {
return !s.members[i].Less(pe)
})
if loc == len(s.members) {
return false
}
_, ok := s.members[pe.String()]
return ok
if s.members[loc].Equals(pe) {
return true
}
return false
}

// Equals returns true if s and s2 have exactly the same members.
Expand All @@ -169,14 +264,14 @@ func (s *PathElementSet) Equals(s2 *PathElementSet) bool {
return false
}
for k := range s.members {
if _, ok := s2.members[k]; !ok {
if !s.members[k].Equals(s2.members[k]) {
return false
}
}
return true
}

// Iterate calls f for each PathElement in the set.
// Iterate calls f for each PathElement in the set. The order is deterministic.
func (s *PathElementSet) Iterate(f func(PathElement)) {
for _, pe := range s.members {
f(pe)
Expand Down
140 changes: 140 additions & 0 deletions fieldpath/element_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package fieldpath

import (
"testing"

"sigs.k8s.io/structured-merge-diff/value"
)

func TestPathElementSet(t *testing.T) {
Expand All @@ -28,4 +30,142 @@ func TestPathElementSet(t *testing.T) {
if s2.Equals(s) {
t.Errorf("unequal sets should not equal")
}
if !s2.Has(PathElement{}) {
t.Errorf("expected to have something: %#v", s2)
}

n1 := "aoeu"
n2 := "asdf"
s2.Insert(PathElement{FieldName: &n1})
if !s2.Has(PathElement{FieldName: &n1}) {
t.Errorf("expected to have something: %#v", s2)
}
if s2.Has(PathElement{FieldName: &n2}) {
t.Errorf("expected to not have something: %#v", s2)
}

s2.Insert(PathElement{FieldName: &n2})
expected := []*string{&n1, &n2, nil}
i := 0
s2.Iterate(func(pe PathElement) {
e, a := expected[i], pe.FieldName
if e == nil || a == nil {
if e != a {
t.Errorf("index %v wanted %#v, got %#v", i, e, a)
}
} else {
if *e != *a {
t.Errorf("index %v wanted %#v, got %#v", i, *e, *a)
}
}
i++
})
}

func strptr(s string) *string { return &s }
func intptr(i int) *int { return &i }
func valptr(i int) *value.Value { v := value.IntValue(i); return &v }

func TestPathElementLess(t *testing.T) {
table := []struct {
name string
// we expect a < b and !(b < a) unless eq is true, in which
// case we expect less to return false in both orders.
a, b PathElement
eq bool
}{
{
name: "FieldName-0",
a: PathElement{},
b: PathElement{},
eq: true,
}, {
name: "FieldName-1",
a: PathElement{FieldName: strptr("anteater")},
b: PathElement{FieldName: strptr("zebra")},
}, {
name: "FieldName-2",
a: PathElement{FieldName: strptr("bee")},
b: PathElement{FieldName: strptr("bee")},
eq: true,
}, {
name: "FieldName-3",
a: PathElement{FieldName: strptr("capybara")},
b: PathElement{Key: []value.Field{{Name: "dog", Value: value.IntValue(3)}}},
}, {
name: "FieldName-4",
a: PathElement{FieldName: strptr("elephant")},
b: PathElement{Value: valptr(4)},
}, {
name: "FieldName-5",
a: PathElement{FieldName: strptr("falcon")},
b: PathElement{Index: intptr(5)},
}, {
name: "Key-1",
a: PathElement{Key: []value.Field{{Name: "goat", Value: value.IntValue(1)}}},
b: PathElement{Key: []value.Field{{Name: "goat", Value: value.IntValue(1)}}},
eq: true,
}, {
name: "Key-2",
a: PathElement{Key: []value.Field{{Name: "horse", Value: value.IntValue(1)}}},
b: PathElement{Key: []value.Field{{Name: "horse", Value: value.IntValue(2)}}},
}, {
name: "Key-3",
a: PathElement{Key: []value.Field{{Name: "ibex", Value: value.IntValue(1)}}},
b: PathElement{Key: []value.Field{{Name: "jay", Value: value.IntValue(1)}}},
}, {
name: "Key-4",
a: PathElement{Key: []value.Field{{Name: "kite", Value: value.IntValue(1)}}},
b: PathElement{Key: []value.Field{{Name: "kite", Value: value.IntValue(1)}, {Name: "kite-2", Value: value.IntValue(1)}}},
}, {
name: "Key-5",
a: PathElement{Key: []value.Field{{Name: "kite", Value: value.IntValue(1)}}},
b: PathElement{Value: valptr(1)},
}, {
name: "Key-6",
a: PathElement{Key: []value.Field{{Name: "kite", Value: value.IntValue(1)}}},
b: PathElement{Index: intptr(5)},
}, {
name: "Value-1",
a: PathElement{Value: valptr(1)},
b: PathElement{Value: valptr(2)},
}, {
name: "Value-2",
a: PathElement{Value: valptr(1)},
b: PathElement{Value: valptr(1)},
eq: true,
}, {
name: "Value-3",
a: PathElement{Value: valptr(1)},
b: PathElement{Index: intptr(1)},
}, {
name: "Index-1",
a: PathElement{Index: intptr(1)},
b: PathElement{Index: intptr(2)},
}, {
name: "Index-2",
a: PathElement{Index: intptr(1)},
b: PathElement{Index: intptr(1)},
eq: true,
},
}

for i := range table {
i := i
t.Run(table[i].name, func(t *testing.T) {
tt := table[i]
if tt.eq {
if tt.a.Less(tt.b) {
t.Errorf("oops, a < b: %#v, %#v", tt.a, tt.b)
}
} else {
if !tt.a.Less(tt.b) {
t.Errorf("oops, a >= b: %#v, %#v", tt.a, tt.b)
}
}
if tt.b.Less(tt.b) {
t.Errorf("oops, b < a: %#v, %#v", tt.b, tt.a)
}
})
}
}
Loading