debug: Link in the debug version of tcmalloc for debug builds. by jmarantz · Pull Request #5424 · envoyproxy/envoy

jmarantz · 2018-12-26T14:20:40Z

Description: Use the debug version of tcmalloc when compiling for debug when --define=tcmalloc=debug is passed to bazel.
Risk Level: low: with this when we debug we are using a different allocator
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
*Fixes #Issue: #5423

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

This seems like a good idea to me if we can get it passing CI

mattklein123 · 2018-12-26T17:10:43Z

bazel/envoy_build_system.bzl

               repository + "//bazel:opt_build": [] if test else ["-ggdb3"],
               repository + "//bazel:fastbuild_build": [],
-               repository + "//bazel:dbg_build": ["-ggdb3"],
+               repository + "//bazel:dbg_build": ["-ggdb3", "-DDEBUG_TCMALLOC"],


why not fastbuild also?

fastbuild sgtm.

Passing CI will be the next hurdle; for some reason the tsan build worked locally on my machine so I must not be using the same options as there are in CI. Will investigate.

jmarantz · 2018-12-27T15:57:47Z

I ran into trouble solving this due to my lack of understanding of bazel functions. Seeking stackoverflow help: https://stackoverflow.com/questions/53947289/bazel-select-help-configuring-tcmalloc-debug

Signed-off-by: Joshua Marantz <jmarantz@google.com>

ggreenway · 2019-01-02T18:44:24Z

I'm strongly in favor of this, if you can work out the bazel incantations.

jmarantz · 2019-01-02T18:49:43Z

I haven't gotten any traction on the stackoverflow question I linked above, but I'm hopeful @htuch will be able to offer some suggestions when he's back online.

I'm pretty sure I can get my alternate approach in #5450 working if I can't do this one, but this seems better as it depends directly on code (https://github.com/gperftools/gperftools/blob/master/src/debugallocation.cc) authored by https://en.wikipedia.org/wiki/Urs_H%C3%B6lzle himself :)

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-01-04T22:35:58Z

closing in favor of #5450 which I've been able to get to work.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-01-06T16:01:38Z

@ggreenway @PiotrSikora let's review this PR instead :)

Given that in #5450 I was forcing the user to explicitly configure memory debugging, we might as well use the memory debugging built into tcmalloc, which seems to work better with protobuf's inconsistent operator new/delete overrides.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

test/common/memory/debug_test.cc

…ete. Signed-off-by: Joshua Marantz <jmarantz@google.com>

PiotrSikora · 2019-01-06T17:07:46Z

Note: since this is now guarded by --define debug_memory=enabled, there is no coverage for it on CI. Consider adding it to the bazel.compile_time_options target.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

PiotrSikora · 2019-01-06T17:31:34Z

Sigh, sorry... bazel.compile_time_options uses --define tcmalloc=disabled, so we can't add it there... Perhaps add it to bazel.ipv6_tests or bazel.tsan (though I'm not sure if it's going to survive use-after-free test)?

PiotrSikora · 2019-01-06T17:32:21Z

...or wait, it doesn't (but perhaps it should???).

jmarantz · 2019-01-06T17:33:12Z

I think tcmalloc=disabled is well-covered by tsan, asan, & mac, so it's OK?

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch

Looks good, a few minor comments.

bazel/target_recipes.bzl

source/common/profiler/profiler.h

test/common/memory/debug_test.cc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

bazel/BUILD

test/common/memory/debug_test.cc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

PiotrSikora

LGTM, but you need to rebase.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

PiotrSikora

Nice!

mattklein123

This is really cool. Is there a reason we decided not to do this by default on dbg/fastbuild builds? It seems like this would be nice to have?

mattklein123

/wait

bazel/BUILD

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-01-08T03:58:46Z

I added doc. I am in favor of using it in fastbuild and dbg compilation modes, but maybe we can just add the capability now and get some community buy-in before switching the default?

PiotrSikora · 2019-01-08T04:39:25Z

@mattklein123 I've asked to make memory scribbling optional in #5450, since both memory overhead (2-4x) and runtime overhead (educated guess) are quite significant, and would render debug build almost unusable.

PiotrSikora · 2019-01-08T04:41:14Z

bazel/README.md

 * ASSERT() can be configured to log failures and increment a stat counter in a release build with
  `--define log_debug_assert_in_release=enabled`. The default behavior is to compile debug assertions out of
  release builds so that the condition is not evaluated. This option has no effect in debug builds.
+* tcmalloc can be disabled with `--define tcmalloc=disabled`


Disabling is in section above (though, I'd argue that we could merge both).

Moved tcmalloc=disabled to disabling section. Merging can be left for a different PR.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-01-08T17:23:13Z

@PiotrSikora I ran all the tests with 4 different configs (fastbuild vs debug, tcmalloc=debug vs default) on my workstation. By far the greatest overhead for running tests is compiling and I think especially linking them (30-60 minutes for debug builds). After that it takes about 1.5 to 2 minutes to run all 366 tests independent of mode. An example command-line is:

bazel test --cache_test_results=no --compilation_mode=dbg //test/... --define=tcmalloc=debug

The main concern I have with running tests only under tcmalloc=debug is that it is not the same as production. However we have bazel.release in CI so I think we're OK.

Obviously it doesn't make sense to load-test or perf-test with memory debugging.

Per comment thread, the test works, shows what the system does, and we can always delete it if it causes trouble with sanitizers.

…proxy#5424) * debug: Use the debug version of tcmalloc when compiling for debug when --define=tcmalloc=debug is passed to bazel. Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

debug: Link in the debug version of tcmalloc for debug builds.

1ed5aba

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 reviewed Dec 26, 2018

View reviewed changes

mattklein123 self-assigned this Dec 26, 2018

mattklein123 added the waiting label Dec 26, 2018

Merge branch 'master' into debug-malloc

49f610c

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz mentioned this pull request Jan 2, 2019

memory: Add memory-debug scribbling when tcmalloc is disabled and not compiled for optimization #5450

Closed

Merge branch 'master' into debug-malloc

93d0bc3

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz closed this Jan 4, 2019

jmarantz added 3 commits January 6, 2019 10:38

revive tcmalloc-debug approach, requiring more explicit config.

1aa4196

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into debug-malloc

8ff4d3a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove superflous define.

dd61c91

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz reopened this Jan 6, 2019

repokitteh-read-only bot removed the waiting label Jan 6, 2019

jmarantz changed the title ~~RFC debug: Link in the debug version of tcmalloc for debug builds.~~ debug: Link in the debug version of tcmalloc for debug builds. Jan 6, 2019

jmarantz added 2 commits January 6, 2019 11:20

Fix #define name to disable profiling.

301ce01

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Fix another stray old #define.

c7cd6a1

Signed-off-by: Joshua Marantz <jmarantz@google.com>

PiotrSikora reviewed Jan 6, 2019

View reviewed changes

test/common/memory/debug_test.cc Show resolved Hide resolved

Add dubious but -- at this point -- functional test for ScribbleOnDel…

632652c

…ete. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Add memory-debugging to the compile-time options.

d882947

Signed-off-by: Joshua Marantz <jmarantz@google.com>

include profiler.h to get the ifdef.

c6e72e3

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch previously requested changes Jan 7, 2019

View reviewed changes

bazel/target_recipes.bzl Show resolved Hide resolved

source/common/profiler/profiler.h Show resolved Hide resolved

test/common/memory/debug_test.cc Show resolved Hide resolved

Add comment explaining profiling dependency and #define.

98e7d95

Signed-off-by: Joshua Marantz <jmarantz@google.com>

PiotrSikora reviewed Jan 7, 2019

View reviewed changes

bazel/BUILD Outdated Show resolved Hide resolved

test/common/memory/debug_test.cc Show resolved Hide resolved

Convert debug_memory=enable to tcmalloc=debug, etc.

ee8af23

Signed-off-by: Joshua Marantz <jmarantz@google.com>

PiotrSikora previously approved these changes Jan 8, 2019

View reviewed changes

Merge branch 'master' into debug-malloc

ae5b1ec

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz dismissed PiotrSikora’s stale review via ae5b1ec January 8, 2019 01:38

PiotrSikora previously approved these changes Jan 8, 2019

View reviewed changes

mattklein123 reviewed Jan 8, 2019

View reviewed changes

mattklein123 requested changes Jan 8, 2019

View reviewed changes

bazel/BUILD Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jan 8, 2019

Document the tcmalloc bazel command options.

b5982e6

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz dismissed PiotrSikora’s stale review via b5982e6 January 8, 2019 03:53

repokitteh-read-only bot removed the waiting label Jan 8, 2019

PiotrSikora reviewed Jan 8, 2019

View reviewed changes

jmarantz added 2 commits January 8, 2019 11:59

Added tcmalloc=disabled to the disabling section.

f6eacf0

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove tcmalloc=disabled from enabling section.

80cb279

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 approved these changes Jan 8, 2019

View reviewed changes

jmarantz merged commit 8e23032 into envoyproxy:master Jan 8, 2019

jmarantz deleted the debug-malloc branch January 8, 2019 19:10

lizan mentioned this pull request Feb 22, 2019

build: debug built tcmalloc crash tests #6033

Closed

Conversation

jmarantz commented Dec 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 26, 2018

Choose a reason for hiding this comment

Uh oh!

jmarantz Dec 26, 2018

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Dec 27, 2018

Uh oh!

ggreenway commented Jan 2, 2019

Uh oh!

jmarantz commented Jan 2, 2019

Uh oh!

jmarantz commented Jan 4, 2019

Uh oh!

jmarantz commented Jan 6, 2019

Uh oh!

Uh oh!

PiotrSikora commented Jan 6, 2019

Uh oh!

PiotrSikora commented Jan 6, 2019

Uh oh!

PiotrSikora commented Jan 6, 2019

Uh oh!

jmarantz commented Jan 6, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz commented Jan 8, 2019

Uh oh!

PiotrSikora commented Jan 8, 2019

Uh oh!

PiotrSikora Jan 8, 2019

Choose a reason for hiding this comment

Uh oh!

jmarantz Jan 8, 2019

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Jan 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jmarantz commented Dec 26, 2018 •

edited

Loading