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

UPSTREAM: 54257: Use GetByKey() in lister NonNamespacedGet #45

Closed

Conversation

tiran
Copy link

@tiran tiran commented Oct 20, 2017

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

Signed-off-by: Christian Heimes cheimes@redhat.com

@@ -1,4 +1,5 @@
#!/bin/bash
set -x
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nuke

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

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran tiran force-pushed the upstream_54257_getbykey-1.7.6 branch from cbda981 to bc159fd Compare October 20, 2017 13:53
@enj
Copy link

enj commented Oct 23, 2017

/lgtm

@openshift-ci-robot
Copy link

@enj: changing LGTM is restricted to assignees, and assigning you to the PR failed.

In response to this:

/lgtm

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.

@deads2k
Copy link

deads2k commented Oct 23, 2017

If this merged into openshift/origin, then this can be closed. We'll get the code during a sync step.

@tiran
Copy link
Author

tiran commented Oct 24, 2017

backport to 1.7 is in progress

@enj
Copy link

enj commented Oct 30, 2017

All backports are merged so this can be closed.

@soltysh
Copy link
Member

soltysh commented Nov 3, 2017

This was picked in origin openshift/origin#16986. We'll pick it up properly to 1.8 rebase branch. @mfojtik this needs to be picked from origin, since this will be part of 1.8.3 only.

@soltysh soltysh closed this Nov 3, 2017
@tiran tiran deleted the upstream_54257_getbykey-1.7.6 branch November 3, 2017 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants