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

Reduce bugtool memory usage #17546

Merged
merged 6 commits into from Oct 7, 2021
Merged

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Oct 6, 2021

See individual commits and #17309 for details.

On my test system this reduces total RSS by about one third:

Before:

$ /usr/bin/time -f 'RSS=%MKB' cilium-bugtool
RSS=63168KB

After:

$ /usr/bin/time -f 'RSS=%MKB' cilium-bugtool
RSS=42752KB

Fixes #17309

s/ethool/ethtool/

Also fix the build for other non-Linux platforms besides macOS.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
Avoid reallocations and GC pressure in the loop.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/bugtool Impacts gathering of data for debugging purposes. labels Oct 6, 2021
@tklauser tklauser requested review from a team as code owners October 6, 2021 11:49
@tklauser tklauser requested a review from rolinh October 6, 2021 11:49
@tklauser
Copy link
Member Author

tklauser commented Oct 6, 2021

/test

Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

🚀

bugtool/cmd/helper.go Outdated Show resolved Hide resolved
There is no need to recompile the constant regexp for each iteration of
the loop. Also, hashEncryptionKeys is called in a loop, so move the
regexp compilation to a global var to avoid recompiling it.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
Instead of open-coding a worker pool use the existing
github.com/cilium/workerpool package. Also limit the number of workers
to the number of CPUs by default, which should help to reduce excessive
memory usage by too many parallel goroutines at the price of potentially
slightly slower bugtool report creation.

If users want more parallel tasks, the number can be specified using the
newly introduces `--parallel-workers` option.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
Converting []byte to string can cause allocation due to the fact that
strings are immutable in Go. By letting execCommand return []byte
instead of string we can avoid some of these allocations, i.e also the
allcoation caused by further processing of that return value.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser force-pushed the pr/tklauser/bugtool-mem-usage branch from c61ff98 to e090a65 Compare October 6, 2021 13:14
When the command output doiesn't need to be postprocessed, the output
can be written directly to the file without buffering. This should
significantly reduce memory usage.

On my test system this reduces total RSS by about one third:

Before:

    $ /usr/bin/time -f 'RSS=%MKB' cilium-bugtool
    RSS=63168KB

After:

    $ /usr/bin/time -f 'RSS=%MKB' cilium-bugtool
    RSS=42752KB

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser force-pushed the pr/tklauser/bugtool-mem-usage branch from e090a65 to 68d1f63 Compare October 6, 2021 14:09
@tklauser
Copy link
Member Author

tklauser commented Oct 6, 2021

/test

@tklauser
Copy link
Member Author

tklauser commented Oct 6, 2021

Multicluster test is the only one failing. That test is not required and the failure will be fixed by #17549. All other enabled tests passed, marking as ready-to-merge.

@tklauser tklauser added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 6, 2021
@errordeveloper errordeveloper merged commit 8d4ec81 into master Oct 7, 2021
@errordeveloper errordeveloper deleted the pr/tklauser/bugtool-mem-usage branch October 7, 2021 15:35
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Amazing, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bugtool Impacts gathering of data for debugging purposes. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

investigate bugtool memory usage
9 participants