Skip to content
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.

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

Add topology.ReplaceNode() for efficiency #3073

Merged
merged 2 commits into from
Feb 19, 2018
Merged

Add topology.ReplaceNode() for efficiency #3073

merged 2 commits into from
Feb 19, 2018

Conversation

bboreham
Copy link
Collaborator

@bboreham bboreham commented Feb 18, 2018

In some places AddNode() was called after adding to an existing node, in which case the Merge() is just a waste of time.

Spotted in a memory profile of the probe: about 14% of all memory allocated is in Merge():

----------------------------------------------------------+-------------
      flat  flat%   sum%        cum   cum%   calls calls% + context 	 	 
----------------------------------------------------------+-------------
                                        16356.22MB 41.27% |   github.com/weaveworks/scope/probe.topologyTagger.Tag.func1 /go/src/github.com/weaveworks/scope/probe/topology_tagger.go
                                        12215.04MB 30.82% |   github.com/weaveworks/scope/probe/host.Tagger.Tag /go/src/github.com/weaveworks/scope/probe/host/tagger.go
                                         4821.62MB 12.17% |   github.com/weaveworks/scope/probe/endpoint.(*connectionTracker).addConnection /go/src/github.com/weaveworks/scope/probe/endpoint/connection_tracker.go
                                         2768.72MB  6.99% |   github.com/weaveworks/scope/probe/docker.(*Tagger).tag /go/src/github.com/weaveworks/scope/probe/docker/tagger.go
                                         1511.73MB  3.81% |   github.com/weaveworks/scope/probe/process.(*Reporter).processTopology.func1 /go/src/github.com/weaveworks/scope/probe/process/reporter.go
                                          557.68MB  1.41% |   github.com/weaveworks/scope/probe/endpoint.natMapper.applyNAT.func1 /go/src/github.com/weaveworks/scope/probe/endpoint/nat.go
                                          400.83MB  1.01% |   github.com/weaveworks/scope/probe/docker.(*Reporter).containerTopology /go/src/github.com/weaveworks/scope/probe/docker/reporter.go
                                          375.10MB  0.95% |   github.com/weaveworks/scope/probe/docker.(*Reporter).containerImageTopology.func1 /go/src/github.com/weaveworks/scope/probe/docker/reporter.go
...
         0     0%     0% 39630.96MB 17.84%                | github.com/weaveworks/scope/report.Topology.AddNode /go/src/github.com/weaveworks/scope/report/topology.go
                                        34675.80MB 87.50% |   github.com/weaveworks/scope/report.Node.Merge /go/src/github.com/weaveworks/scope/report/node.go
                                         4955.16MB 12.50% |   runtime.mapassign /usr/local/go/src/runtime/hashmap.go
----------------------------------------------------------+-------------

The three call sites in this PR are number 1, 2 and 4 in the above profile; number 3 is not directly replacing an existing node so left as-is.

@rade
Copy link
Member

rade commented Feb 18, 2018

In some places AddNode() was called after adding to an existing node, in which case the Merge() is just a waste of time.

That is only correct if all information in the existing node that needs preserving is also present in the new node. It is not at all obvious whether that is the case for the call sites changed in this PR.

In some places AddNode() was called after adding to an existing node,
in which case the Merge() is just a waste of time.
@bboreham
Copy link
Collaborator Author

I changed back one case where there is a curious variable shadowing to confuse the unwary.

Copy link
Member

@rade rade left a comment

Choose a reason for hiding this comment

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

LGTM, with one minor improvement suggestion.

@@ -113,6 +113,16 @@ func (t Topology) AddNode(node Node) Topology {
return t
}

// ReplaceNode adds node to the topology under key nodeID; if a
// node already exists for this key, node replaces that node.
// The same topology is returned to enable chaining.

This comment was marked as abuse.

This comment was marked as abuse.

@rade
Copy link
Member

rade commented Feb 19, 2018

Pls squash before merging.

@bboreham bboreham merged commit f72ced3 into master Feb 19, 2018
@bboreham bboreham deleted the replace-node branch February 19, 2018 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants