Use GetByKey() in typeLister_NonNamespacedGet

[tiran]

The Get() function of non-namespace lister passes a temporary object to
indexer.Get() in order to fetch the actual object from the indexer. This
may cause Go to allocate the temporary object on the heap instead of the
stack, as it is passed into interfaces. For non-namespaced objects,
Get(&Type{ObjectMeta: v1.ObjectMeta{Name: name}}) should be equivalent
to GetByKey(name).

This could be the root cause of excessive allocations, e.g. in tests
clusterRoleLister.Get() has trigger 4 billion allocations. See
openshift/origin#16954

[k8s-ci-robot]

Hi @tiran. Thanks for your PR.

I'm waiting for a kubernetes 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.

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. I understand the commands that are listed here.

[tiran]

CC @enj @sttts @deads2k

[eparis]

/ok-to-test

[enj]

/lgtm
/assign @deads2k @sttts
/unassign @feiskyer @saad-ali

[enj]

/lgtm

[sttts]

/lgtm
/approve no-issue

[k8s-ci-robot]

@tiran: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

[sttts]

Removed the release-note. This change is too internal to be seen in the changelogs.

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enj, sttts, tiran
We suggest the following additional approver: deads2k

Assign the PR to them by writing /assign @deads2k in a comment when ready.

Associated issue: 16954

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

Needs approval from an approver in each of these OWNERS Files:

• pkg/client/OWNERS
• staging/src/k8s.io/apiextensions-apiserver/OWNERS [sttts]
• staging/src/k8s.io/client-go/OWNERS [sttts]
• staging/src/k8s.io/code-generator/OWNERS [sttts]
• staging/src/k8s.io/kube-aggregator/OWNERS
• staging/src/k8s.io/sample-apiserver/OWNERS [sttts]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[sttts]

First commit is approved. 2nd and 3rd are the tail of generated code only, approving.

[k8s-github-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 53194, 54257, 53014). If you want to cherry-pick this change to another branch, please follow the instructions here.

[enj]

@sttts @deads2k do we want to backport this to any older Kube versions?

[deads2k]

@sttts @deads2k do we want to backport this to any older Kube versions?

Perhaps after proof this is the problem.

[enj]

So I did a simple benchmark:

go test -bench=ClusterRoleGetter -benchtime=60s -cpuprofile=cpu1.out -memprofile=mem1.out

var dataD *rbac.ClusterRole
var errD error

func BenchmarkClusterRoleGetter(b *testing.B) {
	idx := cache.NewIndexer(cache.DeletionHandlingMetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc})
	crl := bootstrappolicy.ClusterRoles()
	for i := range crl {
		if err := idx.Add(&crl[i]); err != nil {
			b.Fatal(err)
		}
	}
	var crg rbacregistryvalidation.ClusterRoleGetter = &ClusterRoleGetter{
		Lister: rbaclisters.NewClusterRoleLister(idx),
	}
	var data *rbac.ClusterRole
	var err error
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		for _, cr := range crl {
			data, err = crg.GetClusterRole(cr.Name)
			if err != nil {
				b.Fatal(err)
			}
		}
	}
	dataD = data
	errD = err
}

Allocations before this change:

(pprof) top10
56.04GB of 56.05GB total (  100%)
Dropped 61 nodes (cum <= 0.28GB)
      flat  flat%   sum%        cum   cum%
   56.04GB   100%   100%    56.04GB   100%  k8s.io/kubernetes/pkg/client/listers/rbac/internalversion.(*clusterRoleLister).Get
         0     0%   100%    56.04GB   100%  k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac.(*ClusterRoleGetter).GetClusterRole
         0     0%   100%    56.04GB   100%  k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac.BenchmarkClusterRoleGetter
         0     0%   100%    56.05GB   100%  runtime.goexit
         0     0%   100%    56.04GB   100%  testing.(*B).launch
         0     0%   100%    56.04GB   100%  testing.(*B).runN

(pprof) list clusterRoleLister\).Get      
Total: 56.05GB
ROUTINE ======================== k8s.io/kubernetes/pkg/client/listers/rbac/internalversion.(*clusterRoleLister).Get in /home/mkhan/go/src/k8s.io/kubernetes/pkg/client/listers/rbac/internalversion/clusterrole.go
   56.04GB    56.04GB (flat, cum)   100% of Total
         .          .     53:	return ret, err
         .          .     54:}
         .          .     55:
         .          .     56:// Get retrieves the ClusterRole from the index for a given name.
         .          .     57:func (s *clusterRoleLister) Get(name string) (*rbac.ClusterRole, error) {
   56.04GB    56.04GB     58:	key := &rbac.ClusterRole{ObjectMeta: v1.ObjectMeta{Name: name}}
         .          .     59:	obj, exists, err := s.indexer.Get(key)
         .          .     60:	if err != nil {
         .          .     61:		return nil, err
         .          .     62:	}
         .          .     63:	if !exists {

CPU before this change:

(pprof) top20   
118.16s of 152.79s total (77.33%)
Dropped 174 nodes (cum <= 0.76s)
Showing top 20 nodes out of 79 (cum >= 70.03s)
      flat  flat%   sum%        cum   cum%
    42.06s 27.53% 27.53%     58.75s 38.45%  runtime.scanobject
    11.59s  7.59% 35.11%     11.68s  7.64%  runtime.greyobject
     9.02s  5.90% 41.02%      9.02s  5.90%  runtime.heapBitsForObject
     7.19s  4.71% 45.72%      7.19s  4.71%  runtime.memclrNoHeapPointers
     6.13s  4.01% 49.73%     28.05s 18.36%  runtime.mallocgc
     5.13s  3.36% 53.09%      5.13s  3.36%  runtime.heapBitsSetType
     4.78s  3.13% 56.22%      7.82s  5.12%  runtime.mapaccess2_faststr
     4.54s  2.97% 59.19%      4.54s  2.97%  runtime.duffcopy
     3.20s  2.09% 61.29%      3.20s  2.09%  runtime.getitab
     2.98s  1.95% 63.24%     63.06s 41.27%  runtime.gcDrain
     2.95s  1.93% 65.17%      2.95s  1.93%  runtime.aeshashbody
     2.82s  1.85% 67.01%     10.11s  6.62%  runtime.deferreturn
     2.56s  1.68% 68.69%      4.17s  2.73%  runtime.scanblock
     2.29s  1.50% 70.19%     93.38s 61.12%  runtime.systemstack
     2.10s  1.37% 71.56%      8.22s  5.38%  runtime.writebarrierptr_prewrite1
     1.97s  1.29% 72.85%      1.97s  1.29%  sync.(*RWMutex).RLock
     1.81s  1.18% 74.04%      1.87s  1.22%  runtime.lock
     1.70s  1.11% 75.15%     30.51s 19.97%  k8s.io/kubernetes/vendor/k8s.io/client-go/tools/cache.(*threadSafeMap).Get
     1.70s  1.11% 76.26%      4.98s  3.26%  runtime.freedefer
     1.64s  1.07% 77.33%     70.03s 45.83%  k8s.io/kubernetes/pkg/client/listers/rbac/internalversion.(*clusterRoleLister).Get

---

Allocations after this change:

(pprof) top10
7446.45kB of 7446.45kB total (  100%)
Showing top 10 nodes out of 42 (cum >= 863.81kB)
      flat  flat%   sum%        cum   cum%
 2060.18kB 27.67% 27.67%  2060.18kB 27.67%  runtime/pprof/internal/protopprof.TranslateCPUProfile
 1024.28kB 13.76% 41.42%  2048.56kB 27.51%  encoding/json.Unmarshal
 1024.28kB 13.76% 55.18%  1538.91kB 20.67%  runtime.mapassign
  902.59kB 12.12% 67.30%  1447.25kB 19.44%  compress/flate.NewWriter
  863.81kB 11.60% 78.90%   863.81kB 11.60%  bytes.makeSlice
  544.67kB  7.31% 86.21%   544.67kB  7.31%  compress/flate.(*compressor).init
  514.63kB  6.91% 93.12%   514.63kB  6.91%  runtime.hashGrow
  512.02kB  6.88%   100%   512.02kB  6.88%  k8s.io/kubernetes/vendor/golang.org/x/net/http2/hpack.addDecoderNode
         0     0%   100%   863.81kB 11.60%  bytes.(*Buffer).Write
         0     0%   100%   863.81kB 11.60%  bytes.(*Buffer).grow

(pprof) list clusterRoleLister\).Get
Total: 7.27MB

CPU after this change:

(pprof) top20
1.96mins of 1.98mins total (98.86%)
Dropped 26 nodes (cum <= 0.01mins)
      flat  flat%   sum%        cum   cum%
  0.33mins 16.66% 16.66%   0.33mins 16.66%  runtime.duffcopy
  0.25mins 12.51% 29.17%   0.42mins 21.05%  runtime.mapaccess2_faststr
  0.16mins  8.29% 37.46%   0.16mins  8.29%  runtime.aeshashbody
  0.15mins  7.70% 45.16%   0.15mins  7.70%  sync.(*RWMutex).RLock
  0.13mins  6.48% 51.65%   0.13mins  6.48%  sync.(*RWMutex).RUnlock
  0.12mins  6.19% 57.83%   1.51mins 76.29%  k8s.io/kubernetes/pkg/client/listers/rbac/internalversion.(*clusterRoleLister).Get
  0.12mins  6.12% 63.95%   0.26mins 13.28%  runtime.deferreturn
  0.10mins  5.14% 69.10%   0.10mins  5.14%  runtime.newdefer
  0.10mins  4.95% 74.05%   0.13mins  6.55%  runtime.freedefer
  0.10mins  4.88% 78.93%   1.30mins 65.62%  k8s.io/kubernetes/vendor/k8s.io/client-go/tools/cache.(*threadSafeMap).Get
  0.09mins  4.74% 83.67%   1.98mins 99.85%  k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac.BenchmarkClusterRoleGetter
  0.09mins  4.60% 88.27%   0.22mins 10.89%  runtime.deferproc
  0.09mins  4.48% 92.75%   1.39mins 70.10%  k8s.io/kubernetes/vendor/k8s.io/client-go/tools/cache.(*cache).GetByKey
  0.07mins  3.32% 96.07%   1.58mins 79.60%  k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac.(*ClusterRoleGetter).GetClusterRole
  0.04mins  1.94% 98.01%   0.04mins  1.94%  runtime.jmpdefer
  0.02mins  0.84% 98.86%   0.02mins  0.84%  runtime.getcallerpc
         0     0% 98.86%   1.98mins 99.85%  runtime.goexit
         0     0% 98.86%   1.98mins 99.85%  testing.(*B).launch
         0     0% 98.86%   1.98mins 99.85%  testing.(*B).runN

[tiran]

@sttts @deads2k

@enj test case shows that the patch has a positive impact on memory allocation. In my opinion the fix should be backported to 1.8 and 1.7. I don't have permission to set the backport labels. We should backport only d57280e and regenerate the other files in each branch.

[deads2k]

@wojtek-t This had a big impact on rbac authorization performance.

[k8s-cherrypick-bot]

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

[k8s-cherrypick-bot]

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.