podman run - avoid calls to JSONDeepCopy #11781
Conversation
Access the container's spec field directly inside of libpod instead of calling Spec() which in turn creates expensive JSON deep copies. Accessing the field directly drops memory consumption of a simple podman run --rm busybox true from ~700kB to ~600kB. [NO TESTS NEEDED] Signed-off-by: Valentin Rothberg <[email protected]>
To avoid creating an expensive deep copy, create an internal function to access the exec session. [NO TESTS NEEDED] Signed-off-by: Valentin Rothberg <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrothberg 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
|
Is there reason why you closed the |
Based on your and @mheon's feedback, I decided to first optimize the code and then take another look, if needed, at optimizing JSONDeepCopy. @mheon pointed out https://github.com/kubernetes/gengo/tree/master/examples/deepcopy-gen which may be a nice alternative. |
Add a new function to libpod to directly access the runtime configuration without creating an expensive deep copy. Further migrate a number of callers to this new function. This drops the number of calls to JSONDeepCopy from 4 to 1 in a simple `podman run --rm -d busybox top`. Future work: Please note that there are more callers of GetConfig() that can me migrated to GetConfigNoCopy(). [NO TESTS NEEDED] Signed-off-by: Valentin Rothberg <[email protected]>
Do not create an expensive deep copy for the provided spec.Spec when creating a container. No API should be expected to create deep copies of arguments unless explicitly documented. This removes the last call to JSONDeepCopy in a simple `podman run --rm -d busybox true`. [NO TESTS NEEDED] Signed-off-by: Valentin Rothberg <[email protected]>
|
quay.io looks flaky at the moment. |
| // GetConfig returns the configuration used by the runtime. | ||
| // Note that the returned value is not a copy and must hence | ||
| // only be used in a reading fashion. | ||
| func (r *Runtime) GetConfigNoCopy() (*config.Config, error) { |
There was a problem hiding this comment.
I'm a little uncomfortable about this one, since it does give a backdoor into the Libpod config. I am OK with merging but would prefer more warnings telling people to avoid whenever possible, and how this is fastpath only, etc.
There was a problem hiding this comment.
(If we could get lint to throw errors whenever someone adds a use, requiring a manual whitelisting, that would be neat, but that sounds beyond the scope of this PR)
|
Whoooo, green. @containers/podman-maintainers PTAL |
|
LGTM |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
A series of commits to avoid all calls to JSONDeepCopy when running a simple
podman run --rm -d busybox true. After 5d6ea90 this drops another 100kB of memory usage.Please refer to the individual commits for change descriptions.
Note, this needs a head nod from @mheon.