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 unsafe merge in joinResults.addChildAndChildren() #3143

Merged
merged 2 commits into from
Apr 15, 2018

Conversation

bboreham
Copy link
Collaborator

This was missed from #3138. It's a bug as-is - Merge() can return the rhs which is not safe to do further 'unsafe' operations on.

Benchmarks show the impact is mixed:

benchmark                         old ns/op     new ns/op     delta
BenchmarkRenderList-2             553973514     505759984     -8.70%
BenchmarkRenderHosts-2            345194045     366304720     +6.12%
BenchmarkRenderControllers-2      314828412     309772476     -1.61%
BenchmarkRenderPods-2             283650521     313774503     +10.62%
BenchmarkRenderContainers-2       187017594     194096252     +3.79%
BenchmarkRenderProcesses-2        121727679     111295296     -8.57%
BenchmarkRenderProcessNames-2     127764403     121302933     -5.06%

benchmark                         old allocs     new allocs     delta
BenchmarkRenderList-2             684645         621168         -9.27%
BenchmarkRenderHosts-2            486703         497536         +2.23%
BenchmarkRenderControllers-2      376311         358656         -4.69%
BenchmarkRenderPods-2             325505         334824         +2.86%
BenchmarkRenderContainers-2       221536         221534         -0.00%
BenchmarkRenderProcesses-2        104514         104513         -0.00%
BenchmarkRenderProcessNames-2     142520         130292         -8.58%

benchmark                         old bytes     new bytes     delta
BenchmarkRenderList-2             76111276      69272488      -8.99%
BenchmarkRenderHosts-2            50265090      51547970      +2.55%
BenchmarkRenderControllers-2      39539499      37658241      -4.76%
BenchmarkRenderPods-2             33302721      34403579      +3.31%
BenchmarkRenderContainers-2       23492377      23494531      +0.01%
BenchmarkRenderProcesses-2        11504046      11503747      -0.00%
BenchmarkRenderProcessNames-2     14845492      13599020      -8.40%

Also comment the add() call introduced in #3135 is inconsistent with #3138 - no impact as the two sets of calls are never used together, but it's not ideal. Changing add() to copy children makes performance worse.

@bboreham bboreham merged commit 994493b into master Apr 15, 2018
@bboreham bboreham deleted the unsafe-add-children 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