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

performance: Alter makeResIds.go's PrevIds to use a targeted APIVersion retrieval #4793

Closed
ephesused opened this issue Sep 12, 2022 · 3 comments · Fixed by #4809
Closed

performance: Alter makeResIds.go's PrevIds to use a targeted APIVersion retrieval #4793

ephesused opened this issue Sep 12, 2022 · 3 comments · Fixed by #4809
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@ephesused
Copy link
Contributor

Currently makeResIds.go's PrevIds retrieves the APIVersion by calling GetMeta. However, the only content that is used is the APIVersion. GetMeta is a lot heavier than GetApiVersion, and converting yields a meaningful performance improvement in moderately complex runs.

I'm planning to handle this one myself, since I'd like to use it as a working example to become a contributor.

/assign

Current code:

		meta, err := n.GetMeta()
		if err != nil {
			return nil, err
		}
		group, version := resid.ParseGroupVersion(meta.APIVersion)

Proposed change:

		apiVersion := n.GetApiVersion()
		group, version := resid.ParseGroupVersion(apiVersion)

Comparison analysis

Note that these builds are leveraging the proposed fix in #4790.

[kustomize]$ for i in 1 2 3; do time kustomize/kustomize-useGetMeta -o /tmp/useGetMeta.yaml build /tmp/generate/order-data-science-simple; done

real    0m16.158s
user    0m21.716s
sys     0m0.438s

real    0m16.137s
user    0m22.004s
sys     0m0.509s

real    0m16.717s
user    0m22.453s
sys     0m0.426s
[kustomize]$ for i in 1 2 3; do time kustomize/kustomize-useGetApiVersion -o /tmp/useGetApiVersion.yaml build /tmp/generate/order-data-science-simple; done

real    0m13.167s
user    0m17.383s
sys     0m0.429s

real    0m12.712s
user    0m16.875s
sys     0m0.387s

real    0m12.546s
user    0m16.759s
sys     0m0.333s
[kustomize]$ diff /tmp/useGetMeta.yaml /tmp/useGetApiVersion.yaml
[kustomize]$

Platform

linux

@ephesused ephesused added the kind/bug Categorizes issue or PR as related to a bug. label Sep 12, 2022
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Sep 12, 2022
@natasha41575
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 12, 2022
@ephesused
Copy link
Contributor Author

Quick status update... Currently I'm working through our process to get clearance for submissions.

As I've poked around a bit, I've been able to find some more opportunities for improvement. Here's my cumulative working patch:

patch-wider.txt

There are a few things going on in this combined patch, with the big one being the ability to limit the fields traversed by RNode.Fields(). That then enables PrevIds to limit its annotation retrieval to only the annotations that it needs, which in turn causes a significant decrease in what has to be built for the call. That means less processing time and fewer allocations.

There's still a lot of time spent in garbage collection. With or without the patch, my runs usually are spending 25% to 35% of their time handling gc.

At the moment I'm just hacking to see what I can do. Admittedly, I don't know whether this coding pattern is one that fits kustomize's standards. My approach was to make a call chain variadic, which allows for the field retrieval limits to pass through without changing existing behavior. @natasha41575, if you think I'm heading in the wrong direction here, please let me know.

I've got three test cases I've been using for figuring where to focus. One (ds) is a moderately sized and has a somewhat simple set of kustomizations. One (na) is very large and has a complex set of kustomizations. One (ro) is a large set of resources, with no kustomizations - it's just a resource load. Using go's profiling tools, here's how things look with this patch:

ds-cpu-current.svg: Showing nodes accounting for 11.35s, 58.03% of 19.56s total
ds-cpu-patched.svg: Showing nodes accounting for 6.51s, 52.25% of 12.46s total
ds-mem-current.svg: Showing nodes accounting for 8414.24MB, 79.88% of 10533.89MB total
ds-mem-patched.svg: Showing nodes accounting for 4684.62MB, 70.79% of 6617.97MB total

na-cpu-current.svg: Showing nodes accounting for 244.69s, 73.59% of 332.52s total
na-cpu-patched.svg: Showing nodes accounting for 116.69s, 66.44% of 175.63s total
na-mem-current.svg: Showing nodes accounting for 176831.23MB, 92.29% of 191607.52MB total
na-mem-patched.svg: Showing nodes accounting for 74186.29MB, 84.40% of 87899.61MB total

ro-cpu-current.svg: Showing nodes accounting for 43.18s, 81.63% of 52.90s total
ro-cpu-patched.svg: Showing nodes accounting for 20.18s, 81.37% of 24.80s total
ro-mem-current.svg: Showing nodes accounting for 26586.32MB, 96.92% of 27431.22MB total
ro-mem-patched.svg: Showing nodes accounting for 12034.92MB, 96.34% of 12491.90MB total

@ephesused
Copy link
Contributor Author

I realized it'd be best to keep the PR for this issue specific to what the issue describes. I'll pursue the other fixes separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants