kvstore: share etcd client logger to reduce memory usage #26485
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
giorio94
added
kind/performance
|
/test |
giorio94
force-pushed
the
mio/etcd-logger
branch
from
cbfc728
to
4c5d60f
Compare
rolinh
approved these changes
Jun 26, 2023
Currently, the etcd client logger is not explicitly configured when a new etcd client is initialized, which means that a new one gets created for every client. Yet, this comes with a significant memory cost, which can be avoided explicitly initializing and configuring a shared logger. This change mimics a similar one implemented for the kubernetes apiserver: kubernetes/kubernetes#111477 The following reports the top 3 nodes from the heap pprof profile of an idle process after initializing 1K etcd clients through `kvstore.NewClient`. * Without the patch: Showing nodes accounting for 268.62MB, 88.75% of 302.68MB total Dropped 124 nodes (cum <= 1.51MB) Showing top 3 nodes out of 60 flat flat% sum% cum cum% 224.33MB 74.11% 74.11% 224.33MB 74.11% go.uber.org/zap/zapcore.newCounters 32.45MB 10.72% 84.83% 32.45MB 10.72% google.golang.org/grpc/internal/transport.newBufWriter 11.85MB 3.91% 88.75% 11.85MB 3.91% bufio.NewReaderSize * With the patch: Showing nodes accounting for 65.20MB, 69.19% of 94.23MB total Showing top 3 nodes out of 137 flat flat% sum% cum cum% 43.62MB 46.29% 46.29% 43.62MB 46.29% google.golang.org/grpc/internal/transport.newBufWriter 19.08MB 20.25% 66.54% 19.08MB 20.25% bufio.NewReaderSize 2.50MB 2.65% 69.19% 3MB 3.19% time.NewTimer Co-authored-by: yougjiahe <yongjiahe@tuputech.com> Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
giorio94
force-pushed
the
mio/etcd-logger
branch
from
4c5d60f
to
e515761
Compare
|
Rebased onto main to pick the latest CI changes. |
|
/test |
|
Runtime Test (privileged) hit #26479. Manually rerunning. |
|
The PR has been approved by two reviewers (@marseel covered the kvstore-owned files), and the CI is green. Marking as ready-to-merge so that we can still include it in v1.14 without the need for backports. |
giorio94
added
the
ready-to-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the etcd client logger is not explicitly configured when a new etcd client is initialized, which means that a new one gets created for every client. Yet, this comes with a significant memory cost, which can be avoided explicitly initializing and configuring a shared logger. This change mimics a similar one implemented for the kubernetes apiserver: kubernetes/kubernetes#111477
The following reports the top 3 nodes from the heap pprof profile of an idle process after initializing 1K etcd clients through
kvstore.NewClient.Without the patch:
Showing nodes accounting for 268.62MB, 88.75% of 302.68MB total Dropped 124 nodes (cum <= 1.51MB)
Showing top 3 nodes out of 60
flat flat% sum% cum cum%
224.33MB 74.11% 74.11% 224.33MB 74.11% go.uber.org/zap/zapcore.newCounters
32.45MB 10.72% 84.83% 32.45MB 10.72% google.golang.org/grpc/internal/transport.newBufWriter
11.85MB 3.91% 88.75% 11.85MB 3.91% bufio.NewReaderSize
With the patch:
Showing nodes accounting for 65.20MB, 69.19% of 94.23MB total Showing top 3 nodes out of 137
flat flat% sum% cum cum%
43.62MB 46.29% 46.29% 43.62MB 46.29% google.golang.org/grpc/internal/transport.newBufWriter
19.08MB 20.25% 66.54% 19.08MB 20.25% bufio.NewReaderSize
2.50MB 2.65% 69.19% 3MB 3.19% time.NewTimer
This change was initially proposed by @WingkaiHo as part of #22263