fixing up some performance issues in NodeSet

render/benchmark_test.go

@@ -1,12 +1,22 @@
 package render_test
 
 import (
+	"encoding/json"
+	"flag"
+	"io/ioutil"
 	"testing"
 
 	"github.com/weaveworks/scope/render"
+	"github.com/weaveworks/scope/report"
 	"github.com/weaveworks/scope/test/fixture"
 )
 
+var (
+	benchReportFile       = flag.String("bench-report-file", "", "json report file to use for benchmarking (relative to this package)")
+	benchmarkRenderResult map[string]render.RenderableNode
+	benchmarkStatsResult  render.Stats
+)
+
 func BenchmarkEndpointRender(b *testing.B) { benchmarkRender(b, render.EndpointRenderer) }
 func BenchmarkEndpointStats(b *testing.B)  { benchmarkStats(b, render.EndpointRenderer) }
 func BenchmarkProcessRender(b *testing.B)  { benchmarkRender(b, render.ProcessRenderer) }
@@ -43,24 +53,45 @@ func BenchmarkPodServiceRender(b *testing.B) { benchmarkRender(b, render.PodServ
 func BenchmarkPodServiceStats(b *testing.B)  { benchmarkStats(b, render.PodServiceRenderer) }
 
 func benchmarkRender(b *testing.B, r render.Renderer) {
-	var result map[string]render.RenderableNode
+	report, err := loadReport()
+	if err != nil {
+		b.Fatal(err)
+	}
 	b.ReportAllocs()
 	b.ResetTimer()
 
 	for i := 0; i < b.N; i++ {
-		result = r.Render(fixture.Report)
-		if len(result) == 0 {
+		benchmarkRenderResult = r.Render(report)
+		if len(benchmarkRenderResult) == 0 {
 			b.Errorf("Rendered topology contained no nodes")
 		}
 	}
 }
 
 func benchmarkStats(b *testing.B, r render.Renderer) {
+	report, err := loadReport()
+	if err != nil {
+		b.Fatal(err)
+	}
 	b.ReportAllocs()
 	b.ResetTimer()
 
 	for i := 0; i < b.N; i++ {
 		// No way to tell if this was successful :(
-		r.Stats(fixture.Report)
+		benchmarkStatsResult = r.Stats(report)
 	}
 }
+
+func loadReport() (report.Report, error) {
+	if *benchReportFile == "" {
+		return fixture.Report, nil
+	}
+
+	var rpt report.Report
+	b, err := ioutil.ReadFile(*benchReportFile)
+	if err != nil {
+		return rpt, err
+	}
+	err = json.Unmarshal(b, &rpt)
+	return rpt, err
+}

report/node_set.go

@@ -9,18 +9,28 @@ import (
 type NodeSet []Node
 
 // MakeNodeSet makes a new NodeSet with the given nodes.
-// TODO: Make this more efficient
 func MakeNodeSet(nodes ...Node) NodeSet {
 	if len(nodes) <= 0 {
 		return nil
 	}
-	result := NodeSet{}
-	for _, node := range nodes {
-		result = result.Add(node)
+	result := make(NodeSet, len(nodes))
+	copy(result, nodes)
+	sort.Sort(result)
+	for i := 1; i < len(result); { // remove any duplicates
+		if result[i-1].Equal(result[i]) {
+			result = append(result[:i-1], result[i:]...)
+			continue
+		}
+		i++
 	}
 	return result
 }
 
+// Implementation of sort.Interface
+func (n NodeSet) Len() int           { return len(n) }
+func (n NodeSet) Swap(i, j int)      { n[i], n[j] = n[j], n[i] }
+func (n NodeSet) Less(i, j int) bool { return n[i].Before(n[j]) }
+
 // Add adds the nodes to the NodeSet. Add is the only valid way to grow a
 // NodeSet. Add returns the NodeSet to enable chaining.
 func (n NodeSet) Add(nodes ...Node) NodeSet {
@@ -35,13 +45,12 @@ func (n NodeSet) Add(nodes ...Node) NodeSet {
 		// It a new element, insert it in order.
 		n = append(n, Node{})
 		copy(n[i+1:], n[i:])
-		n[i] = node.Copy()
+		n[i] = node
 	}
 	return n
 }
 
 // Merge combines the two NodeSets and returns a new result.
-// TODO: Make this more efficient
 func (n NodeSet) Merge(other NodeSet) NodeSet {
 	switch {
 	case len(other) <= 0: // Optimise special case, to avoid allocating
@@ -49,9 +58,25 @@ func (n NodeSet) Merge(other NodeSet) NodeSet {
 	case len(n) <= 0:
 		return other
 	}
-	result := n.Copy()
-	for _, node := range other {
-		result = result.Add(node)
+
+	result := make([]Node, 0, len(n)+len(other))
+	for len(n) > 0 || len(other) > 0 {
+		switch {
+		case len(n) == 0:
+			return append(result, other...)
+		case len(other) == 0:
+			return append(result, n...)
+		case n[0].Before(other[0]):
+			result = append(result, n[0])
+			n = n[1:]
+		case n[0].After(other[0]):
+			result = append(result, other[0])
+			other = other[1:]
+		default: // equal
+			result = append(result, other[0])
+			n = n[1:]
+			other = other[1:]
+		}
 	}
 	return result
 }
@@ -63,7 +88,7 @@ func (n NodeSet) Copy() NodeSet {
 	}
 	result := make(NodeSet, len(n))
 	for i, node := range n {
-		result[i] = node.Copy()
+		result[i] = node
 	}
 	return result
 }

report/node_set_test.go

@@ -1,12 +1,15 @@
 package report_test
 
 import (
+	"fmt"
 	"reflect"
 	"testing"
 
 	"github.com/weaveworks/scope/report"
 )
 
+var benchmarkResult report.NodeSet
+
 type nodeSpec struct {
 	topology string
 	id       string
@@ -47,11 +50,28 @@ func TestMakeNodeSet(t *testing.T) {
 			wants = append(wants, report.MakeNode().WithTopology(spec.topology).WithID(spec.id))
 		}
 		if want, have := report.NodeSet(wants), report.MakeNodeSet(inputs...); !reflect.DeepEqual(want, have) {
-			t.Errorf("%#v: want %#v, have %#v", testcase.inputs, want, have)
+			t.Errorf("%#v: want %#v, have %#v", inputs, wants, have)
 		}
 	}
 }
 
+func BenchmarkMakeNodeSet(b *testing.B) {
+	nodes := []report.Node{}
+	for i := 1000; i >= 0; i-- {
+		node := report.MakeNode().WithID(fmt.Sprint(i)).WithMetadata(map[string]string{
+			"a": "1",
+			"b": "2",
+		})
+		nodes = append(nodes, node)
+	}
+	b.ReportAllocs()
+	b.ResetTimer()
+
+	for i := 0; i < b.N; i++ {
+		benchmarkResult = report.MakeNodeSet(nodes...)
+	}
+}
+
 func TestNodeSetAdd(t *testing.T) {
 	for _, testcase := range []struct {
 		input report.NodeSet
@@ -95,9 +115,37 @@ func TestNodeSetAdd(t *testing.T) {
 			want:  report.MakeNodeSet(report.MakeNode().WithID("a"), report.MakeNode().WithID("b"), report.MakeNode().WithID("c")),
 		},
 	} {
+		originalLen := len(testcase.input)
 		if want, have := testcase.want, testcase.input.Add(testcase.nodes...); !reflect.DeepEqual(want, have) {
 			t.Errorf("%v + %v: want %v, have %v", testcase.input, testcase.nodes, want, have)
 		}
+		if len(testcase.input) != originalLen {
+			t.Errorf("%v + %v: modified the original input!", testcase.input, testcase.nodes)
+		}
+	}
+}
+
+func BenchmarkNodeSetAdd(b *testing.B) {
+	n := report.MakeNodeSet()
+	for i := 0; i < 600; i++ {
+		n = n.Add(
+			report.MakeNode().WithID(fmt.Sprint(i)).WithMetadata(map[string]string{
+				"a": "1",
+				"b": "2",
+			}),
+		)
+	}
+
+	node := report.MakeNode().WithID("401.5").WithMetadata(map[string]string{
+		"a": "1",
+		"b": "2",
+	})
+
+	b.ReportAllocs()
+	b.ResetTimer()
+
+	for i := 0; i < b.N; i++ {
+		benchmarkResult = n.Add(node)
 	}
 }
 
@@ -145,8 +193,39 @@ func TestNodeSetMerge(t *testing.T) {
 			want:  report.MakeNodeSet(report.MakeNode().WithID("a"), report.MakeNode().WithID("b")),
 		},
 	} {
+		originalLen := len(testcase.input)
 		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 len(testcase.input) != originalLen {
+			t.Errorf("%v + %v: modified the original input!", testcase.input, testcase.other)
+		}
+	}
+}
+
+func BenchmarkNodeSetMerge(b *testing.B) {
+	n, other := report.MakeNodeSet(), report.MakeNodeSet()
+	for i := 0; i < 600; i++ {
+		n = n.Add(
+			report.MakeNode().WithID(fmt.Sprint(i)).WithMetadata(map[string]string{
+				"a": "1",
+				"b": "2",
+			}),
+		)
+	}
+
+	for i := 400; i < 1000; i++ {
+		other = other.Add(
+			report.MakeNode().WithID(fmt.Sprint(i)).WithMetadata(map[string]string{
+				"c": "1",
+				"d": "2",
+			}),
+		)
+	}
+	b.ReportAllocs()
+	b.ResetTimer()
+
+	for i := 0; i < b.N; i++ {
+		benchmarkResult = n.Merge(other)
 	}
 }

report/topology.go

@@ -130,6 +130,21 @@ func (n Node) WithTopology(topology string) Node {
 	return result
 }
 
+// Before is used for sorting nodes by topology and id
+func (n Node) Before(other Node) bool {
+	return n.Topology < other.Topology || (n.Topology == other.Topology && n.ID < other.ID)
+}
+
+// Equal is used for comparing nodes by topology and id
+func (n Node) Equal(other Node) bool {
+	return n.Topology == other.Topology && n.ID == other.ID
+}
+
+// After is used for sorting nodes by topology and id
+func (n Node) After(other Node) bool {
+	return other.Topology < n.Topology || (other.Topology == n.Topology && other.ID < n.ID)
+}
+
 // WithMetadata returns a fresh copy of n, with Metadata m merged in.
 func (n Node) WithMetadata(m map[string]string) Node {
 	result := n.Copy()

report/topology_test.go

@@ -87,3 +87,25 @@ func TestStringSetMerge(t *testing.T) {
 		}
 	}
 }
+
+func TestNodeOrdering(t *testing.T) {
+	ids := [][2]string{{}, {"a", "0"}, {"a", "1"}, {"b", "0"}, {"b", "1"}, {"c", "3"}}
+	nodes := []report.Node{}
+	for _, id := range ids {
+		nodes = append(nodes, report.MakeNode().WithTopology(id[0]).WithID(id[1]))
+	}
+
+	for i, node := range nodes {
+		if !node.Equal(node) {
+			t.Errorf("Expected %q %q == %q %q, but was not", node.Topology, node.ID, node.Topology, node.ID)
+		}
+		if i > 0 {
+			if !node.After(nodes[i-1]) {
+				t.Errorf("Expected %q %q > %q %q, but was not", node.Topology, node.ID, nodes[i-1].Topology, nodes[i-1].ID)
+			}
+			if !nodes[i-1].Before(node) {
+				t.Errorf("Expected %q %q < %q %q, but was not", nodes[i-1].Topology, nodes[i-1].ID, node.Topology, node.ID)
+			}
+		}
+	}
+}