Skip to content

Commit

Permalink
Don't copy StringSet if merging a subset
Browse files Browse the repository at this point in the history
Make StringSet.Merge() work like StringLatestMap.Merge()
  • Loading branch information
bboreham authored and satyamz committed Jul 26, 2018
1 parent cea6631 commit 3ed5a51
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 28 deletions.
8 changes: 5 additions & 3 deletions report/dns.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@ func (r DNSRecords) Merge(other DNSRecords) DNSRecords {
cp := r.Copy()
for k, v := range other {
if v2, ok := cp[k]; ok {
cp[k] = DNSRecord{
Forward: v.Forward.Merge(v2.Forward),
Reverse: v.Reverse.Merge(v2.Reverse),
fMerged, fUnchanged := v.Forward.Merge(v2.Forward)
rMerged, rUnchanged := v.Reverse.Merge(v2.Reverse)
if fUnchanged && rUnchanged {
continue
}
cp[k] = DNSRecord{Forward: fMerged, Reverse: rMerged}
} else {
cp[k] = v
}
Expand Down
3 changes: 2 additions & 1 deletion report/id_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ func (a IDList) Add(ids ...string) IDList {

// Merge all elements from a and b into a new list
func (a IDList) Merge(b IDList) IDList {
return IDList(StringSet(a).Merge(StringSet(b)))
merged, _ := StringSet(a).Merge(StringSet(b))
return IDList(merged)
}

// Contains returns true if id is in the list.
Expand Down
9 changes: 4 additions & 5 deletions report/report.go
Original file line number Diff line number Diff line change
Expand Up @@ -531,13 +531,12 @@ func (r Report) upgradeDNSRecords() Report {
if ok && (foundS || foundR) {
// Add address and names to report-level map
if existing, found := dns[addr]; found {
// Optimise the expected case that they are equal
if existing.Forward.Equal(snoopedNames) && existing.Reverse.Equal(reverseNames) {
var sUnchanged, rUnchanged bool
snoopedNames, sUnchanged = snoopedNames.Merge(existing.Forward)
reverseNames, rUnchanged = reverseNames.Merge(existing.Reverse)
if sUnchanged && rUnchanged {
continue
}
// Not equal - merge this node's data into existing data,
snoopedNames = snoopedNames.Merge(existing.Forward)
reverseNames = reverseNames.Merge(existing.Reverse)
}
dns[addr] = DNSRecord{Forward: snoopedNames, Reverse: reverseNames}
}
Expand Down
12 changes: 10 additions & 2 deletions report/sets.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ func (s Sets) Add(key string, value StringSet) Sets {
s = emptySets
}
if existingValue, ok := s.psMap.Lookup(key); ok {
value = value.Merge(existingValue.(StringSet))
var unchanged bool
value, unchanged = existingValue.(StringSet).Merge(value)
if unchanged {
return s
}
}
return Sets{
psMap: s.psMap.Set(key, value),
Expand Down Expand Up @@ -94,7 +98,11 @@ func (s Sets) Merge(other Sets) Sets {
iter.ForEach(func(key string, value interface{}) {
set := value.(StringSet)
if existingSet, ok := result.Lookup(key); ok {
set = set.Merge(existingSet.(StringSet))
var unchanged bool
set, unchanged = existingSet.(StringSet).Merge(set)
if unchanged {
return
}
}
result = result.Set(key, set)
})
Expand Down
52 changes: 37 additions & 15 deletions report/string_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,32 +81,54 @@ func (s StringSet) Add(strs ...string) StringSet {
}

// Merge combines the two StringSets and returns a new result.
func (s StringSet) Merge(other StringSet) StringSet {
// Second return value is true if the return value is s
func (s StringSet) Merge(other StringSet) (StringSet, bool) {
switch {
case len(other) <= 0: // Optimise special case, to avoid allocating
return s // (note unit test DeepEquals breaks if we don't do this)
return s, true // (note unit test DeepEquals breaks if we don't do this)
case len(s) <= 0:
return other
return other, false
}
result := make(StringSet, len(s)+len(other))
for i, j, k := 0, 0, 0; ; k++ {

i, j := 0, 0
loop:
for i < len(s) {
switch {
case i >= len(s):
copy(result[k:], other[j:])
return result[:k+len(other)-j]
case j >= len(other):
copy(result[k:], s[i:])
return result[:k+len(s)-i]
return s, true
case s[i] == other[j]:
i++
j++
case s[i] < other[j]:
result[k] = s[i]
i++
case s[i] > other[j]:
result[k] = other[j]
default:
break loop
}
}
if i >= len(s) && j >= len(other) {
return s, true
}

result := make(StringSet, i, len(s)+len(other))
copy(result, s[:i])

for i < len(s) {
switch {
case j >= len(other):
result = append(result, s[i:]...)
return result, false
case s[i] == other[j]:
result = append(result, s[i])
i++
j++
default: // equal
result[k] = s[i]
case s[i] < other[j]:
result = append(result, s[i])
i++
default:
result = append(result, other[j])
j++
}
}
result = append(result, other[j:]...)
return result, false
}
4 changes: 2 additions & 2 deletions report/topology_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ func TestStringSetMerge(t *testing.T) {
{input: report.MakeStringSet("a", "c"), other: report.MakeStringSet("a", "b"), want: report.MakeStringSet("a", "b", "c")},
{input: report.MakeStringSet("b"), other: report.MakeStringSet("a"), want: report.MakeStringSet("a", "b")},
} {
if want, have := testcase.want, testcase.input.Merge(testcase.other); !reflect.DeepEqual(want, have) {
t.Errorf("%v + %v: want %v, have %v", testcase.input, testcase.other, want, have)
if have, _ := testcase.input.Merge(testcase.other); !reflect.DeepEqual(testcase.want, have) {
t.Errorf("%v + %v: want %v, have %v", testcase.input, testcase.other, testcase.want, have)
}
}
}
Expand Down

0 comments on commit 3ed5a51

Please sign in to comment.