Skip to content

Conversation

PiotrSikora
Copy link
Contributor

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

*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>
@PiotrSikora
Copy link
Contributor Author

PiotrSikora commented Jul 18, 2018

@brian-pane
Copy link
Contributor

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.

@rshriram
Copy link
Member

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.).

@ggreenway
Copy link
Member

@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.

Copy link
Member

@ggreenway ggreenway left a 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.

@rshriram
Copy link
Member

Ah its built sequentially? Then this is fine.
So I validated the this branch against my local setup. The RSS was back at at normal levels.. the heap profiles also stayed at the normal (pre bug-introduction) levels.

@htuch
Copy link
Member

htuch commented Jul 18, 2018

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.

@htuch
Copy link
Member

htuch commented Jul 18, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants