Avoid unnecessary calls to Container.Config() and Container.Spec() #16123
Conversation
|
@alexlarsson: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
openshift-ci
bot
added
the
do-not-merge/release-note-label-needed
github-actions
bot
added
the
kind/api-change
| @@ -688,6 +688,14 @@ func (c *Container) Terminal() bool { | |||
| return false | |||
| } | |||
|
|
|||
| // LinuxResources return the contaienrs Linux Resources (if any) | |||
| func (c *Container) LinuxResources() *spec.LinuxResources { | |||
There was a problem hiding this comment.
Does this change really have positive performance impacts? I'd expect the compiler to be smart enough to allocate a variable but did not decompile.
What callers now have is one additional nil check (i.e., if resources != nil), so my gut feeling is performance suffered.
There was a problem hiding this comment.
Most of the callers were indeed not affected, and some may have actually been made a few cycles slower. However, the call in moveConmonToCgroupAndSignal() now avoids a call to container.Spec() which saves around 2 milliseconds (bazillions of cycles), so it does affect performance.
Now, one could add a hack just for that particular caller, but imho most of the callers that were changed to use the helper are shorter, easier to read, and generally more maintainable, while suffering negligible performance decrease.
There was a problem hiding this comment.
(8 million cycles on my machine)
There was a problem hiding this comment.
Thanks for elaborating on that. The argument seems convincing to me. @mheon WDYT?
|
LGTM |
This gets c.config.Spec.Linux.Resources, with some nil checks. Using this means less open coding of the nil-checks, but also the existing user of this field in moveConmonToCgroupAndSignal() was using ctr.Spec().Linux.Resources instead, and the Spec() call is very expensive. [NO NEW TESTS NEEDED] Just minor performance effects Signed-off-by: Alexander Larsson <[email protected]>
This call does a deep copy, which is only needed if you want to modify the return value. Instead we use ctr.ConfigNoCopy().Spec which is just a pointer dereference. [NO NEW TESTS NEEDED] Just minor performance effects Signed-off-by: Alexander Larsson <[email protected]>
This is a very expensive function as it does a deep copy. Instead use pre-existing accessors like ctr.CreatedTime() where they exist and ctr.ConfigNoCopy() where not. [NO NEW TESTS NEEDED] Just minor performance effects Signed-off-by: Alexander Larsson <[email protected]>
This is a very expensive call as it deep duplicates the Config, and we just need to read a single member, so use ConfigNoCopy() instead. [NO NEW TESTS NEEDED] Just minor performance effects Signed-off-by: Alexander Larsson <[email protected]>
alexlarsson
force-pushed
the
less-json-dup
branch
from
27cbcc7 to
d08b4c1
Compare
rhatdan
removed
the
do-not-merge/release-note-label-needed
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexlarsson, rhatdan 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 |
openshift-ci
bot
added
the
approved
github-actions
bot
added
the
locked - please file new issue/PR
As mentioned in #16117 there are more places in the codebase where we unnecessarily call these expensive functions. This removes a bunch of them by calling existing or new accessors, or in some cases by using .ConfigNoCopy() instead.
@vrothberg