memory: switch to the latest tcmalloc for x86_64 builds #13251

rojkov · 2020-09-24T08:43:11Z

Commit Message: memory: switch to the latest tcmalloc for x86_64 builds
Additional Description: Switch to the new tcmalloc for linux x86_64 builds. All other builds still get the old tcmalloc from gperftools. Switching profiling or the debugalloc feature on will result in linking to the old tcmalloc too. The old tcmalloc can still be enabled for x86_64 builds with --define tcmalloc=gperftools.
Risk Level: High
Testing: run unit tests
Docs Changes: added a note about the new --define tcmalloc=gperftools option to bazel/README.md.
Release Notes: updated the "Minor Behavior Changes" section of version_hstory/current.rst
Fixes #10053

Signed-off-by: Dmitry Rozhkov <[email protected]>

repokitteh-read-only · 2020-09-24T08:43:15Z

CC @envoyproxy/dependency-watchers: FYI only for changes made to (bazel/repository_locations\.bzl)|(api/bazel/repository_locations\.bzl)|(.*/requirements\.txt).

🐱

Caused by: #13251 was opened by rojkov.

see: more, trace.

Signed-off-by: Dmitry Rozhkov <[email protected]>

bazel/repository_locations.bzl

source/common/memory/utils.cc

Signed-off-by: Dmitry Rozhkov <[email protected]>

rojkov · 2020-09-25T15:03:30Z

Hm.. the way how generic.current_allocated_bytes is calculated in the new tcmalloc has changed a bit (per-cpu cache is substructed now). I need more tests to run -> converting to Draft for now.

…ew tcmalloc Signed-off-by: Dmitry Rozhkov <[email protected]>

Signed-off-by: Dmitry Rozhkov <[email protected]>

rojkov · 2020-09-29T06:57:42Z

/retest

repokitteh-read-only · 2020-09-29T06:57:46Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13251 (comment) was created by @rojkov.

see: more, trace.

rojkov · 2020-09-30T06:15:39Z

/retest

repokitteh-read-only · 2020-09-30T06:15:44Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13251 (comment) was created by @rojkov.

see: more, trace.

Signed-off-by: Dmitry Rozhkov <[email protected]>

When Envoy is linked with the new tcmalloc and compiled with gcc it crashes upon assert in tcmalloc because a buffer slice is allocated with `new []`, but deallocated with `delete`. Signed-off-by: Dmitry Rozhkov <[email protected]>

Signed-off-by: Dmitry Rozhkov <[email protected]>

rojkov · 2020-10-06T06:48:15Z

/retest

repokitteh-read-only · 2020-10-06T06:48:20Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13251 (comment) was created by @rojkov.

see: more, trace.

rojkov · 2020-10-06T08:06:00Z

@jmarantz With the new tcmalloc the amount of consumed memory becomes non deterministic and from run to run I see different results of memory_test.consumedBytes() in the same test. Can we relax the tests a bit and avoid the strict equivalence checks?

jmarantz

@jmarantz With the new tcmalloc the amount of consumed memory becomes non deterministic and from run to run I see different results of memory_test.consumedBytes() in the same test. Can we relax the tests a bit and avoid the strict equivalence checks?

Are you sure that deterministic byte-counts are not still available via the API in some way? It looks like maybe you changed the memory-stats APIs to include unmapped bytes?

jmarantz · 2020-10-06T17:48:31Z

test/integration/stats_integration_test.cc

@@ -310,7 +311,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) {
    // https://github.com/envoyproxy/envoy/issues/12209
    // EXPECT_MEMORY_EQ(m_per_cluster, 44949);
  }
-  EXPECT_MEMORY_LE(m_per_cluster, 47500); // Round up to allow platform variations.
+  EXPECT_MEMORY_LE(m_per_cluster, 47700); // Round up to allow platform variations.


this whole test is now gone, once you merge, so you can just keep the edit for the real symbol-tables test below, and drop this one.

jmarantz · 2020-10-06T17:53:44Z

source/common/memory/stats.cc

+namespace Memory {
+
+uint64_t Stats::totalCurrentlyAllocated() {
+  return tcmalloc::MallocExtension::GetNumericProperty("generic.current_allocated_bytes")


maybe for deterministic memory checks we can just use generic.current_allocated_bytes and not include tcmalloc.cpu_free?

We can make 2 APIs if that helps.

I'm also confused by the github UI; it is not showing me the prior impl of this method.

This part (from #if to #elif) is new for tcmalloc. The previous impl is below.

maybe for deterministic memory checks we can just use generic.current_allocated_bytes and not include tcmalloc.cpu_free?

Right, my bad. It shouldn't be included. I'll drop it. Though after the last merge all tests passed for a6d4c42. But the version without tcmalloc.cpu_free passes the release tests too on my local host. Let's see...

I'm also confused by the github UI; it is not showing me the prior impl of this method.

The old version is still there, just wrapped in #if defined(GPERFTOOLS_TCMALLOC). It's needed for non-x86 builds.

Signed-off-by: Dmitry Rozhkov <[email protected]>

lizan

LGTM, thanks. Defer to @jmarantz for the deterministic memory checks and then merge.

rojkov · 2020-10-07T10:02:27Z

Are you sure that deterministic byte-counts are not still available via the API in some way? It looks like maybe you changed the memory-stats APIs to include unmapped bytes?

I did include unmapped bytes to Stats::totalCurrentlyReserved() only. The reason is that the new tcmalloc doesn't include them now for generic.heap_size. See https://github.com/google/tcmalloc/blob/43cb87553037d858d819437e294bd6a4eb0210ae/tcmalloc/tcmalloc.cc#L877 vs https://github.com/gperftools/gperftools/blob/bda3c82e11615ca9e7751d1f3cfb161026ee742a/src/tcmalloc.cc#L717

Signed-off-by: Dmitry Rozhkov <[email protected]>

jmarantz · 2020-10-07T12:12:27Z

test/common/stats/symbol_table_impl_test.cc

  // symbol_table_mem_used:  1726056 (3.9x) -- does not seem to depend on STL sizes.
-  EXPECT_MEMORY_LE(string_mem_used, 7759488);
+  EXPECT_MEMORY_LE(string_mem_used, 7775872);


CI fails on gcc now with:

Expected: (string_mem_used) <= (7775872), actual: 7792256 vs 7775872

I'm guessing this may be related to varying STL versions for std::string across platforms than tcmalloc. I think we can just remove the exact check for string_mem_used. We can keep the test that symbol tables cut the usage by 2/3.

Signed-off-by: Dmitry Rozhkov <[email protected]>

mattklein123

Awesome, thanks!

alyssawilk · 2020-10-08T14:22:27Z

This morning my default build was using clang+llvm-8.0.0 and build failed with "error: 'asm goto' constructs are not supported yet". I updated clang to 9 and I'm good, but based on that and an unsubstantiated google search (http://llvm.1065342.n5.nabble.com/llvm-dev-is-clang9-supporting-asm-goto-td128867.html) I wonder if we need to update the build requirements doc (https://www.envoyproxy.io/docs/envoy/latest/install/building#requirements)

alyssawilk · 2020-10-08T14:32:00Z

for the record, I just restored clang-8 as default and verified it builds with --define tcmalloc=gperftools so ideally we can just update requirements doc to note folks should use that option with older versions of clang

rojkov · 2020-10-08T14:45:40Z

for the record, I just restored clang-8 as default and verified it builds with --define tcmalloc=gperftools so ideally we can just update requirements doc to note folks should use that option with older versions of clang

Right. Thanks for letting know! I'll make a PR to mention it.

Basically for non-x86 builds the current requirement is still sufficient too. Those who build Envoy for embedded very often have to deal with quite outdated toolchains from BSPs they can get from board vendors.

alyssawilk · 2020-10-08T14:48:47Z

I added some comments in #13438 as we're hoping to cut the release today or tomorrow.
LMK what you think?

rojkov · 2020-10-08T15:00:33Z

Great you corrected it already! I commented the PR.

rgs1 · 2020-10-08T21:32:17Z

Hmm this change is triggering this for our build:

14:21:45 warning: unknown warning option '-Wno-attribute-alias'; did you mean '-Wno-attributes'? [-Wunknown-warning-option]
14:21:45 In file included from external/com_github_google_tcmalloc/tcmalloc/static_vars.cc:26:
14:21:45 In file included from external/com_github_google_tcmalloc/tcmalloc/cpu_cache.h:30:
14:21:45 external/com_github_google_tcmalloc/tcmalloc/internal/percpu_tcmalloc.h:255:7: error: 'asm goto' constructs are not supported yet
14:21:45   asm goto(
14:21:45       ^
14:21:45 external/com_github_google_tcmalloc/tcmalloc/internal/percpu_tcmalloc.h:459:25: error: invalid output constraint '=@ccbe' in asm
14:21:45             [underflow] "=@ccbe"(underflow),

rgs1 · 2020-10-08T21:35:20Z

nvm just saw the prev comment

memory: switch to the latest tcmalloc for x86_64 builds

87741cf

Signed-off-by: Dmitry Rozhkov <[email protected]>

fix building docs

c5854c5

Signed-off-by: Dmitry Rozhkov <[email protected]>

lizan self-requested a review September 24, 2020 16:20

lizan self-assigned this Sep 24, 2020

moderation reviewed Sep 24, 2020

View reviewed changes

bazel/repository_locations.bzl Show resolved Hide resolved

lizan reviewed Sep 24, 2020

View reviewed changes

bazel/repository_locations.bzl Outdated Show resolved Hide resolved

source/common/memory/utils.cc Show resolved Hide resolved

rojkov added 3 commits September 25, 2020 12:55

fix windows build

69cdb1a

Signed-off-by: Dmitry Rozhkov <[email protected]>

tweak exact numbers for consumed memory

df9aa9d

Signed-off-by: Dmitry Rozhkov <[email protected]>

address review comments

f14a4d3

Signed-off-by: Dmitry Rozhkov <[email protected]>

rojkov marked this pull request as draft September 25, 2020 15:03

rojkov added 5 commits September 28, 2020 10:59

make calculation on totalCurrentlyAllocated() the same over old and n…

c1485cc

…ew tcmalloc Signed-off-by: Dmitry Rozhkov <[email protected]>

revert tweaking of MemoryWithoutTlsRealSymbolTable case

a01a91c

Signed-off-by: Dmitry Rozhkov <[email protected]>

add project_desc to tcmalloc

1cc9937

Signed-off-by: Dmitry Rozhkov <[email protected]>

Merge remote-tracking branch 'upstream/master' into tcmalloc

0e24124

Signed-off-by: Dmitry Rozhkov <[email protected]>

run check_format.py

956e914

Signed-off-by: Dmitry Rozhkov <[email protected]>

Merge remote-tracking branch 'upstream/master' into tcmalloc

cd1d298

Signed-off-by: Dmitry Rozhkov <[email protected]>

rojkov mentioned this pull request Oct 1, 2020

quiche: fix memory deallocation #13353

Closed

rojkov added 7 commits October 1, 2020 18:05

quiche: fix memory deallocation

f1c5796

When Envoy is linked with the new tcmalloc and compiled with gcc it crashes upon assert in tcmalloc because a buffer slice is allocated with `new []`, but deallocated with `delete`. Signed-off-by: Dmitry Rozhkov <[email protected]>

don't define literal if it is unused

b8c0bc1

Signed-off-by: Dmitry Rozhkov <[email protected]>

Merge remote-tracking branch 'upstream/master' into tcmalloc

8dddc10

Signed-off-by: Dmitry Rozhkov <[email protected]>

move tcmalloc from dataplane to dataplane_core

0753f9e

Signed-off-by: Dmitry Rozhkov <[email protected]>

add tcmalloc to controlplane category

1579935

Signed-off-by: Dmitry Rozhkov <[email protected]>

tweak stats_integration_test.cc

a7480b4

Signed-off-by: Dmitry Rozhkov <[email protected]>

coverage: use gperftools to cover profiler's code

7f01a58

Signed-off-by: Dmitry Rozhkov <[email protected]>

rojkov marked this pull request as ready for review October 6, 2020 08:06

rojkov requested review from alyssawilk and mattklein123 as code owners October 6, 2020 08:06

jmarantz reviewed Oct 6, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into tcmalloc

a6d4c42

Signed-off-by: Dmitry Rozhkov <[email protected]>

lizan reviewed Oct 7, 2020

View reviewed changes

do not include per-cpu caches to totalCurrentlyAllocated()

abc9d8e

Signed-off-by: Dmitry Rozhkov <[email protected]>

jmarantz reviewed Oct 7, 2020

View reviewed changes

remove memory check with absolute value

5c75f1a

Signed-off-by: Dmitry Rozhkov <[email protected]>

jmarantz approved these changes Oct 7, 2020

View reviewed changes

mattklein123 approved these changes Oct 7, 2020

View reviewed changes

mattklein123 merged commit 4c0d2d2 into envoyproxy:master Oct 7, 2020

rojkov deleted the tcmalloc branch October 8, 2020 06:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

memory: switch to the latest tcmalloc for x86_64 builds #13251

memory: switch to the latest tcmalloc for x86_64 builds #13251

rojkov commented Sep 24, 2020

repokitteh-read-only bot commented Sep 24, 2020

rojkov commented Sep 25, 2020

rojkov commented Sep 29, 2020

repokitteh-read-only bot commented Sep 29, 2020

rojkov commented Sep 30, 2020

repokitteh-read-only bot commented Sep 30, 2020

rojkov commented Oct 6, 2020

repokitteh-read-only bot commented Oct 6, 2020

rojkov commented Oct 6, 2020

jmarantz left a comment

jmarantz Oct 6, 2020

jmarantz Oct 6, 2020

lizan Oct 7, 2020

rojkov Oct 7, 2020

lizan left a comment

rojkov commented Oct 7, 2020

jmarantz Oct 7, 2020

mattklein123 left a comment

alyssawilk commented Oct 8, 2020

alyssawilk commented Oct 8, 2020

rojkov commented Oct 8, 2020

alyssawilk commented Oct 8, 2020

rojkov commented Oct 8, 2020

rgs1 commented Oct 8, 2020

rgs1 commented Oct 8, 2020

memory: switch to the latest tcmalloc for x86_64 builds #13251

memory: switch to the latest tcmalloc for x86_64 builds #13251

Conversation

rojkov commented Sep 24, 2020

repokitteh-read-only bot commented Sep 24, 2020

rojkov commented Sep 25, 2020

rojkov commented Sep 29, 2020

repokitteh-read-only bot commented Sep 29, 2020

rojkov commented Sep 30, 2020

repokitteh-read-only bot commented Sep 30, 2020

rojkov commented Oct 6, 2020

repokitteh-read-only bot commented Oct 6, 2020

rojkov commented Oct 6, 2020

jmarantz left a comment

Choose a reason for hiding this comment

jmarantz Oct 6, 2020

Choose a reason for hiding this comment

jmarantz Oct 6, 2020

Choose a reason for hiding this comment

lizan Oct 7, 2020

Choose a reason for hiding this comment

rojkov Oct 7, 2020

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

rojkov commented Oct 7, 2020

jmarantz Oct 7, 2020

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

alyssawilk commented Oct 8, 2020

alyssawilk commented Oct 8, 2020

rojkov commented Oct 8, 2020

alyssawilk commented Oct 8, 2020

rojkov commented Oct 8, 2020

rgs1 commented Oct 8, 2020

rgs1 commented Oct 8, 2020