From 87741cf96e6ef33fdeb676fd5373c152577e248b Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Fri, 18 Sep 2020 07:48:26 +0000 Subject: [PATCH 01/17] memory: switch to the latest tcmalloc for x86_64 builds Signed-off-by: Dmitry Rozhkov --- bazel/BUILD | 31 +++++++++++++ bazel/README.md | 6 ++- bazel/envoy_internal.bzl | 17 +++++-- bazel/envoy_library.bzl | 6 +++ bazel/repositories.bzl | 11 +++++ bazel/repository_locations.bzl | 9 ++++ ci/do_ci.sh | 1 + docs/root/version_history/current.rst | 1 + source/common/memory/stats.cc | 49 ++++++++++++++++++++- source/common/memory/utils.cc | 19 +++++--- source/common/profiler/profiler.h | 4 +- test/common/memory/heap_shrinker_test.cc | 2 +- test/common/stats/stat_test_utility.cc | 2 +- test/common/stats/symbol_table_impl_test.cc | 8 ++-- test/exe/main_common_test.cc | 4 ++ test/integration/stats_integration_test.cc | 3 +- 16 files changed, 152 insertions(+), 21 deletions(-) diff --git a/bazel/BUILD b/bazel/BUILD index 0cdca5a06d7ff..5bdd7b7520cc2 100644 --- a/bazel/BUILD +++ b/bazel/BUILD @@ -155,6 +155,37 @@ config_setting( values = {"define": "tcmalloc=debug"}, ) +config_setting( + name = "gperftools_tcmalloc", + values = {"define": "tcmalloc=gperftools"}, +) + +# As select() can't be nested we need these specialized settings to avoid ambiguity when choosing +# tcmalloc's flavor for x86_64 builds. +config_setting( + name = "disable_tcmalloc_on_linux_x86_64", + values = { + "define": "tcmalloc=disabled", + "cpu": "k8", + }, +) + +config_setting( + name = "gperftools_tcmalloc_on_linux_x86_64", + values = { + "define": "tcmalloc=gperftools", + "cpu": "k8", + }, +) + +config_setting( + name = "debug_tcmalloc_on_linux_x86_64", + values = { + "define": "tcmalloc=debug", + "cpu": "k8", + }, +) + config_setting( name = "disable_signal_trace", values = {"define": "signal_trace=disabled"}, diff --git a/bazel/README.md b/bazel/README.md index 027d125046455..66ff5d5b78e46 100644 --- a/bazel/README.md +++ b/bazel/README.md @@ -607,7 +607,8 @@ The following optional features can be disabled on the Bazel build command-line: * Google C++ gRPC client with `--define google_grpc=disabled` * Backtracing on signals with `--define signal_trace=disabled` * Active stream state dump on signals with `--define signal_trace=disabled` or `--define disable_object_dump_on_signal_trace=disabled` -* tcmalloc with `--define tcmalloc=disabled` +* tcmalloc with `--define tcmalloc=disabled`. Also you can choose Gperftools' implementation of + tcmalloc with `--define tcmalloc=gperftools` which is the default for non-x86 builds. * deprecated features with `--define deprecated_features=disabled` @@ -626,7 +627,8 @@ The following optional features can be enabled on the Bazel build command-line: `--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. * memory-debugging (scribbling over memory after allocation and before freeing) with - `--define tcmalloc=debug`. Note this option cannot be used with FIPS-compliant mode BoringSSL. + `--define tcmalloc=debug`. Note this option cannot be used with FIPS-compliant mode BoringSSL and + tcmalloc is built from the sources of Gperftools. * Default [path normalization](https://github.com/envoyproxy/envoy/issues/6435) with `--define path_normalization_by_default=true`. Note this still could be disable by explicit xDS config. * Manual stamping via VersionInfo with `--define manual_stamp=manual_stamp`. diff --git a/bazel/envoy_internal.bzl b/bazel/envoy_internal.bzl index 10b3448c00eea..296f465965eba 100644 --- a/bazel/envoy_internal.bzl +++ b/bazel/envoy_internal.bzl @@ -57,10 +57,13 @@ def envoy_copts(repository, test = False): "//conditions:default": [], }) + select({ repository + "//bazel:disable_tcmalloc": ["-DABSL_MALLOC_HOOK_MMAP_DISABLE"], - "//conditions:default": ["-DTCMALLOC"], - }) + select({ - repository + "//bazel:debug_tcmalloc": ["-DENVOY_MEMORY_DEBUG_ENABLED=1"], - "//conditions:default": [], + repository + "//bazel:disable_tcmalloc_on_linux_x86_64": ["-DABSL_MALLOC_HOOK_MMAP_DISABLE"], + repository + "//bazel:gperftools_tcmalloc": ["-DGPERFTOOLS_TCMALLOC"], + repository + "//bazel:gperftools_tcmalloc_on_linux_x86_64": ["-DGPERFTOOLS_TCMALLOC"], + repository + "//bazel:debug_tcmalloc": ["-DENVOY_MEMORY_DEBUG_ENABLED=1", "-DGPERFTOOLS_TCMALLOC"], + repository + "//bazel:debug_tcmalloc_on_linux_x86_64": ["-DENVOY_MEMORY_DEBUG_ENABLED=1", "-DGPERFTOOLS_TCMALLOC"], + repository + "//bazel:linux_x86_64": ["-DTCMALLOC"], + "//conditions:default": ["-DGPERFTOOLS_TCMALLOC"], }) + select({ repository + "//bazel:disable_signal_trace": [], "//conditions:default": ["-DENVOY_HANDLE_SIGNALS"], @@ -115,6 +118,12 @@ def envoy_stdlib_deps(): def tcmalloc_external_dep(repository): return select({ repository + "//bazel:disable_tcmalloc": None, + repository + "//bazel:disable_tcmalloc_on_linux_x86_64": None, + repository + "//bazel:debug_tcmalloc": envoy_external_dep_path("gperftools"), + repository + "//bazel:debug_tcmalloc_on_linux_x86_64": envoy_external_dep_path("gperftools"), + repository + "//bazel:gperftools_tcmalloc": envoy_external_dep_path("gperftools"), + repository + "//bazel:gperftools_tcmalloc_on_linux_x86_64": envoy_external_dep_path("gperftools"), + repository + "//bazel:linux_x86_64": envoy_external_dep_path("tcmalloc"), "//conditions:default": envoy_external_dep_path("gperftools"), }) diff --git a/bazel/envoy_library.bzl b/bazel/envoy_library.bzl index 471c8b72eec7e..00fe85672dcb4 100644 --- a/bazel/envoy_library.bzl +++ b/bazel/envoy_library.bzl @@ -20,6 +20,12 @@ load( def tcmalloc_external_deps(repository): return select({ repository + "//bazel:disable_tcmalloc": [], + repository + "//bazel:disable_tcmalloc_on_linux_x86_64": [], + repository + "//bazel:debug_tcmalloc": [envoy_external_dep_path("gperftools")], + repository + "//bazel:debug_tcmalloc_on_linux_x86_64": [envoy_external_dep_path("gperftools")], + repository + "//bazel:gperftools_tcmalloc": [envoy_external_dep_path("gperftools")], + repository + "//bazel:gperftools_tcmalloc_on_linux_x86_64": [envoy_external_dep_path("gperftools")], + repository + "//bazel:linux_x86_64": [envoy_external_dep_path("tcmalloc")], "//conditions:default": [envoy_external_dep_path("gperftools")], }) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index e3d574953d2cd..9982dc73fe0c6 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -173,6 +173,7 @@ def envoy_dependencies(skip_targets = []): _com_github_google_benchmark() _com_github_google_jwt_verify() _com_github_google_libprotobuf_mutator() + _com_github_google_tcmalloc() _com_github_gperftools_gperftools() _com_github_grpc_grpc() _com_github_jbeder_yaml_cpp() @@ -854,6 +855,16 @@ def _com_github_moonjit_moonjit(): actual = "@envoy//bazel/foreign_cc:moonjit", ) +def _com_github_google_tcmalloc(): + _repository_impl( + name = "com_github_google_tcmalloc", + ) + + native.bind( + name = "tcmalloc", + actual = "@com_github_google_tcmalloc//tcmalloc", + ) + def _com_github_gperftools_gperftools(): location = _get_location("com_github_gperftools_gperftools") http_archive( diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 6d5fd2dc6a4e9..333bcd586ecd5 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -206,6 +206,15 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( urls = ["https://github.com/google/libprotobuf-mutator/archive/{version}.tar.gz"], use_category = ["test"], ), + com_github_google_tcmalloc = dict( + project_name = "tcmalloc", + project_url = "https://github.com/google/tcmalloc", + version = "d1311bf409db47c3441d3de6ea07d768c6551dec", + sha256 = "e22444b6544edd81f11c987dd5e482a2e00bbff717badb388779ca57525dad50", + strip_prefix = "tcmalloc-{version}", + urls = ["https://github.com/google/tcmalloc/archive/{version}.tar.gz"], + use_category = ["test"], + ), com_github_gperftools_gperftools = dict( project_name = "gperftools", project_url = "https://github.com/gperftools/gperftools", diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 0ab1bd0f81f22..fff05020a76ae 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -280,6 +280,7 @@ elif [[ "$CI_TARGET" == "bazel.compile_time_options" ]]; then "--define" "path_normalization_by_default=true" "--define" "deprecated_features=disabled" "--define" "use_new_codecs_in_integration_tests=true" + "--define" "tcmalloc=gperftools" "--define" "zlib=ng") ENVOY_STDLIB="${ENVOY_STDLIB:-libstdc++}" diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 042dcfcbc86e9..488182028f719 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -41,6 +41,7 @@ Minor Behavior Changes * logging: nghttp2 log messages no longer appear at trace level unless `ENVOY_NGHTTP2_TRACE` is set in the environment. * lua: changed the response body returned by `httpCall()` API to raw data. Previously, the returned data was string. +* memory: switched to the new `tcmalloc `_ for linux_x86_64 builds. The old `tcmalloc `_ can still be enabled with the `--define tcmalloc=gperftools` option. * router: added transport failure reason to response body when upstream reset happens. After this change, the response body will be of the form `upstream connect error or disconnect/reset before headers. reset reason:{}, transport failure reason:{}`.This behavior may be reverted by setting runtime feature `envoy.reloadable_features.http_transport_failure_reason_in_body` to false. * router: now consumes all retry related headers to prevent them from being propagated to the upstream. This behavior may be reverted by setting runtime feature `envoy.reloadable_features.consume_all_retry_headers` to false. * thrift_proxy: special characters {'\0', '\r', '\n'} will be stripped from thrift headers. diff --git a/source/common/memory/stats.cc b/source/common/memory/stats.cc index 657f9b6b9b080..5b4e97261a4c9 100644 --- a/source/common/memory/stats.cc +++ b/source/common/memory/stats.cc @@ -4,7 +4,52 @@ #include "common/common/logger.h" -#ifdef TCMALLOC +#if defined(TCMALLOC) + +#include "tcmalloc/malloc_extension.h" + +namespace Envoy { +namespace Memory { + +uint64_t Stats::totalCurrentlyAllocated() { + return tcmalloc::MallocExtension::GetNumericProperty("generic.current_allocated_bytes") + .value_or(0); +} + +uint64_t Stats::totalCurrentlyReserved() { + // In Google's tcmalloc the semantics of generic.heap_size has + // changed: it doesn't include unmapped bytes. + return tcmalloc::MallocExtension::GetNumericProperty("generic.heap_size").value_or(0) + + tcmalloc::MallocExtension::GetNumericProperty("tcmalloc.pageheap_unmapped_bytes") + .value_or(0); +} + +uint64_t Stats::totalThreadCacheBytes() { + return tcmalloc::MallocExtension::GetNumericProperty("tcmalloc.current_total_thread_cache_bytes") + .value_or(0); +} + +uint64_t Stats::totalPageHeapFree() { + return tcmalloc::MallocExtension::GetNumericProperty("tcmalloc.pageheap_free_bytes").value_or(0); +} + +uint64_t Stats::totalPageHeapUnmapped() { + return tcmalloc::MallocExtension::GetNumericProperty("tcmalloc.pageheap_unmapped_bytes") + .value_or(0); +} + +uint64_t Stats::totalPhysicalBytes() { + return tcmalloc::MallocExtension::GetProperties()["generic.physical_memory_used"].value; +} + +void Stats::dumpStatsToLog() { + ENVOY_LOG_MISC(debug, "TCMalloc stats:\n{}", tcmalloc::MallocExtension::GetStats()); +} + +} // namespace Memory +} // namespace Envoy + +#elif defined(GPERFTOOLS_TCMALLOC) #include "gperftools/malloc_extension.h" @@ -74,4 +119,4 @@ void Stats::dumpStatsToLog() {} } // namespace Memory } // namespace Envoy -#endif // #ifdef TCMALLOC +#endif // #if defined(TCMALLOC) diff --git a/source/common/memory/utils.cc b/source/common/memory/utils.cc index 2fa957572217a..c6ac4f6c5fe87 100644 --- a/source/common/memory/utils.cc +++ b/source/common/memory/utils.cc @@ -3,15 +3,26 @@ #include "common/common/assert.h" #include "common/memory/stats.h" -#ifdef TCMALLOC +#if defined(TCMALLOC) +#include "tcmalloc/malloc_extension.h" +#elif defined(GPERFTOOLS_TCMALLOC) #include "gperftools/malloc_extension.h" #endif namespace Envoy { namespace Memory { +namespace { +#if defined(TCMALLOC) || defined(GPERFTOOLS_TCMALLOC) +// TODO(zyfjeff): Make max unfreed memory byte configurable +constexpr uint64_t MAX_UNFREED_MEMORY_BYTE = 100 * 1024 * 1024; +#endif +} // namespace + void Utils::releaseFreeMemory() { -#ifdef TCMALLOC +#if defined(TCMALLOC) + tcmalloc::MallocExtension::ReleaseMemoryToSystem(MAX_UNFREED_MEMORY_BYTE); +#elif defined(GPERFTOOLS_TCMALLOC) MallocExtension::instance()->ReleaseFreeMemory(); #endif } @@ -23,9 +34,7 @@ void Utils::releaseFreeMemory() { Ref: https://github.com/envoyproxy/envoy/pull/9471#discussion_r363825985 */ void Utils::tryShrinkHeap() { -#ifdef TCMALLOC - // TODO(zyfjeff): Make max unfreed memory byte configurable - static const uint64_t MAX_UNFREED_MEMORY_BYTE = 100 * 1024 * 1024; +#if defined(TCMALLOC) || defined(GPERFTOOLS_TCMALLOC) auto total_physical_bytes = Stats::totalPhysicalBytes(); auto allocated_size_by_app = Stats::totalCurrentlyAllocated(); diff --git a/source/common/profiler/profiler.h b/source/common/profiler/profiler.h index fdf4b20ee8f96..057ffda6f271d 100644 --- a/source/common/profiler/profiler.h +++ b/source/common/profiler/profiler.h @@ -2,10 +2,10 @@ #include -// Profiling support is provided in the release tcmalloc, but not in the library +// Profiling support is provided in the release tcmalloc of `gperftools`, but not in the library // that supplies the debug tcmalloc. So all the profiling code must be ifdef'd // on PROFILER_AVAILABLE which is dependent on those two settings. -#if defined(TCMALLOC) && !defined(ENVOY_MEMORY_DEBUG_ENABLED) +#if defined(GPERFTOOLS_TCMALLOC) && !defined(ENVOY_MEMORY_DEBUG_ENABLED) #define PROFILER_AVAILABLE #endif diff --git a/test/common/memory/heap_shrinker_test.cc b/test/common/memory/heap_shrinker_test.cc index 9b921847f671a..346a74899ff97 100644 --- a/test/common/memory/heap_shrinker_test.cc +++ b/test/common/memory/heap_shrinker_test.cc @@ -67,7 +67,7 @@ TEST_F(HeapShrinkerTest, ShrinkWhenTriggered) { const uint64_t physical_mem_after_shrink = Stats::totalCurrentlyReserved() - Stats::totalPageHeapUnmapped(); -#ifdef TCMALLOC +#if defined(TCMALLOC) || defined(GPERFTOOLS_TCMALLOC) EXPECT_GE(physical_mem_before_shrink, physical_mem_after_shrink); #else EXPECT_EQ(physical_mem_before_shrink, physical_mem_after_shrink); diff --git a/test/common/stats/stat_test_utility.cc b/test/common/stats/stat_test_utility.cc index 7cdbc08ab4dc5..a47c84d346f17 100644 --- a/test/common/stats/stat_test_utility.cc +++ b/test/common/stats/stat_test_utility.cc @@ -101,7 +101,7 @@ void forEachSampleStat(int num_clusters, std::function } MemoryTest::Mode MemoryTest::mode() { -#if !defined(TCMALLOC) || defined(ENVOY_MEMORY_DEBUG_ENABLED) +#if !(defined(TCMALLOC) || defined(GPERFTOOLS_TCMALLOC)) || defined(ENVOY_MEMORY_DEBUG_ENABLED) // We can only test absolute memory usage if the malloc library is a known // quantity. This decision is centralized here. As the preferred malloc // library for Envoy is TCMALLOC that's what we test for here. If we switch diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc index 5913b47b4be6b..3a1ff2b105019 100644 --- a/test/common/stats/symbol_table_impl_test.cc +++ b/test/common/stats/symbol_table_impl_test.cc @@ -789,11 +789,13 @@ TEST(SymbolTableTest, Memory) { } } - // Make sure we don't regress. Data as of 2019/05/29: - // + // Make sure we don't regress. + // Data as of 2019/05/29: // string_mem_used: 6710912 (libc++), 7759488 (libstdc++). + // Data as of 2020/09/23 (switch to https://github.com/google/tcmalloc): + // string_mem_used: 6710912 (libc++), 7775872 (libstdc++). // 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); EXPECT_MEMORY_LE(symbol_table_mem_used, string_mem_used / 3); EXPECT_MEMORY_EQ(symbol_table_mem_used, 1726056); } diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 39d0486683d7d..50aec7d0250ac 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -177,8 +177,12 @@ TEST_P(MainCommonDeathTest, OutOfMemoryHandler) { ENVOY_LOG_MISC(debug, "p={}", reinterpret_cast(p)); } }(), +#if defined(TCMALLOC) + ".*Unable to allocate.*"); +#else ".*panic: out of memory.*"); #endif +#endif } class AdminRequestTest : public MainCommonTest { diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index a5600628a402d..6143b62b4f11a 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -370,6 +370,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // 2020/08/10 12275 37061 38000 Re-organize tls histogram maps to improve continuity. // 2020/08/11 12202 37061 38500 router: add new retry back-off strategy // 2020/09/11 12973 38993 upstream: predictive prefetch + // 2020/09/23 39244 switch to google tcmalloc // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -390,7 +391,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 37061); } - EXPECT_MEMORY_LE(m_per_cluster, 39000); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 39250); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { From c5854c58ccdb82891722a3107f909c67b872abcf Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Thu, 24 Sep 2020 12:03:50 +0300 Subject: [PATCH 02/17] fix building docs Signed-off-by: Dmitry Rozhkov --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 488182028f719..6280a1346ba9d 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -41,7 +41,7 @@ Minor Behavior Changes * logging: nghttp2 log messages no longer appear at trace level unless `ENVOY_NGHTTP2_TRACE` is set in the environment. * lua: changed the response body returned by `httpCall()` API to raw data. Previously, the returned data was string. -* memory: switched to the new `tcmalloc `_ for linux_x86_64 builds. The old `tcmalloc `_ can still be enabled with the `--define tcmalloc=gperftools` option. +* memory: switched to the `new tcmalloc `_ for linux_x86_64 builds. The `old tcmalloc `_ can still be enabled with the `--define tcmalloc=gperftools` option. * router: added transport failure reason to response body when upstream reset happens. After this change, the response body will be of the form `upstream connect error or disconnect/reset before headers. reset reason:{}, transport failure reason:{}`.This behavior may be reverted by setting runtime feature `envoy.reloadable_features.http_transport_failure_reason_in_body` to false. * router: now consumes all retry related headers to prevent them from being propagated to the upstream. This behavior may be reverted by setting runtime feature `envoy.reloadable_features.consume_all_retry_headers` to false. * thrift_proxy: special characters {'\0', '\r', '\n'} will be stripped from thrift headers. From 69cdb1a50cb3adb476afb8f9ea5e589a433f1f45 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Fri, 25 Sep 2020 12:55:51 +0000 Subject: [PATCH 03/17] fix windows build Signed-off-by: Dmitry Rozhkov --- test/exe/main_common_test.cc | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 50aec7d0250ac..d28d5a26800fd 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -31,6 +31,18 @@ using testing::Return; namespace Envoy { +namespace { + +const std::string& outOfMemoryPattern() { +#if defined(TCMALLOC) + CONSTRUCT_ON_FIRST_USE(std::string, ".*Unable to allocate.*"); +#else + CONSTRUCT_ON_FIRST_USE(std::string, ".*panic: out of memory.*"); +#endif +} + +} // namespace + /** * Captures common functions needed for invoking MainCommon.Maintains * an argv array that is terminated with nullptr. Identifies the config @@ -177,11 +189,7 @@ TEST_P(MainCommonDeathTest, OutOfMemoryHandler) { ENVOY_LOG_MISC(debug, "p={}", reinterpret_cast(p)); } }(), -#if defined(TCMALLOC) - ".*Unable to allocate.*"); -#else - ".*panic: out of memory.*"); -#endif + outOfMemoryPattern()); #endif } From df9aa9d5dc93b02003486b77796e0df9f730fdf2 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Fri, 25 Sep 2020 13:02:22 +0000 Subject: [PATCH 04/17] tweak exact numbers for consumed memory Signed-off-by: Dmitry Rozhkov --- test/common/stats/thread_local_store_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 135c6b424097e..e5ad8c2690b1a 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -1188,7 +1188,7 @@ TEST_F(StatsThreadLocalStoreTestNoFixture, MemoryWithTlsFakeSymbolTable) { TestUtil::MemoryTest memory_test; TestUtil::forEachSampleStat( 100, [this](absl::string_view name) { store_->counterFromString(std::string(name)); }); - EXPECT_MEMORY_EQ(memory_test.consumedBytes(), 1498128); // July 30, 2020 + EXPECT_MEMORY_EQ(memory_test.consumedBytes(), 1498112); // Sep 25, 2020 EXPECT_MEMORY_LE(memory_test.consumedBytes(), 1.6 * million_); } @@ -1198,7 +1198,7 @@ TEST_F(StatsThreadLocalStoreTestNoFixture, MemoryWithoutTlsRealSymbolTable) { TestUtil::MemoryTest memory_test; TestUtil::forEachSampleStat( 100, [this](absl::string_view name) { store_->counterFromString(std::string(name)); }); - EXPECT_MEMORY_EQ(memory_test.consumedBytes(), 688080); // July 2, 2020 + EXPECT_MEMORY_EQ(memory_test.consumedBytes(), 696272); // Sep 25, 2020 EXPECT_MEMORY_LE(memory_test.consumedBytes(), 0.75 * million_); } @@ -1208,7 +1208,7 @@ TEST_F(StatsThreadLocalStoreTestNoFixture, MemoryWithTlsRealSymbolTable) { TestUtil::MemoryTest memory_test; TestUtil::forEachSampleStat( 100, [this](absl::string_view name) { store_->counterFromString(std::string(name)); }); - EXPECT_MEMORY_EQ(memory_test.consumedBytes(), 827632); // July 20, 2020 + EXPECT_MEMORY_EQ(memory_test.consumedBytes(), 827616); // Sep 25, 2020 EXPECT_MEMORY_LE(memory_test.consumedBytes(), 0.9 * million_); } From f14a4d36d49d1fd224e2401afed253d05b0f3298 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Fri, 25 Sep 2020 13:02:44 +0000 Subject: [PATCH 05/17] address review comments Signed-off-by: Dmitry Rozhkov --- bazel/repository_locations.bzl | 4 +++- test/integration/stats_integration_test.cc | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 333bcd586ecd5..6672b9751a4a1 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -209,11 +209,13 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( com_github_google_tcmalloc = dict( project_name = "tcmalloc", project_url = "https://github.com/google/tcmalloc", + # 2020-09-16 version = "d1311bf409db47c3441d3de6ea07d768c6551dec", sha256 = "e22444b6544edd81f11c987dd5e482a2e00bbff717badb388779ca57525dad50", strip_prefix = "tcmalloc-{version}", urls = ["https://github.com/google/tcmalloc/archive/{version}.tar.gz"], - use_category = ["test"], + use_category = ["dataplane"], + cpe = "N/A", ), com_github_gperftools_gperftools = dict( project_name = "gperftools", diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 6143b62b4f11a..18fb04a8e7b0d 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -370,7 +370,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // 2020/08/10 12275 37061 38000 Re-organize tls histogram maps to improve continuity. // 2020/08/11 12202 37061 38500 router: add new retry back-off strategy // 2020/09/11 12973 38993 upstream: predictive prefetch - // 2020/09/23 39244 switch to google tcmalloc + // 2020/09/23 13251 39244 switch to google tcmalloc // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you From c1485cc0d03d01d4871ef94f88914f9ff86c4e94 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Mon, 28 Sep 2020 10:59:21 +0300 Subject: [PATCH 06/17] make calculation on totalCurrentlyAllocated() the same over old and new tcmalloc Signed-off-by: Dmitry Rozhkov --- source/common/memory/stats.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/memory/stats.cc b/source/common/memory/stats.cc index 5b4e97261a4c9..e992d41374476 100644 --- a/source/common/memory/stats.cc +++ b/source/common/memory/stats.cc @@ -13,7 +13,7 @@ namespace Memory { uint64_t Stats::totalCurrentlyAllocated() { return tcmalloc::MallocExtension::GetNumericProperty("generic.current_allocated_bytes") - .value_or(0); + .value_or(0) + tcmalloc::MallocExtension::GetProperties()["tcmalloc.cpu_free"].value; } uint64_t Stats::totalCurrentlyReserved() { From a01a91cc797162cdd93dca96fc664abee946fa0f Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Mon, 28 Sep 2020 11:12:51 +0300 Subject: [PATCH 07/17] revert tweaking of MemoryWithoutTlsRealSymbolTable case Signed-off-by: Dmitry Rozhkov --- test/common/stats/thread_local_store_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index e5ad8c2690b1a..0d28b5a88245d 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -1198,7 +1198,7 @@ TEST_F(StatsThreadLocalStoreTestNoFixture, MemoryWithoutTlsRealSymbolTable) { TestUtil::MemoryTest memory_test; TestUtil::forEachSampleStat( 100, [this](absl::string_view name) { store_->counterFromString(std::string(name)); }); - EXPECT_MEMORY_EQ(memory_test.consumedBytes(), 696272); // Sep 25, 2020 + EXPECT_MEMORY_EQ(memory_test.consumedBytes(), 688080); // July 2, 2020 EXPECT_MEMORY_LE(memory_test.consumedBytes(), 0.75 * million_); } From 1cc99375167b8cbadda9cbd7a8ccbc86f449da22 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Mon, 28 Sep 2020 11:24:34 +0300 Subject: [PATCH 08/17] add project_desc to tcmalloc Signed-off-by: Dmitry Rozhkov --- bazel/repository_locations.bzl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 6672b9751a4a1..4d2e25adc4b2f 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -208,13 +208,14 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( ), com_github_google_tcmalloc = dict( project_name = "tcmalloc", + project_desc = "Fast, multi-threaded malloc implementation", project_url = "https://github.com/google/tcmalloc", - # 2020-09-16 version = "d1311bf409db47c3441d3de6ea07d768c6551dec", sha256 = "e22444b6544edd81f11c987dd5e482a2e00bbff717badb388779ca57525dad50", strip_prefix = "tcmalloc-{version}", urls = ["https://github.com/google/tcmalloc/archive/{version}.tar.gz"], use_category = ["dataplane"], + last_updated = "2020-09-16", cpe = "N/A", ), com_github_gperftools_gperftools = dict( From 956e9141eb9bcb0487903f29b36b62f218ae741a Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Mon, 28 Sep 2020 11:55:55 +0300 Subject: [PATCH 09/17] run check_format.py Signed-off-by: Dmitry Rozhkov --- source/common/memory/stats.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/memory/stats.cc b/source/common/memory/stats.cc index e992d41374476..e5333b1da3639 100644 --- a/source/common/memory/stats.cc +++ b/source/common/memory/stats.cc @@ -13,7 +13,8 @@ namespace Memory { uint64_t Stats::totalCurrentlyAllocated() { return tcmalloc::MallocExtension::GetNumericProperty("generic.current_allocated_bytes") - .value_or(0) + tcmalloc::MallocExtension::GetProperties()["tcmalloc.cpu_free"].value; + .value_or(0) + + tcmalloc::MallocExtension::GetProperties()["tcmalloc.cpu_free"].value; } uint64_t Stats::totalCurrentlyReserved() { From f1c5796b56288f447caa7db615b18e364c494886 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Thu, 1 Oct 2020 14:44:15 +0300 Subject: [PATCH 10/17] quiche: fix memory deallocation 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 --- .../quic_listeners/quiche/platform/quic_mem_slice_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/quic_listeners/quiche/platform/quic_mem_slice_impl.cc b/source/extensions/quic_listeners/quiche/platform/quic_mem_slice_impl.cc index c75f99c0bafbe..903ee1332d04e 100644 --- a/source/extensions/quic_listeners/quiche/platform/quic_mem_slice_impl.cc +++ b/source/extensions/quic_listeners/quiche/platform/quic_mem_slice_impl.cc @@ -16,7 +16,7 @@ QuicMemSliceImpl::QuicMemSliceImpl(QuicUniqueBufferPtr buffer, size_t length) : fragment_(std::make_unique( buffer.release(), length, [](const void* p, size_t, const Envoy::Buffer::BufferFragmentImpl*) { - delete static_cast(p); + delete[] static_cast(p); })) { single_slice_buffer_.addBufferFragment(*fragment_); ASSERT(this->length() == length); From b8c0bc11638acc12b9056eec83392cc4ede561c9 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Wed, 30 Sep 2020 17:59:25 +0300 Subject: [PATCH 11/17] don't define literal if it is unused Signed-off-by: Dmitry Rozhkov --- test/exe/main_common_test.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index d28d5a26800fd..000142b7ff2ea 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -33,6 +33,10 @@ namespace Envoy { namespace { +#if !(defined(__clang_analyzer__) || \ + (defined(__has_feature) && \ + (__has_feature(thread_sanitizer) || __has_feature(address_sanitizer) || \ + __has_feature(memory_sanitizer)))) const std::string& outOfMemoryPattern() { #if defined(TCMALLOC) CONSTRUCT_ON_FIRST_USE(std::string, ".*Unable to allocate.*"); @@ -40,6 +44,7 @@ const std::string& outOfMemoryPattern() { CONSTRUCT_ON_FIRST_USE(std::string, ".*panic: out of memory.*"); #endif } +#endif } // namespace From 0753f9e436be88658af4c523f87fc3ba41dc7e1e Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Fri, 2 Oct 2020 09:34:15 +0300 Subject: [PATCH 12/17] move tcmalloc from dataplane to dataplane_core Signed-off-by: Dmitry Rozhkov --- bazel/repository_locations.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 0585a3801e12c..33f0170449a8f 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -249,7 +249,7 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( sha256 = "e22444b6544edd81f11c987dd5e482a2e00bbff717badb388779ca57525dad50", strip_prefix = "tcmalloc-{version}", urls = ["https://github.com/google/tcmalloc/archive/{version}.tar.gz"], - use_category = ["dataplane"], + use_category = ["dataplane_core"], last_updated = "2020-09-16", cpe = "N/A", ), From 1579935fd34bb5ded0b03394de9ff2a2c0278a79 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Fri, 2 Oct 2020 09:49:42 +0300 Subject: [PATCH 13/17] add tcmalloc to controlplane category Signed-off-by: Dmitry Rozhkov --- bazel/repository_locations.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 33f0170449a8f..95e1d8a20dda5 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -249,7 +249,7 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( sha256 = "e22444b6544edd81f11c987dd5e482a2e00bbff717badb388779ca57525dad50", strip_prefix = "tcmalloc-{version}", urls = ["https://github.com/google/tcmalloc/archive/{version}.tar.gz"], - use_category = ["dataplane_core"], + use_category = ["dataplane_core", "controlplane"], last_updated = "2020-09-16", cpe = "N/A", ), From a7480b480d5c8bb5942d4121b7e42400552e2cf3 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Fri, 2 Oct 2020 11:54:38 +0300 Subject: [PATCH 14/17] tweak stats_integration_test.cc Signed-off-by: Dmitry Rozhkov --- test/integration/stats_integration_test.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 18fb04a8e7b0d..dad5d4c34ad6e 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -290,6 +290,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // 2020/08/10 12275 44949 46000 Re-organize tls histogram maps to improve continuity. // 2020/08/11 12202 44949 46500 router: add new retry back-off strategy // 2020/09/11 12973 47500 upstream: predictive prefetch + // 2020/10/02 13251 47602 47700 switch to the latest tcmalloc for x86_64 builds // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -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. } TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { @@ -370,7 +371,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // 2020/08/10 12275 37061 38000 Re-organize tls histogram maps to improve continuity. // 2020/08/11 12202 37061 38500 router: add new retry back-off strategy // 2020/09/11 12973 38993 upstream: predictive prefetch - // 2020/09/23 13251 39244 switch to google tcmalloc + // 2020/10/02 13251 39326 switch to google tcmalloc // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -391,7 +392,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 37061); } - EXPECT_MEMORY_LE(m_per_cluster, 39250); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 39350); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { From 7f01a5813f8469136492e42a5cc24efd69fa6616 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Mon, 5 Oct 2020 18:45:38 +0300 Subject: [PATCH 15/17] coverage: use gperftools to cover profiler's code Signed-off-by: Dmitry Rozhkov --- ci/do_ci.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 3ad93f09593f3..c5c534a9c9ef6 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -329,7 +329,8 @@ elif [[ "$CI_TARGET" == "bazel.coverage" || "$CI_TARGET" == "bazel.fuzz_coverage [[ "$CI_TARGET" == "bazel.fuzz_coverage" ]] && export FUZZ_COVERAGE=true - BAZEL_BUILD_OPTIONS="${BAZEL_BUILD_OPTIONS[*]}" test/run_envoy_bazel_coverage.sh "${COVERAGE_TEST_TARGETS[@]}" + # We use custom BAZEL_BUILD_OPTIONS here to cover profiler's code. + BAZEL_BUILD_OPTIONS="${BAZEL_BUILD_OPTIONS[*]} --define tcmalloc=gperftools" test/run_envoy_bazel_coverage.sh "${COVERAGE_TEST_TARGETS[@]}" collect_build_profile coverage exit 0 elif [[ "$CI_TARGET" == "bazel.clang_tidy" ]]; then From abc9d8e4743fee345afc71b6b70733b8c9152aa1 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Wed, 7 Oct 2020 11:55:18 +0300 Subject: [PATCH 16/17] do not include per-cpu caches to totalCurrentlyAllocated() Signed-off-by: Dmitry Rozhkov --- source/common/memory/stats.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/common/memory/stats.cc b/source/common/memory/stats.cc index e5333b1da3639..5b4e97261a4c9 100644 --- a/source/common/memory/stats.cc +++ b/source/common/memory/stats.cc @@ -13,8 +13,7 @@ namespace Memory { uint64_t Stats::totalCurrentlyAllocated() { return tcmalloc::MallocExtension::GetNumericProperty("generic.current_allocated_bytes") - .value_or(0) + - tcmalloc::MallocExtension::GetProperties()["tcmalloc.cpu_free"].value; + .value_or(0); } uint64_t Stats::totalCurrentlyReserved() { From 5c75f1a4734e5aec436903d517b4d5b0a34b23c9 Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Wed, 7 Oct 2020 16:25:05 +0300 Subject: [PATCH 17/17] remove memory check with absolute value Signed-off-by: Dmitry Rozhkov --- test/common/stats/symbol_table_impl_test.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc index 1b5f05a266ec8..5ac09db06a4ba 100644 --- a/test/common/stats/symbol_table_impl_test.cc +++ b/test/common/stats/symbol_table_impl_test.cc @@ -734,11 +734,7 @@ TEST(SymbolTableTest, Memory) { // Make sure we don't regress. // Data as of 2019/05/29: - // string_mem_used: 6710912 (libc++), 7759488 (libstdc++). - // Data as of 2020/09/23 (switch to https://github.com/google/tcmalloc): - // string_mem_used: 6710912 (libc++), 7775872 (libstdc++). // symbol_table_mem_used: 1726056 (3.9x) -- does not seem to depend on STL sizes. - EXPECT_MEMORY_LE(string_mem_used, 7775872); EXPECT_MEMORY_LE(symbol_table_mem_used, string_mem_used / 3); EXPECT_MEMORY_EQ(symbol_table_mem_used, 1726056); }