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.

Sign up for GitHub

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

Jump to bottom

Remove and optimize more Copy()s #1739

Merged
merged 3 commits into from
Aug 12, 2016
Merged

Remove and optimize more Copy()s #1739

merged 3 commits into from
Aug 12, 2016

Conversation

rade
Copy link
Member

@rade rade commented Aug 2, 2016

Part of #1730.

@rade
Copy link
Member Author

rade commented Aug 2, 2016

This reduces CPU usage by ~15% in my tests.

@2opremio This currently fails a couple of unit tests. Can't work out why. Mind taking a look?

@2opremio 2opremio self-assigned this Aug 2, 2016
2opremio
2opremio reviewed Aug 2, 2016
View reviewed changes
report/node.go
result := n.Copy()
result.Metrics[key] = n.Metrics[key].Merge(metric)
return result
n.Metrics = n.Metrics.Copy()

This comment was marked as abuse.

Sign in to view

@2opremio
Copy link
Contributor

2opremio commented Aug 2, 2016 •
edited
Loading

This currently fails a couple of unit tests. Can't work out why. Mind taking a look?

I am not an expert on the rendering pipeline, but I will take a look.

2opremio
2opremio reviewed Aug 2, 2016
View reviewed changes
report/string_set.go
@@ -54,33 +54,35 @@ func (s StringSet) Intersection(b StringSet) StringSet {
// Add adds the strings to the StringSet. Add is the only valid way to grow a
// StringSet. Add returns the StringSet to enable chaining.
func (s StringSet) Add(strs ...string) StringSet {
r := s.Copy()

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@rade
get rid of Node.Copy()
1091225 
A shallow copy is sufficient. Which we get for free in most cases
since Node is passed around by value.
@2opremio 2opremio force-pushed the 1730-no-node-copy branch from 044e9d7 to c180c56 Compare August 2, 2016 13:22
@2opremio
Copy link
Contributor

2opremio commented Aug 2, 2016

Rebased and added some more improvements.

@2opremio 2opremio changed the title [WIP] don't Copy() Node [WIP] Remove and optimize Copy()s Aug 2, 2016
@2opremio 2opremio changed the title [WIP] Remove and optimize Copy()s [WIP] Remove and optimize more Copy()s Aug 2, 2016
@2opremio 2opremio force-pushed the 1730-no-node-copy branch from c180c56 to 5d857d0 Compare August 2, 2016 13:29
@2opremio 2opremio changed the title [WIP] Remove and optimize more Copy()s Remove and optimize more Copy()s Aug 2, 2016
@2opremio
Copy link
Contributor

2opremio commented Aug 2, 2016 •
edited
Loading

I see a very minor improvement in CPU consumption in the Scope App in dev-c4. Maybe I wasn't interacting with the details panel enough (it's hard to try to do it manually and spread usage fairly between tests).

Before:

$ go tool pprof --seconds 90 http://localhost:4040/debug/pprof/profile
Fetching profile from http://localhost:4040/debug/pprof/profile?seconds=90
Please wait... (1m30s)
Saved profile in /Users/fons/pprof/pprof.localhost:4040.samples.cpu.001.pb.gz
Entering interactive mode (type "help" for commands)
(pprof) top10
31.14s of 60.94s total (51.10%)
Dropped 456 nodes (cum <= 0.30s)
Showing top 10 nodes out of 208 (cum >= 1.12s)
      flat  flat%   sum%        cum   cum%
     8.01s 13.14% 13.14%     12.48s 20.48%  runtime.scanobject
     5.44s  8.93% 22.07%      8.88s 14.57%  runtime.heapBitsSweepSpan
     3.44s  5.64% 27.72%      3.44s  5.64%  runtime.(*mspan).sweep.func1
     2.85s  4.68% 32.39%     10.08s 16.54%  runtime.mallocgc
     2.76s  4.53% 36.92%      2.76s  4.53%  runtime.memmove
     2.33s  3.82% 40.74%      2.34s  3.84%  runtime.heapBitsSetType
     1.79s  2.94% 43.68%      2.79s  4.58%  runtime.greyobject
     1.78s  2.92% 46.60%      1.78s  2.92%  runtime.heapBitsForObject
     1.66s  2.72% 49.33%      1.66s  2.72%  runtime.duffcopy
     1.08s  1.77% 51.10%      1.12s  1.84%  runtime.mapiternext
(pprof)

After:

$ go tool pprof --seconds 90 http://localhost:4040/debug/pprof/profile
Fetching profile from http://localhost:4040/debug/pprof/profile?seconds=90
Please wait... (1m30s)
Saved profile in /Users/fons/pprof/pprof.localhost:4040.samples.cpu.002.pb.gz
Entering interactive mode (type "help" for commands)
(pprof) top10
29.03s of 59.84s total (48.51%)
Dropped 480 nodes (cum <= 0.30s)
Showing top 10 nodes out of 205 (cum >= 1.15s)
      flat  flat%   sum%        cum   cum%
     7.60s 12.70% 12.70%     12.13s 20.27%  runtime.scanobject
     5.51s  9.21% 21.91%      8.34s 13.94%  runtime.heapBitsSweepSpan
     2.83s  4.73% 26.64%      2.83s  4.73%  runtime.(*mspan).sweep.func1
     2.49s  4.16% 30.80%     10.39s 17.36%  runtime.mallocgc
     2.48s  4.14% 34.94%      2.48s  4.14%  runtime.memmove
     2.22s  3.71% 38.65%      2.24s  3.74%  runtime.heapBitsSetType
     2.08s  3.48% 42.13%      3.27s  5.46%  runtime.greyobject
     1.42s  2.37% 44.50%      1.42s  2.37%  runtime.heapBitsForObject
     1.25s  2.09% 46.59%      1.25s  2.09%  runtime.duffcopy
     1.15s  1.92% 48.51%      1.15s  1.92%  github.com/weaveworks/scope/vendor/github.com/weaveworks/ps.hashKey
(pprof)

@2opremio 2opremio assigned paulbellamy and unassigned 2opremio Aug 2, 2016
paulbellamy
paulbellamy reviewed Aug 2, 2016
View reviewed changes
render/container.go
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)

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@paulbellamy paulbellamy removed their assignment Aug 2, 2016
@rade
Copy link
Member Author

rade commented Aug 2, 2016

I can no longer see any statistically significant improvement between this branch and master :(

@2opremio
Copy link
Contributor

2opremio commented Aug 3, 2016

I think we should merge it anyhow to remove unneeded uses of Copy() which prompt to abuse it.

@paulbellamy Any further comments?

@paulbellamy
Copy link
Contributor

Nope

@rade rade merged commit 21c188d into master Aug 12, 2016
@2opremio 2opremio deleted the 1730-no-node-copy branch February 13, 2017 12:17
rade added a commit that referenced this pull request Dec 28, 2017
@rade
don't propagate ImageID in MapContainerImage2Name
0c726e9 
This was building a set of all the image ids represented by the same
unversioned image. Well, it was doing that until I broke it with a
silly mistake in #1739 - instead of extracting the imageID from the
original node ID, it's extracting it from the updated ID, which is the
unversioned image. Even if it was working though, it's pointless
since nothing is looking at that info.
rade added a commit that referenced this pull request Dec 28, 2017
@rade
don't propagate ImageID in MapContainerImage2Name
9034296 
This was building a set of all the image ids represented by the same
unversioned image. Well, it was doing that until I broke it with a
silly mistake in #1739 - instead of extracting the imageID from the
original node ID, it's extracting it from the updated ID, which is the
unversioned image. Even if it was working though, it's pointless
since nothing is looking at that info.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rade @2opremio @paulbellamy