docs: added developer docs for pprof/tcmalloc testing #4194

ambuc · 2018-08-17T16:40:24Z

Signed-off-by: James Buckland jbuckland@google.com

Description: Follow-up to #4190, explaining how pprof/tcmalloc can be used for memory consumption performance testing.
Risk Level: None
Testing: None
Docs Changes: Added a developer-facing documentation under bazel/PPROF.md.
Release Notes: N/A

Signed-off-by: James Buckland <jbuckland@google.com>

mattklein123

Thanks for writing this up. FYI, this already works with static linking and enabling the heap profiler via env variables. Static linking forcing is here: https://github.com/envoyproxy/envoy/blob/master/source/common/profiler/profiler.cc#L21

Then you just run Envoy with the appropriate environment variables: https://gperftools.github.io/gperftools/heapprofile.html (e.g., HEAPPROFILE).

What you show here is how to get it working against the dynamic linked tcmalloc which seems fine but would it be better to document how it works out of the box? Up to you.

Signed-off-by: James Buckland <jbuckland@google.com>

mattklein123 · 2018-08-17T19:24:25Z

bazel/PPROF.md

+
+Just compile Envoy as normal:
+
+    $ bazel build //source/exe:envoy-static --copt='-ltcmalloc'


The copt isn't needed. We compile against tcmalloc by default.

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32

Looks awesome! Just a few minor nits

mrice32 · 2018-08-17T21:58:43Z

bazel/PPROF.md

+
+### Compiling a statically-linked Envoy
+
+Just compile Envoy as normal:


nit: phrasing - I would prefer something like: Build the static binary using bazel:

mrice32 · 2018-08-17T22:03:40Z

bazel/PPROF.md

+
+### Running a statically-linked Envoy with `pprof`
+
+And run the binary with a `HEAPPROFILE` env var, like so:


nit: remove And.

Also, can you add a brief explanation of what the HEAPPROFILE environment variable does (It's explained below, so just saying it will be explained in section X would work too)?

mrice32 · 2018-08-17T22:06:19Z

bazel/PPROF.md

+
+### Adding `tcmalloc_dep` to Envoy
+
+To add a `HeapProfiler` breakpoint yourself, add `tcmalloc` as a dependency


nit: maybe add a one sentence explanation (or link) explaining what a heap profiler break point does? Or, you could add a section (or link) at the top that explains the ways you can use the heap profiler.

Also, do heap profiler break points only work/apply in the dynamic linking setup?

I believe the statically linked Envoy profiles everything by default.

dnoe

Thanks always for docs!

dnoe · 2018-08-18T00:16:55Z

bazel/PPROF.md

+    }
+```
+
+Once these changes have been implemented against head, it might make sense to


The usage of "head" here seems a little weird. For example, I bet lots of people would want to use this to test a branch where they are developing a PR, not actually "head". Instead, maybe something like "Once these changes have been made in your working directory"?

Oops, you're right. This was a holdover from when I was enabling profiling against both upstream/master and my branch.

dnoe · 2018-08-18T00:17:33Z

bazel/PPROF.md

+
+### Compiling a dynamically-linked Envoy with `pprof`
+
+The binary must be explicitly built with `tcmalloc` like so:


Did you mean to write -ltcmalloc for this?

(Also, are you sure this is needed even for the dynamic case?)

Good catch -- the inclusion of tcmalloc_dep = 1 is sufficient.

dnoe · 2018-08-18T00:18:29Z

bazel/PPROF.md

+
+### Running a dynamically-linked Envoy with `pprof`
+
+You can then run the binary without any env vars:


Just say "Run the binary without any env vars:"

dnoe · 2018-08-18T00:19:15Z

bazel/PPROF.md

+
+    $ timeout <num_seconds> bazel-bin/source/exe/envoy <options>
+
+During a run, the binary should print something like:


To stdout? stderr?

To stdout by default.

dnoe · 2018-08-18T00:21:06Z

bazel/PPROF.md

+
+*NB:* There is no reason this needs to be titled `main_common_base`: whatever
+flag you supply `HeapProfilerStart` / `HeapProfilerDump` will become the
+filename. Multiple ranges could be tested at the same time with this method.


What does "range" refer to here? The time interval between HeapProfilerStart and HeapProfilerDump?

This should be more explicit about profiling a section of code, not a range of time. I'll fix.

dnoe · 2018-08-18T00:21:20Z

bazel/PPROF.md

+
+# Analyzing with `pprof`
+
+[pprof](https://github.com/google/pprof) can then read these heap files in a


nit: drop "then"

Signed-off-by: James Buckland <jbuckland@google.com>

* origin/master: (34 commits) fuzz: fixes oss-fuzz: 9895 (envoyproxy#4189) docs: added developer docs for pprof/tcmalloc testing (envoyproxy#4194) fix sometimes returns response body to HEAD requests (envoyproxy#3985) admin: fix config dump order (envoyproxy#4197) thrift: allow translation between downstream and upstream protocols (envoyproxy#4136) Use local_info.node() instead of bootstrap.node() whenever possible (envoyproxy#4120) upstream: allow extension protocol options to be used with http filters (envoyproxy#4165) [thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] (envoyproxy#4169) docs: improve gRPC-JSON filter doc (envoyproxy#4184) stats: refactoring MetricImpl without strings (envoyproxy#4190) fuzz: coverage profile generation instructions. (envoyproxy#4193) upstream: rebuild cluster when health check config is changed (envoyproxy#4075) build: use clang-6.0. (envoyproxy#4168) thrift_proxy: introduce header transport (envoyproxy#4082) tcp: allow connection pool callers to store protocol state (envoyproxy#4131) configs: match latest API changes (envoyproxy#4185) Fix wrong mock function name. (envoyproxy#4187) Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) Converting envoy configs to V2 (envoyproxy#2957) Add timestamp to HealthCheckEvent definition (envoyproxy#4119) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>

ambuc added 2 commits August 17, 2018 12:37

First draft of pprof usage doc

1cbcfe1

Signed-off-by: James Buckland <jbuckland@google.com>

Closing quotes in PPROF.md code example

5c0a04f

Signed-off-by: James Buckland <jbuckland@google.com>

mattklein123 reviewed Aug 17, 2018

View reviewed changes

ambuc added 2 commits August 17, 2018 14:39

Include statically-linked pprof too

a94412d

Signed-off-by: James Buckland <jbuckland@google.com>

Stylistic fixes

0d63507

Signed-off-by: James Buckland <jbuckland@google.com>

mattklein123 reviewed Aug 17, 2018

View reviewed changes

Remove unnecessary copt

94e4ff4

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 reviewed Aug 17, 2018

View reviewed changes

dnoe reviewed Aug 18, 2018

View reviewed changes

Various edits

02df9b3

Signed-off-by: James Buckland <jbuckland@google.com>

dnoe approved these changes Aug 20, 2018

View reviewed changes

dnoe merged commit 9058d46 into envoyproxy:master Aug 20, 2018


		Just compile Envoy as normal:

		$ bazel build //source/exe:envoy-static --copt='-ltcmalloc'


		### Compiling a statically-linked Envoy

		Just compile Envoy as normal:


		### Running a statically-linked Envoy with `pprof`

		And run the binary with a `HEAPPROFILE` env var, like so:


		### Adding `tcmalloc_dep` to Envoy

		To add a `HeapProfiler` breakpoint yourself, add `tcmalloc` as a dependency


		### Compiling a dynamically-linked Envoy with `pprof`

		The binary must be explicitly built with `tcmalloc` like so:


		### Running a dynamically-linked Envoy with `pprof`

		You can then run the binary without any env vars:


		$ timeout <num_seconds> bazel-bin/source/exe/envoy <options>

		During a run, the binary should print something like:


		# Analyzing with `pprof`

		[pprof](https://github.com/google/pprof) can then read these heap files in a

docs: added developer docs for pprof/tcmalloc testing #4194

docs: added developer docs for pprof/tcmalloc testing #4194

Uh oh!

Conversation

ambuc commented Aug 17, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!