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

Use single-owner code path to accumulate children when rendering #3138

Merged
merged 2 commits into from
Apr 13, 2018

Conversation

bboreham
Copy link
Collaborator

We added ps.Map.UnsafeMutableSet() to optimise the case where you know a NodeSet is not shared with any other nodes. We can arrange this quite easily for the Children of each node in joinResults, hence speed up rendering.

I had to adjust the smart/dumb merger benchmarks when they showed an improvement, which is spurious because this code isn't involved in merging.

Benchmarks comparison between first commit and last commit:

benchmark                            old ns/op      new ns/op      delta
BenchmarkRenderList-2                575964931      553021897      -3.98%
BenchmarkRenderHosts-2               408721601      362484026      -11.31%
BenchmarkRenderControllers-2         334499307      334047789      -0.13%
BenchmarkRenderPods-2                298092408      289996139      -2.72%
BenchmarkRenderContainers-2          195267791      194515228      -0.39%
BenchmarkRenderProcesses-2           120603030      118073991      -2.10%
BenchmarkRenderProcessNames-2        140712909      137637707      -2.19%

benchmark                            old allocs     new allocs     delta
BenchmarkRenderList-2                803614         699045         -13.01%
BenchmarkRenderHosts-2               603945         508666         -15.78%
BenchmarkRenderControllers-2         427973         388357         -9.26%
BenchmarkRenderPods-2                383876         343816         -10.44%
BenchmarkRenderContainers-2          253831         238770         -5.93%
BenchmarkRenderProcesses-2           119587         104522         -12.60%
BenchmarkRenderProcessNames-2        163859         142304         -13.15%

benchmark                            old bytes     new bytes     delta
BenchmarkRenderList-2                91847940      79224980      -13.74%
BenchmarkRenderHosts-2               65673920      54034450      -17.72%
BenchmarkRenderControllers-2         46943949      41555952      -11.48%
BenchmarkRenderPods-2                41884841      36448283      -12.98%
BenchmarkRenderContainers-2          28795293      26194801      -9.03%
BenchmarkRenderProcesses-2           13985262      11506184      -17.73%
BenchmarkRenderProcessNames-2        17980727      14821811      -17.57%

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.

One naming niggle. Rest LGTM.

}

// SingleOwnerAdd adds the nodes to the NodeSet. Only call this if n has one owner.
func (n *NodeSet) SingleOwnerAdd(node Node) {

This comment was marked as abuse.

Make sure all the Children NodeSets are not shared with any other
nodes, then we can use the non-persistent add path.
@bboreham bboreham force-pushed the single-owner-nodeset branch from f60d947 to 7a03bc0 Compare April 13, 2018 07:45
@bboreham bboreham merged commit b7b7789 into master Apr 13, 2018
@bboreham bboreham deleted the single-owner-nodeset branch September 13, 2019 15:34
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