kustomize icon indicating copy to clipboard operation
kustomize copied to clipboard

perf: Improve resmap performance with AppendAll and Transform functions

Open chlunde opened this issue 2 years ago • 6 comments

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 f5cdc84e6b490a5235f4002efdd5d884310def39 perf: resmap: Add AppendMany function no effect on its own

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

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

chlunde avatar Oct 30 '23 21:10 chlunde

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 avatar Oct 30 '23 21:10 k8s-ci-robot

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 avatar Oct 30 '23 21:10 k8s-ci-robot

@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.

chlunde avatar Nov 13 '23 08:11 chlunde

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-triage-robot avatar Feb 11 '24 08:02 k8s-triage-robot

/remove-lifecycle stale

chlunde avatar Feb 16 '24 16:02 chlunde

[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

k8s-ci-robot avatar Apr 16 '24 17:04 k8s-ci-robot

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-triage-robot avatar Jul 15 '24 20:07 k8s-triage-robot

/remove-lifecycle stale /auto-cc

On Mon, Jul 15, 2024, 4:23 p.m. Kubernetes Triage Robot < @.***> wrote:

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:

Please send feedback to sig-contributor-experience at kubernetes/community https://github.com/kubernetes/community.

/lifecycle stale

— Reply to this email directly, view it on GitHub https://github.com/kubernetes-sigs/kustomize/pull/5427#issuecomment-2229335390, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFKQYME3E5I422CCVKRGXDZMQVVZAVCNFSM6AAAAAA6WWTZC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRZGMZTKMZZGA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

stormqueen1990 avatar Jul 15 '24 21:07 stormqueen1990

@stormqueen1990 Could I assign you for this and https://github.com/kubernetes-sigs/kustomize/pull/5425 PR?

koba1t avatar Jul 23 '24 21:07 koba1t

@koba1t yes, absolutely!

stormqueen1990 avatar Jul 23 '24 23:07 stormqueen1990

Hi there, @chlunde! 👋🏻

I noticed this PR is still in draft state. Would you like to have it be reviewed again?

stormqueen1990 avatar Jul 31 '24 21:07 stormqueen1990

I tried to run CI on GitHub Actions. /assign @stormqueen1990

koba1t avatar Aug 04 '24 17:08 koba1t