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

perf: Improve resmap performance with AppendAll and Transform functions #5427

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

perf: Improve resmap performance with AppendAll and Transform functions #5427

wants to merge 8 commits into from

Conversation

chlunde
Copy link
Contributor

@chlunde chlunde commented Oct 30, 2023 •
edited

Personal notes, keeping as draft :)

Rebased, since the SortOrderTransformer PR is in

With the benchmark suite, this PR reduces the runtime from 48s to 2.5s on my machine.
I think we could split it into multiple independent PRs now. I've benchmarked them on their own to see where the major effect

perf: ResAccumulator: Use map for resmap intersection

standalone. 48s to 47s for the benchmark.

Transform

perf: NamespaceTransformer: Use Transform function for conflict detection
perf: resmap: Add Transform function

pair of PRs, 48s to 47s for the benchmark

AppendMany

commit f5cdc84
perf: resmap: Add AppendMany function
no effect on its own

commit e06a969
perf: improve applyOrdering by avoid call to GetByCurrentId
little effect on its own

commit f6ed5d5
perf: Improve SortOrderTransformer and use AppendMany
commit 0fe5421
perf: Use AppendMany for SortOrderTransformer/newResMapFromResourceSlice/appendAll
should split this. Here's the major change 48s->2.5s Could split this into more pieces.

@k8s-ci-robot
Copy link
Contributor

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2023
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 30, 2023
@chlunde chlunde changed the title Improve resmap performance alt two Improve resmap performance alternative two Oct 30, 2023
@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 30, 2023
@chlunde chlunde mentioned this pull request Oct 30, 2023
Open
1 task
@chlunde chlunde changed the title Improve resmap performance alternative two perf: Improve resmap performance with AppendAll and Transform functions Oct 30, 2023
@chlunde chlunde mentioned this pull request Nov 13, 2023
Merged
@chlunde
Copy link
Contributor Author

chlunde commented Nov 13, 2023

@annasong20 would you like this as one PR, or do you prefer to split some of the commits into different three PRs? I've attempted to make it easier to remove by having a logical story if you start at the first commit, except for perf: improve applyOrdering by avoid call to GetByCurrentId which is actually another PR that this depends on.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 11, 2024
@chlunde
Copy link
Contributor Author

chlunde commented Feb 16, 2024

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 16, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2024
@chlunde
perf: resmap: Add Transform function
7202cdb 
Introduce a new function, Transform, which can be used to safely modify
entries an resmap.

The callback function is called for each entry in the map and handles ID
conflict detection, which was the most expensive operation causing
significant performance issues.

The ID cache is local to this function as we cannot guarantee that all
mutations are performed using this function.
@chlunde
perf: NamespaceTransformer: Use Transform function for conflict detec…
0990677 
…tion

This avoids expensive calls to GetMatchingResourcesByCurrentId
@chlunde
perf: resmap: Add AppendMany function
f5cdc84 
Introduce a new function, AppendMany, which can be used to safely add
entries to a resmap.

The function handles ID conflict detection, which was the most expensive
operation causing significant performance issues.

The ID cache is local to this function as we cannot guarantee that all
mutations are performed using this function.
@chlunde
perf: improve applyOrdering by avoid call to GetByCurrentId
e06a969 
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
@chlunde
perf: Use AppendMany for SortOrderTransformer/newResMapFromResourceSl…
0fe5421 
…ice/appendAll

Append in a loop is very expensive, O(N*M). By using AppendMany we get closer to O(N+M)
instead.
@chlunde
perf: Improve SortOrderTransformer and use AppendMany
f6ed5d5 
Reduce calls to CurId when sorting, and use AppendMany when collecting
results.
@chlunde
perf: ResAccumulator: Use map for resmap intersection
4188920
@chlunde
NOTE: Draft, commiting
01e154a 
Regenerate plugins

	make generate-kustomize-builtin-plugins
	gofmt -s -w -l plugin/builtin/sortordertransformer/SortOrderTransformer.go etc

Possibly split in two, one for appendmany another for transform

Run benchmark on both
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chlunde
Once this PR has been reviewed and has the lgtm label, please assign knverey for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@chlunde chlunde mentioned this pull request Apr 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annasong20 annasong20 Awaiting requested review from annasong20

@yuwenma yuwenma Awaiting requested review from yuwenma

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@chlunde @k8s-ci-robot @k8s-triage-robot