lc trie: fix memory leak.

[PiotrSikora]

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]

cc @rshriram @mattklein123 @brian-pane @ccaraman

[brian-pane]

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]

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]

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

[ggreenway]

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]

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]

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]

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.