Revert "Configurable heap memory release rate (envoyproxy#30353)" (en…

…voyproxy#32453) This reverts commit 4994e59. Signed-off-by: Kateryna Nezdolii <[email protected]>
Winbobob · Feb 17, 2024 · 295e925 · 295e925
1 parent f301eeb
commit 295e925
Show file tree

Hide file tree

Showing 8 changed files with 60 additions and 316 deletions.
diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto
@@ -41,7 +41,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 // <config_overview_bootstrap>` for more detail.
 
 // Bootstrap :ref:`configuration overview <config_overview_bootstrap>`.
-// [#next-free-field: 42]
+// [#next-free-field: 41]
 message Bootstrap {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.config.bootstrap.v2.Bootstrap";
@@ -411,9 +411,6 @@ message Bootstrap {
 
   // Optional gRPC async manager config.
   GrpcAsyncClientManagerConfig grpc_async_client_manager_config = 40;
-
-  // Optional configuration for memory allocation manager.
-  MemoryAllocatorManager memory_allocator_manager = 41;
 }
 
 // Administration interface :ref:`operations documentation
@@ -737,14 +734,3 @@ message CustomInlineHeader {
   // The type of the header that is expected to be set as the inline header.
   InlineHeaderType inline_header_type = 2 [(validate.rules).enum = {defined_only: true}];
 }
-
-message MemoryAllocatorManager {
-  // Configures tcmalloc to perform background release of free memory in amount of bytes per ``memory_release_interval`` interval.
-  // If equals to ``0``, no memory release will occur. Defaults to ``0``.
-  uint64 bytes_to_release = 1 [(validate.rules).uint64 = {gte: 1}];
-
-  // Interval in milliseconds for memory releasing. If specified, during every
-  // interval Envoy will try to ``release bytes_to_release`` of free memory back to operating system for reuse.
-  // Defaults to 1000 milliseconds.
-  google.protobuf.Duration memory_release_interval = 2;
-}
diff --git a/source/common/memory/BUILD b/source/common/memory/BUILD
@@ -14,12 +14,7 @@ envoy_cc_library(
     hdrs = ["stats.h"],
     tcmalloc_dep = 1,
     deps = [
-        "//envoy/stats:stats_macros",
-        "//source/common/common:assert_lib",
         "//source/common/common:logger_lib",
-        "//source/common/common:thread_lib",
-        "//source/common/protobuf:utility_lib",
-        "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
     ],
 )
 

diff --git a/source/common/memory/stats.cc b/source/common/memory/stats.cc
@@ -2,155 +2,121 @@
 
 #include <cstdint>
 
-#include "source/common/common/assert.h"
 #include "source/common/common/logger.h"
 
 #if defined(TCMALLOC)
+
 #include "tcmalloc/malloc_extension.h"
-#elif defined(GPERFTOOLS_TCMALLOC)
-#include "gperftools/malloc_extension.h"
-#endif
 
 namespace Envoy {
 namespace Memory {
 
 uint64_t Stats::totalCurrentlyAllocated() {
-#if defined(TCMALLOC)
   return tcmalloc::MallocExtension::GetNumericProperty("generic.current_allocated_bytes")
       .value_or(0);
-#elif defined(GPERFTOOLS_TCMALLOC)
-  size_t value = 0;
-  MallocExtension::instance()->GetNumericProperty("generic.current_allocated_bytes", &value);
-  return value;
-#else
-  return 0;
-#endif
 }
 
 uint64_t Stats::totalCurrentlyReserved() {
-#if defined(TCMALLOC)
   // 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"
+
+namespace Envoy {
+namespace Memory {
+
+uint64_t Stats::totalCurrentlyAllocated() {
+  size_t value = 0;
+  MallocExtension::instance()->GetNumericProperty("generic.current_allocated_bytes", &value);
+  return value;
+}
+
+uint64_t Stats::totalCurrentlyReserved() {
   size_t value = 0;
   MallocExtension::instance()->GetNumericProperty("generic.heap_size", &value);
   return value;
-#else
-  return 0;
-#endif
 }
 
 uint64_t Stats::totalThreadCacheBytes() {
-#if defined(TCMALLOC)
-  return tcmalloc::MallocExtension::GetNumericProperty("tcmalloc.current_total_thread_cache_bytes")
-      .value_or(0);
-#elif defined(GPERFTOOLS_TCMALLOC)
   size_t value = 0;
   MallocExtension::instance()->GetNumericProperty("tcmalloc.current_total_thread_cache_bytes",
                                                   &value);
   return value;
-#else
-  return 0;
-#endif
 }
 
 uint64_t Stats::totalPageHeapFree() {
-#if defined(TCMALLOC)
-  return tcmalloc::MallocExtension::GetNumericProperty("tcmalloc.pageheap_free_bytes").value_or(0);
-#elif defined(GPERFTOOLS_TCMALLOC)
   size_t value = 0;
   MallocExtension::instance()->GetNumericProperty("tcmalloc.pageheap_free_bytes", &value);
   return value;
-#else
-  return 0;
-#endif
 }
 
 uint64_t Stats::totalPageHeapUnmapped() {
-#if defined(TCMALLOC)
-  return tcmalloc::MallocExtension::GetNumericProperty("tcmalloc.pageheap_unmapped_bytes")
-      .value_or(0);
-#elif defined(GPERFTOOLS_TCMALLOC)
   size_t value = 0;
   MallocExtension::instance()->GetNumericProperty("tcmalloc.pageheap_unmapped_bytes", &value);
   return value;
-#else
-  return 0;
-#endif
 }
 
 uint64_t Stats::totalPhysicalBytes() {
-#if defined(TCMALLOC)
-  return tcmalloc::MallocExtension::GetProperties()["generic.physical_memory_used"].value;
-#elif defined(GPERFTOOLS_TCMALLOC)
   size_t value = 0;
   MallocExtension::instance()->GetNumericProperty("generic.total_physical_bytes", &value);
   return value;
-#else
-  return 0;
-#endif
 }
 
 void Stats::dumpStatsToLog() {
-#if defined(TCMALLOC)
-  ENVOY_LOG_MISC(debug, "TCMalloc stats:\n{}", tcmalloc::MallocExtension::GetStats());
-#elif defined(GPERFTOOLS_TCMALLOC)
   constexpr int buffer_size = 100000;
   auto buffer = std::make_unique<char[]>(buffer_size);
   MallocExtension::instance()->GetStats(buffer.get(), buffer_size);
   ENVOY_LOG_MISC(debug, "TCMalloc stats:\n{}", buffer.get());
-#else
-  return;
-#endif
 }
 
-void AllocatorManager::tcmallocRelease() {
-#if defined(TCMALLOC)
-  tcmalloc::MallocExtension::ReleaseMemoryToSystem(bytes_to_release_);
-#elif defined(GPERFTOOLS_TCMALLOC)
-  MallocExtension::instance()->ReleaseToSystem(bytes_to_release_);
+} // namespace Memory
+} // namespace Envoy
+
 #else
-  return;
-#endif
-}
 
-/**
- * Configures tcmalloc release rate from the page heap. If `bytes_to_release_`
- * has been initialized to `0`, no heap memory will be released in background.
- */
-void AllocatorManager::configureBackgroundMemoryRelease(Api::Api& api) {
-  RELEASE_ASSERT(!tcmalloc_thread_, "Invalid state, tcmalloc has already been initialised");
-  if (bytes_to_release_ > 0) {
-    tcmalloc_routine_dispatcher_ = api.allocateDispatcher(std::string(TCMALLOC_ROUTINE_THREAD_ID));
-    memory_release_timer_ = tcmalloc_routine_dispatcher_->createTimer([this]() -> void {
-      const uint64_t unmapped_bytes_before_release = Stats::totalPageHeapUnmapped();
-      tcmallocRelease();
-      const uint64_t unmapped_bytes_after_release = Stats::totalPageHeapUnmapped();
-      if (unmapped_bytes_after_release > unmapped_bytes_before_release) {
-        // Only increment stats if memory was actually released. As tcmalloc releases memory on a
-        // span granularity, during some release rounds there may be no memory released, if during
-        // past round too much memory was released.
-        // https://github.com/google/tcmalloc/blob/master/tcmalloc/tcmalloc.cc#L298
-        allocator_manager_stats_.released_by_timer_.inc();
-      }
-      memory_release_timer_->enableTimer(memory_release_interval_msec_);
-    });
-    tcmalloc_thread_ = api.threadFactory().createThread(
-        [this]() -> void {
-          ENVOY_LOG_MISC(debug, "Started {}", TCMALLOC_ROUTINE_THREAD_ID);
-          memory_release_timer_->enableTimer(memory_release_interval_msec_);
-          tcmalloc_routine_dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit);
-        },
-        Thread::Options{std::string(TCMALLOC_ROUTINE_THREAD_ID)});
-    ENVOY_LOG_MISC(
-        info, fmt::format(
-                  "Configured tcmalloc with background release rate: {} bytes per {} milliseconds",
-                  bytes_to_release_, memory_release_interval_msec_.count()));
-  }
-}
+namespace Envoy {
+namespace Memory {
+
+uint64_t Stats::totalCurrentlyAllocated() { return 0; }
+uint64_t Stats::totalThreadCacheBytes() { return 0; }
+uint64_t Stats::totalCurrentlyReserved() { return 0; }
+uint64_t Stats::totalPageHeapUnmapped() { return 0; }
+uint64_t Stats::totalPageHeapFree() { return 0; }
+uint64_t Stats::totalPhysicalBytes() { return 0; }
+void Stats::dumpStatsToLog() {}
 
 } // namespace Memory
 } // namespace Envoy
+
+#endif // #if defined(TCMALLOC)
diff --git a/source/common/memory/stats.h b/source/common/memory/stats.h
@@ -2,24 +2,9 @@
 
 #include <cstdint>
 
-#include "envoy/config/bootstrap/v3/bootstrap.pb.h"
-#include "envoy/stats/store.h"
-
-#include "source/common/common/thread.h"
-#include "source/common/protobuf/utility.h"
-
 namespace Envoy {
-
-#define MEMORY_ALLOCATOR_MANAGER_STATS(COUNTER) COUNTER(released_by_timer)
-
-struct MemoryAllocatorManagerStats {
-  MEMORY_ALLOCATOR_MANAGER_STATS(GENERATE_COUNTER_STRUCT)
-};
-
 namespace Memory {
 
-constexpr absl::string_view TCMALLOC_ROUTINE_THREAD_ID = "TcmallocProcessBackgroundActions";
-
 /**
  * Runtime stats for process memory usage.
  */
@@ -66,40 +51,5 @@ class Stats {
   static void dumpStatsToLog();
 };
 
-class AllocatorManager {
-public:
-  AllocatorManager(Api::Api& api, Envoy::Stats::Scope& scope,
-                   const envoy::config::bootstrap::v3::MemoryAllocatorManager& config)
-      : bytes_to_release_(config.bytes_to_release()),
-        memory_release_interval_msec_(std::chrono::milliseconds(
-            PROTOBUF_GET_MS_OR_DEFAULT(config, memory_release_interval, 1000))),
-        allocator_manager_stats_(MemoryAllocatorManagerStats{
-            MEMORY_ALLOCATOR_MANAGER_STATS(POOL_COUNTER_PREFIX(scope, "tcmalloc."))}) {
-    configureBackgroundMemoryRelease(api);
-  };
-
-  ~AllocatorManager() {
-    if (tcmalloc_routine_dispatcher_) {
-      tcmalloc_routine_dispatcher_->exit();
-    }
-    if (tcmalloc_thread_) {
-      tcmalloc_thread_->join();
-      tcmalloc_thread_.reset();
-    }
-  }
-
-private:
-  const uint64_t bytes_to_release_;
-  const std::chrono::milliseconds memory_release_interval_msec_;
-  MemoryAllocatorManagerStats allocator_manager_stats_;
-  Thread::ThreadPtr tcmalloc_thread_;
-  Event::DispatcherPtr tcmalloc_routine_dispatcher_;
-  Event::TimerPtr memory_release_timer_;
-  void configureBackgroundMemoryRelease(Api::Api& api);
-  void tcmallocRelease();
-  // Used for testing.
-  friend class AllocatorManagerPeer;
-};
-
 } // namespace Memory
 } // namespace Envoy
diff --git a/source/server/server.cc b/source/server/server.cc
@@ -35,6 +35,7 @@
 #include "source/common/http/codes.h"
 #include "source/common/http/headers.h"
 #include "source/common/local_info/local_info_impl.h"
+#include "source/common/memory/stats.h"
 #include "source/common/network/address_impl.h"
 #include "source/common/network/dns_resolver/dns_factory_util.h"
 #include "source/common/network/socket_interface.h"
@@ -521,9 +522,6 @@ void InstanceBase::initializeOrThrow(Network::Address::InstanceConstSharedPtr lo
                                   server_stats_->dynamic_unknown_fields_,
                                   server_stats_->wip_protos_);
 
-  memory_allocator_ = std::make_unique<Memory::AllocatorManager>(
-      *api_, *stats_store_.rootScope(), bootstrap_.memory_allocator_manager());
-
   initialization_timer_ = std::make_unique<Stats::HistogramCompletableTimespanImpl>(
       server_stats_->initialization_time_ms_, timeSource());
   server_stats_->concurrency_.set(options_.concurrency());

diff --git a/source/server/server.h b/source/server/server.h
@@ -32,7 +32,6 @@
 #include "source/common/grpc/context_impl.h"
 #include "source/common/http/context_impl.h"
 #include "source/common/init/manager_impl.h"
-#include "source/common/memory/stats.h"
 #include "source/common/protobuf/message_validator_impl.h"
 #include "source/common/quic/quic_stat_names.h"
 #include "source/common/router/context_impl.h"
@@ -337,6 +336,7 @@ class InstanceBase : Logger::Loggable<Logger::Id::main>,
       Stage stage, std::function<void()> completion_cb = [] {});
   void onRuntimeReady();
   void onClusterManagerPrimaryInitializationComplete();
+
   using LifecycleNotifierCallbacks = std::list<StageCallback>;
   using LifecycleNotifierCompletionCallbacks = std::list<StageCallbackWithCompletion>;
 
@@ -414,8 +414,8 @@ class InstanceBase : Logger::Loggable<Logger::Id::main>,
   ServerFactoryContextImpl server_contexts_;
   bool enable_reuse_port_default_{false};
   Regex::EnginePtr regex_engine_;
+
   bool stats_flush_in_progress_ : 1;
-  std::unique_ptr<Memory::AllocatorManager> memory_allocator_;
 
   template <class T>
   class LifecycleCallbackHandle : public ServerLifecycleNotifier::Handle, RaiiListElement<T> {

diff --git a/test/common/memory/BUILD b/test/common/memory/BUILD
@@ -14,18 +14,6 @@ envoy_cc_test(
     deps = ["//source/common/memory:stats_lib"],
 )
 
-envoy_cc_test(
-    name = "memory_release_test",
-    srcs = ["memory_release_test.cc"],
-    deps = [
-        "//source/common/event:dispatcher_lib",
-        "//source/common/memory:stats_lib",
-        "//test/common/stats:stat_test_utility_lib",
-        "//test/test_common:simulated_time_system_lib",
-        "//test/test_common:utility_lib",
-    ],
-)
-
 envoy_cc_test(
     name = "heap_shrinker_test",
     srcs = ["heap_shrinker_test.cc"],