-
Notifications
You must be signed in to change notification settings - Fork 5.1k
lc trie: fix memory leak. #3890
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
Conversation
*Risk Level*: Low *Testing*: curl -s 127.0.0.1:<admin_port>/stats | grep server.memory *Docs Changes*: n/a *Release Notes*: n/a Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Thanks for the fix. Given some more time to hack on this, I think we could eliminate the need to pre-allocate such a large buffer in the first place. But for now, this one-line fix should be a big win. |
But this is still going to allocate 16M per listener and then shrink it right? So, if 100 listeners come down the wire, are we going to see memory usage shoot up to 1600Mb and then go down to a smaller number? If so, this will still cause instability (potential oom issues, etc.). |
@rshriram I think that the 100 Trie's will get built serially, so memory usage will balloon by 16MB above what's needed and then shrink down. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to add a test? I don't know that any existing tests are monitoring memory usage, but it would be nice to have a test that makes a simple/small LcTrie and verifies that memory consumed is something reasonable.
Ah its built sequentially? Then this is fine. |
You can validate memory use by looking at tcmalloc stats, e.g. https://github.com/envoyproxy/envoy/blob/master/source/common/memory/stats.cc#L14. |
If you want to file an issue for the test as a followup, I will merge this, as it seems harmless and Istio needs this for a release. |
Risk Level: Low
Testing: curl -s 127.0.0.1:<admin_port>/stats | grep server.memory
Docs Changes: n/a
Release Notes: n/a
Signed-off-by: Piotr Sikora piotrsikora@google.com