From 3ed5a51e80a670955450e849fcd5dceb4a8f4060 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 27 Jun 2018 08:13:05 +0000 Subject: [PATCH] Don't copy StringSet if merging a subset Make StringSet.Merge() work like StringLatestMap.Merge() --- report/dns.go | 8 ++++--- report/id_list.go | 3 ++- report/report.go | 9 ++++--- report/sets.go | 12 ++++++++-- report/string_set.go | 52 +++++++++++++++++++++++++++++------------ report/topology_test.go | 4 ++-- 6 files changed, 60 insertions(+), 28 deletions(-) diff --git a/report/dns.go b/report/dns.go index a09ed0b449..d50667c643 100644 --- a/report/dns.go +++ b/report/dns.go @@ -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 } diff --git a/report/id_list.go b/report/id_list.go index 259b1d3b7c..e74901e7a4 100644 --- a/report/id_list.go +++ b/report/id_list.go @@ -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. diff --git a/report/report.go b/report/report.go index d21551c504..5c065d1867 100644 --- a/report/report.go +++ b/report/report.go @@ -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} } diff --git a/report/sets.go b/report/sets.go index 2775dbeaa0..a057275860 100644 --- a/report/sets.go +++ b/report/sets.go @@ -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), @@ -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) }) diff --git a/report/string_set.go b/report/string_set.go index c20014e314..7580d140ca 100644 --- a/report/string_set.go +++ b/report/string_set.go @@ -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 } diff --git a/report/topology_test.go b/report/topology_test.go index 08ca1a2b15..f50b2b4608 100644 --- a/report/topology_test.go +++ b/report/topology_test.go @@ -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) } } }