From 1091225bf67b1afa1f1d2b770b52a5b65c85cbe3 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Tue, 2 Aug 2016 10:33:29 +0100 Subject: [PATCH 1/3] get rid of Node.Copy() A shallow copy is sufficient. Which we get for free in most cases since Node is passed around by value. --- probe/host/tagger_test.go | 2 +- render/container.go | 16 +++--- render/detailed/connections.go | 8 ++- render/process.go | 7 ++- report/node.go | 99 ++++++++++++---------------------- report/topology.go | 2 +- 6 files changed, 49 insertions(+), 85 deletions(-) diff --git a/probe/host/tagger_test.go b/probe/host/tagger_test.go index 6001815e49..ad4582b1d5 100644 --- a/probe/host/tagger_test.go +++ b/probe/host/tagger_test.go @@ -17,7 +17,7 @@ func TestTagger(t *testing.T) { r := report.MakeReport() r.Process.AddNode(node) rpt, _ := host.NewTagger(hostID).Tag(r) - have := rpt.Process.Nodes[endpointNodeID].Copy() + have := rpt.Process.Nodes[endpointNodeID] // It should now have the host ID wantHostID := report.MakeHostNodeID(hostID) diff --git a/render/container.go b/render/container.go index 6b48a758e0..440d9fbdc8 100644 --- a/render/container.go +++ b/render/container.go @@ -191,13 +191,12 @@ func (r containerWithImageNameRenderer) Render(rpt report.Report, dct Decorator) imageNameWithoutVersion := docker.ImageNameWithoutVersion(imageName) imageNodeID := report.MakeContainerImageNodeID(imageNameWithoutVersion) - output := c.Copy() - output = propagateLatest(docker.ImageName, image, output) - output = propagateLatest(docker.ImageLabelPrefix+"works.weave.role", image, output) - output.Parents = output.Parents. + c = propagateLatest(docker.ImageName, image, c) + c = propagateLatest(docker.ImageLabelPrefix+"works.weave.role", image, c) + c.Parents = c.Parents. Delete(report.ContainerImage). Add(report.ContainerImage, report.MakeStringSet(imageNodeID)) - outputs[id] = output + outputs[id] = c } return outputs } @@ -365,14 +364,13 @@ func MapContainerImage2Name(n report.Node, _ report.Networks) report.Nodes { } imageNameWithoutVersion := docker.ImageNameWithoutVersion(imageName) - output := n.Copy() - output.ID = report.MakeContainerImageNodeID(imageNameWithoutVersion) + n.ID = report.MakeContainerImageNodeID(imageNameWithoutVersion) if imageID, ok := report.ParseContainerImageNodeID(n.ID); ok { - output.Sets = output.Sets.Add(docker.ImageID, report.EmptyStringSet.Add(imageID)) + n.Sets = n.Sets.Add(docker.ImageID, report.EmptyStringSet.Add(imageID)) } - return report.Nodes{output.ID: output} + return report.Nodes{n.ID: n} } // MapContainer2Hostname maps container Nodes to 'hostname' renderabled nodes.. diff --git a/render/detailed/connections.go b/render/detailed/connections.go index fe1ba57deb..576690013a 100644 --- a/render/detailed/connections.go +++ b/render/detailed/connections.go @@ -75,7 +75,6 @@ func incomingConnectionsSummary(topologyID string, r report.Report, n report.Nod if !node.Adjacency.Contains(n.ID) { continue } - remoteNode := node.Copy() // Work out what port they are talking to, and count the number of // connections to that port. @@ -91,7 +90,7 @@ func incomingConnectionsSummary(topologyID string, r report.Report, n report.Nod } key := connection{ localNode: &n, - remoteNode: &remoteNode, + remoteNode: &node, port: port, } if isInternetNode(n) { @@ -127,8 +126,7 @@ func outgoingConnectionsSummary(topologyID string, r report.Report, n report.Nod if !ok { continue } - remoteNode := node.Copy() - remoteEndpointIDs := endpointChildIDsOf(remoteNode) + remoteEndpointIDs := endpointChildIDsOf(node) for _, localEndpoint := range localEndpoints { _, localAddr, _, ok := report.ParseEndpointNodeID(localEndpoint.ID) @@ -143,7 +141,7 @@ func outgoingConnectionsSummary(topologyID string, r report.Report, n report.Nod } key := connection{ localNode: &n, - remoteNode: &remoteNode, + remoteNode: &node, port: port, } if isInternetNode(n) { diff --git a/render/process.go b/render/process.go index 0d624807df..c756fa2c3a 100644 --- a/render/process.go +++ b/render/process.go @@ -63,12 +63,11 @@ func (r processWithContainerNameRenderer) Render(rpt report.Report, dct Decorato if !ok { continue } - output := p.Copy() - output.Latest = output.Latest.Set(docker.ContainerID, timestamp, containerID) + p.Latest = p.Latest.Set(docker.ContainerID, timestamp, containerID) if containerName, timestamp, ok := container.Latest.LookupEntry(docker.ContainerName); ok { - output.Latest = output.Latest.Set(docker.ContainerName, timestamp, containerName) + p.Latest = p.Latest.Set(docker.ContainerName, timestamp, containerName) } - outputs[id] = output + outputs[id] = p } return outputs } diff --git a/report/node.go b/report/node.go index b1143f23f7..4ff2547f01 100644 --- a/report/node.go +++ b/report/node.go @@ -45,16 +45,14 @@ func MakeNodeWith(id string, m map[string]string) Node { // WithID returns a fresh copy of n, with ID changed. func (n Node) WithID(id string) Node { - result := n.Copy() - result.ID = id - return result + n.ID = id + return n } // WithTopology returns a fresh copy of n, with ID changed. func (n Node) WithTopology(topology string) Node { - result := n.Copy() - result.Topology = topology - return result + n.Topology = topology + return n } // Before is used for sorting nodes by topology and id @@ -74,121 +72,92 @@ func (n Node) After(other Node) bool { // WithLatests returns a fresh copy of n, with Metadata m merged in. func (n Node) WithLatests(m map[string]string) Node { - result := n.Copy() ts := mtime.Now() for k, v := range m { - result.Latest = result.Latest.Set(k, ts, v) + n.Latest = n.Latest.Set(k, ts, v) } - return result + return n } // WithLatest produces a new Node with k mapped to v in the Latest metadata. func (n Node) WithLatest(k string, ts time.Time, v string) Node { - result := n.Copy() - result.Latest = result.Latest.Set(k, ts, v) - return result + n.Latest = n.Latest.Set(k, ts, v) + return n } // WithCounters returns a fresh copy of n, with Counters c merged in. func (n Node) WithCounters(c map[string]int) Node { - result := n.Copy() - result.Counters = result.Counters.Merge(Counters{}.fromIntermediate(c)) - return result + n.Counters = n.Counters.Merge(Counters{}.fromIntermediate(c)) + return n } // WithSet returns a fresh copy of n, with set merged in at key. func (n Node) WithSet(key string, set StringSet) Node { - result := n.Copy() - result.Sets = result.Sets.Add(key, set) - return result + n.Sets = n.Sets.Add(key, set) + return n } // WithSets returns a fresh copy of n, with sets merged in. func (n Node) WithSets(sets Sets) Node { - result := n.Copy() - result.Sets = result.Sets.Merge(sets) - return result + n.Sets = n.Sets.Merge(sets) + return n } // WithMetric returns a fresh copy of n, with metric merged in at key. func (n Node) WithMetric(key string, metric Metric) Node { - result := n.Copy() - result.Metrics[key] = n.Metrics[key].Merge(metric) - return result + n.Metrics = n.Metrics.Copy() + n.Metrics[key] = n.Metrics[key].Merge(metric) + return n } // WithMetrics returns a fresh copy of n, with metrics merged in. func (n Node) WithMetrics(metrics Metrics) Node { - result := n.Copy() - result.Metrics = result.Metrics.Merge(metrics) - return result + n.Metrics = n.Metrics.Merge(metrics) + return n } // WithAdjacent returns a fresh copy of n, with 'a' added to Adjacency func (n Node) WithAdjacent(a ...string) Node { - result := n.Copy() - result.Adjacency = result.Adjacency.Add(a...) - return result + n.Adjacency = n.Adjacency.Add(a...) + return n } // WithEdge returns a fresh copy of n, with 'dst' added to Adjacency and md // added to EdgeMetadata. func (n Node) WithEdge(dst string, md EdgeMetadata) Node { - result := n.Copy() - result.Adjacency = result.Adjacency.Add(dst) - result.Edges = result.Edges.Add(dst, md) - return result + n.Adjacency = n.Adjacency.Add(dst) + n.Edges = n.Edges.Add(dst, md) + return n } // WithControls returns a fresh copy of n, with cs added to Controls. func (n Node) WithControls(cs ...string) Node { - result := n.Copy() - result.Controls = result.Controls.Add(cs...) - return result + n.Controls = n.Controls.Add(cs...) + return n } // WithParents returns a fresh copy of n, with sets merged in. func (n Node) WithParents(parents Sets) Node { - result := n.Copy() - result.Parents = result.Parents.Merge(parents) - return result + n.Parents = n.Parents.Merge(parents) + return n } // PruneParents returns a fresh copy of n, without any parents. func (n Node) PruneParents() Node { - result := n.Copy() - result.Parents = EmptySets - return result + n.Parents = EmptySets + return n } // WithChildren returns a fresh copy of n, with children merged in. func (n Node) WithChildren(children NodeSet) Node { - result := n.Copy() - result.Children = result.Children.Merge(children) - return result + n.Children = n.Children.Merge(children) + return n } // WithChild returns a fresh copy of n, with one child merged in. func (n Node) WithChild(child Node) Node { - result := n.Copy() - result.Children = result.Children.Merge(MakeNodeSet(child)) - return result -} - -// Copy returns a value copy of the Node. -func (n Node) Copy() Node { - cp := MakeNode(n.ID) - cp.Topology = n.Topology - cp.Counters = n.Counters.Copy() - cp.Sets = n.Sets.Copy() - cp.Adjacency = n.Adjacency.Copy() - cp.Edges = n.Edges.Copy() - cp.Controls = n.Controls.Copy() - cp.Latest = n.Latest.Copy() - cp.Metrics = n.Metrics.Copy() - cp.Parents = n.Parents.Copy() - cp.Children = n.Children.Copy() - return cp + n.Children = n.Children.Merge(MakeNodeSet(child)) + return n } // Merge mergses the individual components of a node and returns a diff --git a/report/topology.go b/report/topology.go index f64be71223..1beba4f79d 100644 --- a/report/topology.go +++ b/report/topology.go @@ -167,7 +167,7 @@ type Nodes map[string]Node func (n Nodes) Copy() Nodes { cp := make(Nodes, len(n)) for k, v := range n { - cp[k] = v.Copy() + cp[k] = v } return cp } From bdd09d8aa9df40ff36d1ec97b45725dff182971d Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Tue, 2 Aug 2016 11:08:24 +0000 Subject: [PATCH 2/3] Restore copying nodes when obtaining connection summaries --- render/detailed/connections.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/render/detailed/connections.go b/render/detailed/connections.go index 576690013a..0a4decfaee 100644 --- a/render/detailed/connections.go +++ b/render/detailed/connections.go @@ -75,6 +75,9 @@ func incomingConnectionsSummary(topologyID string, r report.Report, n report.Nod if !node.Adjacency.Contains(n.ID) { continue } + // Shallow-copy the node to obtain a different pointer + // on the remote side + remoteNode := node // Work out what port they are talking to, and count the number of // connections to that port. @@ -90,7 +93,7 @@ func incomingConnectionsSummary(topologyID string, r report.Report, n report.Nod } key := connection{ localNode: &n, - remoteNode: &node, + remoteNode: &remoteNode, port: port, } if isInternetNode(n) { @@ -126,8 +129,13 @@ func outgoingConnectionsSummary(topologyID string, r report.Report, n report.Nod if !ok { continue } + remoteEndpointIDs := endpointChildIDsOf(node) + // Shallow-copy the node to obtain a different pointer + // on the remote side + remoteNode := node + for _, localEndpoint := range localEndpoints { _, localAddr, _, ok := report.ParseEndpointNodeID(localEndpoint.ID) if !ok { @@ -141,7 +149,7 @@ func outgoingConnectionsSummary(topologyID string, r report.Report, n report.Nod } key := connection{ localNode: &n, - remoteNode: &node, + remoteNode: &remoteNode, port: port, } if isInternetNode(n) { From 5d857d05d6c19fae541474c381f59e6a0a6d84ab Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Tue, 2 Aug 2016 12:42:26 +0000 Subject: [PATCH 3/3] Remove and optimize more Copy()s --- render/detailed/node.go | 2 +- render/detailed/summary.go | 57 +++------------------------ render/detailed/topology_diff_test.go | 2 +- report/metric_row.go | 20 +++------- report/metrics.go | 10 ----- report/metrics_test.go | 14 ------- 6 files changed, 14 insertions(+), 91 deletions(-) diff --git a/render/detailed/node.go b/render/detailed/node.go index c01a5e73a9..b13805a515 100644 --- a/render/detailed/node.go +++ b/render/detailed/node.go @@ -226,7 +226,7 @@ func children(r report.Report, n report.Node) []NodeSummaryGroup { for _, spec := range nodeSummaryGroupSpecs { if len(summaries[spec.topologyID]) > 0 { sort.Sort(nodeSummariesByID(summaries[spec.TopologyID])) - group := spec.NodeSummaryGroup.Copy() + group := spec.NodeSummaryGroup group.Nodes = summaries[spec.topologyID] nodeSummaryGroups = append(nodeSummaryGroups, group) } diff --git a/render/detailed/summary.go b/render/detailed/summary.go index a78f52436d..54e8f982f4 100644 --- a/render/detailed/summary.go +++ b/render/detailed/summary.go @@ -32,19 +32,6 @@ type NodeSummaryGroup struct { Columns []Column `json:"columns"` } -// Copy returns a value copy of the NodeSummaryGroup -func (g NodeSummaryGroup) Copy() NodeSummaryGroup { - result := NodeSummaryGroup{ - TopologyID: g.TopologyID, - Label: g.Label, - Columns: g.Columns, - } - for _, node := range g.Nodes { - result.Nodes = append(result.Nodes, node.Copy()) - } - return result -} - // Column provides special json serialization for column ids, so they include // their label for the frontend. type Column struct { @@ -94,35 +81,12 @@ func MakeNodeSummary(r report.Report, n report.Node) (NodeSummary, bool) { // SummarizeMetrics returns a copy of the NodeSummary where the metrics are // replaced with their summaries func (n NodeSummary) SummarizeMetrics() NodeSummary { - cp := n.Copy() - for i, m := range cp.Metrics { - cp.Metrics[i] = m.Summary() + summarizedMetrics := make([]report.MetricRow, len(n.Metrics)) + for i, m := range n.Metrics { + summarizedMetrics[i] = m.Summary() } - return cp -} - -// Copy returns a value copy of the NodeSummary -func (n NodeSummary) Copy() NodeSummary { - result := NodeSummary{ - ID: n.ID, - Label: n.Label, - LabelMinor: n.LabelMinor, - Rank: n.Rank, - Shape: n.Shape, - Stack: n.Stack, - Linkable: n.Linkable, - Adjacency: n.Adjacency.Copy(), - } - for _, row := range n.Metadata { - result.Metadata = append(result.Metadata, row.Copy()) - } - for _, table := range n.Tables { - result.Tables = append(result.Tables, table.Copy()) - } - for _, row := range n.Metrics { - result.Metrics = append(result.Metrics, row.Copy()) - } - return result + n.Metrics = summarizedMetrics + return n } func baseNodeSummary(r report.Report, n report.Node) NodeSummary { @@ -134,7 +98,7 @@ func baseNodeSummary(r report.Report, n report.Node) NodeSummary { Metadata: NodeMetadata(r, n), Metrics: NodeMetrics(r, n), Tables: NodeTables(r, n), - Adjacency: n.Adjacency.Copy(), + Adjacency: n.Adjacency, } } @@ -372,15 +336,6 @@ func Summaries(r report.Report, rns report.Nodes) NodeSummaries { return result } -// Copy returns a deep value-copy of NodeSummaries -func (n NodeSummaries) Copy() NodeSummaries { - result := NodeSummaries{} - for k, v := range n { - result[k] = v.Copy() - } - return result -} - // getRenderableContainerName obtains a user-friendly container name, to render in the UI func getRenderableContainerName(nmd report.Node) string { for _, key := range []string{ diff --git a/render/detailed/topology_diff_test.go b/render/detailed/topology_diff_test.go index dfdc360cda..93e21c2cdb 100644 --- a/render/detailed/topology_diff_test.go +++ b/render/detailed/topology_diff_test.go @@ -25,7 +25,7 @@ func TestTopoDiff(t *testing.T) { Pseudo: false, Adjacency: report.MakeIDList("nodeb"), } - nodeap := nodea.Copy() + nodeap := nodea nodeap.Adjacency = report.MakeIDList("nodeb", "nodeq") // not the same anymore nodeb := detailed.NodeSummary{ ID: "nodeb", diff --git a/report/metric_row.go b/report/metric_row.go index f92bdae1f6..b33b378510 100644 --- a/report/metric_row.go +++ b/report/metric_row.go @@ -27,21 +27,13 @@ type MetricRow struct { // Summary returns a copy of the MetricRow, without the samples, just the value if there is one. func (m MetricRow) Summary() MetricRow { - row := m.Copy() - if m.Metric != nil { - row.Metric.Samples = nil + if m.Metric.Len() > 0 { + // shallow-copy + metricCopy := *m.Metric + metricCopy.Samples = nil + m.Metric = &metricCopy } - return row -} - -// Copy returns a value copy of the MetricRow -func (m MetricRow) Copy() MetricRow { - row := m - if m.Metric != nil { - var metric = m.Metric.Copy() - row.Metric = &metric - } - return row + return m } // MarshalJSON shouldn't be used, use CodecEncodeSelf instead diff --git a/report/metrics.go b/report/metrics.go index b6b8b7a731..65ff5a35ec 100644 --- a/report/metrics.go +++ b/report/metrics.go @@ -94,16 +94,6 @@ func MakeMetric(samples []Sample) Metric { } } -// Copy returns a copy of the Metric. -func (m Metric) Copy() Metric { - c := m - if c.Samples != nil { - c.Samples = make([]Sample, len(m.Samples)) - copy(c.Samples, m.Samples) - } - return c -} - // WithMax returns a fresh copy of m, with Max set to max func (m Metric) WithMax(max float64) Metric { return Metric{ diff --git a/report/metrics_test.go b/report/metrics_test.go index 012c3c42d3..214e67b622 100644 --- a/report/metrics_test.go +++ b/report/metrics_test.go @@ -122,20 +122,6 @@ func TestMetricMerge(t *testing.T) { } } -func TestMetricCopy(t *testing.T) { - want := report.MakeMetric(nil) - have := want.Copy() - if !reflect.DeepEqual(want, have) { - t.Errorf("diff: %s", test.Diff(want, have)) - } - - want = report.MakeSingletonMetric(time.Now(), 1) - have = want.Copy() - if !reflect.DeepEqual(want, have) { - t.Errorf("diff: %s", test.Diff(want, have)) - } -} - func TestMetricDiv(t *testing.T) { t1 := time.Now() t2 := time.Now().Add(1 * time.Minute)