-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
perf: improve applyOrdering by avoid call to GetByCurrentId #5079
Conversation
Welcome @chlunde! |
Hi @chlunde. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
/lgtm |
@yanniszark Do you have time to take a look at this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chlunde thanks a lot for your perf PRs, they are greatly needed and will massively improve the user experience.
I left 2 comments for small changes but overall if this passes tests it should be good to go.
/triage accepted |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
This shaves of 14 seconds (one third) of the execution time for a kustomization tree with 4000 documents, from 40.68s to 27.41s 0 0% 5.44% 18.42s 40.56% sigs.k8s.io/kustomize/api/krusty.(*Kustomizer).applySortOrder 0 0% 5.44% 18.40s 40.52% sigs.k8s.io/kustomize/api/internal/builtins.applyOrdering before (pprof) top20 -cum Showing nodes accounting for 5.85s, 12.88% of 45.41s total Dropped 622 nodes (cum <= 0.23s) Showing top 20 nodes out of 157 flat flat% sum% cum cum% 0 0% 0% 40.68s 89.58% github.com/spf13/cobra.(*Command).Execute 0 0% 0% 40.68s 89.58% github.com/spf13/cobra.(*Command).ExecuteC 0 0% 0% 40.68s 89.58% github.com/spf13/cobra.(*Command).execute 0 0% 0% 40.68s 89.58% main.main 0 0% 0% 40.68s 89.58% runtime.main 0 0% 0% 40.68s 89.58% sigs.k8s.io/kustomize/kustomize/v5/commands/build.NewCmdBuild.func1 0 0% 0% 40.12s 88.35% sigs.k8s.io/kustomize/api/krusty.(*Kustomizer).Run 0.51s 1.12% 1.12% 33.20s 73.11% sigs.k8s.io/kustomize/api/resource.(*Resource).CurId 0 0% 1.12% 26.95s 59.35% sigs.k8s.io/kustomize/api/resmap.(*resWrangler).GetMatchingResourcesByCurrentId 0.35s 0.77% 1.89% 26.95s 59.35% sigs.k8s.io/kustomize/api/resmap.(*resWrangler).filteredById 0.07s 0.15% 2.05% 25.53s 56.22% sigs.k8s.io/kustomize/api/resmap.GetCurrentId 0 0% 2.05% 21.68s 47.74% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).MakeCustomizedResMap (inline) 0 0% 2.05% 21.68s 47.74% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).makeCustomizedResMap 0.54s 1.19% 3.24% 19.75s 43.49% sigs.k8s.io/kustomize/api/resource.(*Resource).GetGvk (inline) 1s 2.20% 5.44% 19.21s 42.30% sigs.k8s.io/kustomize/kyaml/resid.GvkFromNode 0 0% 5.44% 18.42s 40.56% sigs.k8s.io/kustomize/api/internal/builtins.(*SortOrderTransformerPlugin).Transform 0 0% 5.44% 18.42s 40.56% sigs.k8s.io/kustomize/api/krusty.(*Kustomizer).applySortOrder 0 0% 5.44% 18.40s 40.52% sigs.k8s.io/kustomize/api/internal/builtins.applyOrdering 0.87s 1.92% 7.36% 16.55s 36.45% sigs.k8s.io/kustomize/kyaml/yaml.visitMappingNodeFields 2.51s 5.53% 12.88% 15.68s 34.53% sigs.k8s.io/kustomize/kyaml/yaml.visitFieldsWhileTrue after (pprof) top20 -cum Showing nodes accounting for 1.23s, 3.85% of 31.98s total Dropped 584 nodes (cum <= 0.16s) Showing top 20 nodes out of 184 flat flat% sum% cum cum% 0 0% 0% 27.41s 85.71% github.com/spf13/cobra.(*Command).Execute 0 0% 0% 27.41s 85.71% github.com/spf13/cobra.(*Command).ExecuteC 0 0% 0% 27.41s 85.71% github.com/spf13/cobra.(*Command).execute 0 0% 0% 27.41s 85.71% main.main 0 0% 0% 27.41s 85.71% runtime.main 0 0% 0% 27.41s 85.71% sigs.k8s.io/kustomize/kustomize/v5/commands/build.NewCmdBuild.func1 0 0% 0% 26.85s 83.96% sigs.k8s.io/kustomize/api/krusty.(*Kustomizer).Run 0 0% 0% 22.07s 69.01% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).MakeCustomizedResMap (inline) 0 0% 0% 22.07s 69.01% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).makeCustomizedResMap 0.38s 1.19% 1.19% 20.69s 64.70% sigs.k8s.io/kustomize/api/resource.(*Resource).CurId 0 0% 1.19% 13.64s 42.65% sigs.k8s.io/kustomize/api/resmap.(*resWrangler).Append 0 0% 1.19% 13.55s 42.37% sigs.k8s.io/kustomize/api/resmap.(*resWrangler).GetMatchingResourcesByCurrentId (inline) 0.12s 0.38% 1.56% 13.55s 42.37% sigs.k8s.io/kustomize/api/resmap.(*resWrangler).filteredById 0.01s 0.031% 1.59% 12.67s 39.62% sigs.k8s.io/kustomize/api/resmap.GetCurrentId 0.21s 0.66% 2.25% 12.49s 39.06% sigs.k8s.io/kustomize/api/resource.(*Resource).GetGvk (inline) 0.51s 1.59% 3.85% 12.28s 38.40% sigs.k8s.io/kustomize/kyaml/resid.GvkFromNode 0 0% 3.85% 11.52s 36.02% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).IgnoreLocal 0 0% 3.85% 10.53s 32.93% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).AccumulateTarget 0 0% 3.85% 10.53s 32.93% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).accumulateResources 0 0% 3.85% 10.53s 32.93% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).accumulateTarget
I believe I've addressed the comments, can you take another look? Thanks! |
@natasha41575 or @yanniszark - can we get this merged? I'd like some input on how to structure the next PR #5427 but since it creates merge conflicts with this PR it would be better if this is merged first. |
Hi, @koba1t can we get this merged? Thanks for taking a look :) |
@natasha41575 |
@natasha41575 could you check this again or delegate to someone else? |
@yanniszark I believe I've addressed your comments, could we get this merged now? |
Hi @chlunde and sorry for the late reply! |
HI @yanniszark |
@koba1t could we get another reviewer here please? Soon 1 year birthday 😄 🍰 (a lot of that time is on me) |
/lgtm |
@yanniszark: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@chlunde /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chlunde, koba1t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/reopen |
@koba1t: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/lgtm |
Sorry. Maybe I missed merging the last time. |
@koba1t thanks! 👍 |
This shaves of 14 seconds (one third) of the execution time for a kustomization tree with 4000 documents, from 40.68s to 27.41s
before
after