[server] Remove name parameter from CreateMemoryOwner (grpc#34479)

arvindbr8 · Oct 18, 2023 · 940e2c3 · 940e2c3
1 parent dd3a355
commit 940e2c3
Show file tree

Hide file tree

Showing 14 changed files with 32 additions and 47 deletions.
diff --git a/src/core/ext/transport/chttp2/server/chttp2_server.cc b/src/core/ext/transport/chttp2/server/chttp2_server.cc
@@ -833,8 +833,7 @@ void Chttp2ServerListener::OnAccept(void* arg, grpc_endpoint* tcp,
       return;
     }
   }
-  auto memory_owner = self->memory_quota_->CreateMemoryOwner(
-      absl::StrCat(grpc_endpoint_get_peer(tcp), ":server_channel"));
+  auto memory_owner = self->memory_quota_->CreateMemoryOwner();
   EventEngine* const event_engine = self->args_.GetObject<EventEngine>();
   auto connection = memory_owner.MakeOrphanable<ActiveConnection>(
       accepting_pollset, acceptor, event_engine, args, std::move(memory_owner));

diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc
@@ -619,8 +619,7 @@ grpc_chttp2_transport::grpc_chttp2_transport(
           grpc_core::Slice::FromCopiedString(grpc_endpoint_get_peer(ep))),
       memory_owner(channel_args.GetObject<grpc_core::ResourceQuota>()
                        ->memory_quota()
-                       ->CreateMemoryOwner(absl::StrCat(
-                           grpc_endpoint_get_peer(ep), ":client_transport"))),
+                       ->CreateMemoryOwner()),
       self_reservation(
           memory_owner.MakeReservation(sizeof(grpc_chttp2_transport))),
       event_engine(

diff --git a/src/core/lib/event_engine/posix_engine/posix_endpoint.cc b/src/core/lib/event_engine/posix_engine/posix_endpoint.cc
@@ -1246,8 +1246,7 @@ PosixEndpointImpl::PosixEndpointImpl(EventHandle* handle,
   GPR_ASSERT(options.resource_quota != nullptr);
   auto peer_addr_string = sock.PeerAddressString();
   mem_quota_ = options.resource_quota->memory_quota();
-  memory_owner_ = mem_quota_->CreateMemoryOwner(
-      peer_addr_string.ok() ? *peer_addr_string : "");
+  memory_owner_ = mem_quota_->CreateMemoryOwner();
   self_reservation_ = memory_owner_.MakeReservation(sizeof(PosixEndpointImpl));
   auto local_address = sock.LocalAddress();
   if (local_address.ok()) {

diff --git a/src/core/lib/iomgr/event_engine_shims/tcp_client.cc b/src/core/lib/iomgr/event_engine_shims/tcp_client.cc
@@ -72,8 +72,7 @@ int64_t event_engine_tcp_client_connect(
       },
       CreateResolvedAddress(*addr), config,
       resource_quota != nullptr
-          ? resource_quota->memory_quota()->CreateMemoryOwner(
-                absl::StrCat("tcp-client:", addr_uri.value()))
+          ? resource_quota->memory_quota()->CreateMemoryOwner()
           : grpc_event_engine::experimental::MemoryAllocator(),
       std::max(grpc_core::Duration::Milliseconds(1),
                deadline - grpc_core::Timestamp::Now()));

diff --git a/src/core/lib/iomgr/tcp_posix.cc b/src/core/lib/iomgr/tcp_posix.cc
@@ -1944,7 +1944,7 @@ grpc_endpoint* grpc_tcp_create(grpc_fd* em_fd,
   tcp->fd = grpc_fd_wrapped_fd(em_fd);
   GPR_ASSERT(options.resource_quota != nullptr);
   tcp->memory_owner =
-      options.resource_quota->memory_quota()->CreateMemoryOwner(peer_string);
+      options.resource_quota->memory_quota()->CreateMemoryOwner();
   tcp->self_reservation = tcp->memory_owner.MakeReservation(sizeof(grpc_tcp));
   grpc_resolved_address resolved_local_addr;
   memset(&resolved_local_addr, 0, sizeof(resolved_local_addr));

diff --git a/src/core/lib/resource_quota/memory_quota.cc b/src/core/lib/resource_quota/memory_quota.cc
@@ -203,8 +203,8 @@ Poll<RefCountedPtr<ReclaimerQueue::Handle>> ReclaimerQueue::PollNext() {
 //
 
 GrpcMemoryAllocatorImpl::GrpcMemoryAllocatorImpl(
-    std::shared_ptr<BasicMemoryQuota> memory_quota, std::string name)
-    : memory_quota_(memory_quota), name_(std::move(name)) {
+    std::shared_ptr<BasicMemoryQuota> memory_quota)
+    : memory_quota_(memory_quota) {
   memory_quota_->Take(
       /*allocator=*/this, taken_bytes_);
   memory_quota_->AddNewAllocator(this);
@@ -314,8 +314,7 @@ void GrpcMemoryAllocatorImpl::MaybeDonateBack() {
                                           std::memory_order_acq_rel,
                                           std::memory_order_acquire)) {
       if (GRPC_TRACE_FLAG_ENABLED(grpc_resource_quota_trace)) {
-        gpr_log(GPR_INFO, "[%p|%s] Early return %" PRIdPTR " bytes", this,
-                name_.c_str(), ret);
+        gpr_log(GPR_INFO, "[%p] Early return %" PRIdPTR " bytes", this, ret);
       }
       GPR_ASSERT(taken_bytes_.fetch_sub(ret, std::memory_order_relaxed) >= ret);
       memory_quota_->Return(ret);
@@ -735,15 +734,19 @@ double PressureTracker::AddSampleAndGetControlValue(double sample) {
 // MemoryQuota
 //
 
-MemoryAllocator MemoryQuota::CreateMemoryAllocator(absl::string_view name) {
-  auto impl = std::make_shared<GrpcMemoryAllocatorImpl>(
-      memory_quota_, absl::StrCat(memory_quota_->name(), "/allocator/", name));
+MemoryAllocator MemoryQuota::CreateMemoryAllocator(
+    GRPC_UNUSED absl::string_view name) {
+  auto impl = std::make_shared<GrpcMemoryAllocatorImpl>(memory_quota_);
   return MemoryAllocator(std::move(impl));
 }
 
-MemoryOwner MemoryQuota::CreateMemoryOwner(absl::string_view name) {
-  auto impl = std::make_shared<GrpcMemoryAllocatorImpl>(
-      memory_quota_, absl::StrCat(memory_quota_->name(), "/owner/", name));
+MemoryOwner MemoryQuota::CreateMemoryOwner() {
+  // Note: we will likely want to add a name or some way to distinguish
+  // between memory owners once resource quota is fully rolled out and we need
+  // full metrics. One thing to note, however, is that manipulating the name
+  // here (e.g. concatenation) can add significant memory increase when many
+  // owners are created.
+  auto impl = std::make_shared<GrpcMemoryAllocatorImpl>(memory_quota_);
   return MemoryOwner(std::move(impl));
 }
 

diff --git a/src/core/lib/resource_quota/memory_quota.h b/src/core/lib/resource_quota/memory_quota.h
@@ -392,7 +392,7 @@ class BasicMemoryQuota final
 class GrpcMemoryAllocatorImpl final : public EventEngineMemoryAllocatorImpl {
  public:
   explicit GrpcMemoryAllocatorImpl(
-      std::shared_ptr<BasicMemoryQuota> memory_quota, std::string name);
+      std::shared_ptr<BasicMemoryQuota> memory_quota);
   ~GrpcMemoryAllocatorImpl() override;
 
   // Reserve bytes from the quota.
@@ -445,9 +445,6 @@ class GrpcMemoryAllocatorImpl final : public EventEngineMemoryAllocatorImpl {
     return memory_quota_->GetPressureInfo();
   }
 
-  // Name of this allocator
-  absl::string_view name() const { return name_; }
-
   size_t GetFreeBytes() const {
     return free_bytes_.load(std::memory_order_relaxed);
   }
@@ -494,9 +491,6 @@ class GrpcMemoryAllocatorImpl final : public EventEngineMemoryAllocatorImpl {
   OrphanablePtr<ReclaimerQueue::Handle>
       reclamation_handles_[kNumReclamationPasses] ABSL_GUARDED_BY(
           reclaimer_mu_);
-
-  // Name of this allocator.
-  std::string name_;
 };
 
 // MemoryOwner is an enhanced MemoryAllocator that can also reclaim memory, and
@@ -531,9 +525,6 @@ class MemoryOwner final : public MemoryAllocator {
     return OrphanablePtr<T>(New<T>(std::forward<Args>(args)...));
   }
 
-  // Name of this object
-  absl::string_view name() const { return impl()->name(); }
-
   // Is this object valid (ie has not been moved out of or reset)
   bool is_valid() const { return impl() != nullptr; }
 
@@ -565,7 +556,7 @@ class MemoryQuota final
   MemoryQuota& operator=(MemoryQuota&&) = default;
 
   MemoryAllocator CreateMemoryAllocator(absl::string_view name) override;
-  MemoryOwner CreateMemoryOwner(absl::string_view name);
+  MemoryOwner CreateMemoryOwner();
 
   // Resize the quota to new_size.
   void SetSize(size_t new_size) { memory_quota_->SetSize(new_size); }

diff --git a/src/core/lib/security/transport/secure_endpoint.cc b/src/core/lib/security/transport/secure_endpoint.cc
@@ -28,7 +28,6 @@
 
 #include "absl/base/thread_annotations.h"
 #include "absl/status/status.h"
-#include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 #include "absl/types/optional.h"
 
@@ -85,11 +84,9 @@ struct secure_endpoint {
                             grpc_core::CSliceRef(leftover_slices[i]));
     }
     grpc_slice_buffer_init(&output_buffer);
-    memory_owner =
-        grpc_core::ResourceQuotaFromChannelArgs(channel_args)
-            ->memory_quota()
-            ->CreateMemoryOwner(absl::StrCat(grpc_endpoint_get_peer(transport),
-                                             ":secure_endpoint"));
+    memory_owner = grpc_core::ResourceQuotaFromChannelArgs(channel_args)
+                       ->memory_quota()
+                       ->CreateMemoryOwner();
     self_reservation = memory_owner.MakeReservation(sizeof(*this));
     if (zero_copy_protector) {
       read_staging_buffer = grpc_empty_slice();

diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc
@@ -77,7 +77,7 @@ Channel::Channel(bool is_client, bool is_promising, std::string target,
       channelz_node_(channel_args.GetObjectRef<channelz::ChannelNode>()),
       allocator_(channel_args.GetObject<ResourceQuota>()
                      ->memory_quota()
-                     ->CreateMemoryOwner(target)),
+                     ->CreateMemoryOwner()),
       target_(std::move(target)),
       channel_stack_(std::move(channel_stack)) {
   // We need to make sure that grpc_shutdown() does not shut things down

diff --git a/test/core/resource_quota/memory_quota_fuzzer.cc b/test/core/resource_quota/memory_quota_fuzzer.cc
@@ -91,10 +91,9 @@ class Fuzzer {
           memory_quotas_.erase(action.quota());
           break;
         case memory_quota_fuzzer::Action::kCreateAllocator:
-          WithQuota(action.quota(), [this, action, i](MemoryQuota* q) {
-            memory_allocators_.emplace(
-                action.allocator(),
-                q->CreateMemoryOwner(absl::StrCat("allocator-step-", i)));
+          WithQuota(action.quota(), [this, action](MemoryQuota* q) {
+            memory_allocators_.emplace(action.allocator(),
+                                       q->CreateMemoryOwner());
           });
           break;
         case memory_quota_fuzzer::Action::kDeleteAllocator:

diff --git a/test/core/resource_quota/memory_quota_stress_test.cc b/test/core/resource_quota/memory_quota_stress_test.cc
@@ -51,8 +51,7 @@ class StressTest {
     std::random_device g;
     std::uniform_int_distribution<size_t> dist(0, num_quotas - 1);
     for (size_t i = 0; i < num_allocators; ++i) {
-      allocators_.emplace_back(quotas_[dist(g)].CreateMemoryOwner(
-          absl::StrCat("allocator[", i, "]")));
+      allocators_.emplace_back(quotas_[dist(g)].CreateMemoryOwner());
     }
   }
 

diff --git a/test/core/resource_quota/memory_quota_test.cc b/test/core/resource_quota/memory_quota_test.cc
@@ -82,7 +82,7 @@ TEST(MemoryQuotaTest, CreateSomeObjectsAndExpectReclamation) {
 
   MemoryQuota memory_quota("foo");
   memory_quota.SetSize(4096);
-  auto memory_allocator = memory_quota.CreateMemoryOwner("bar");
+  auto memory_allocator = memory_quota.CreateMemoryOwner();
   auto object = memory_allocator.MakeUnique<Sized<2048>>();
 
   auto checker1 = CallChecker::Make();
@@ -157,7 +157,7 @@ TEST(MemoryQuotaTest, NoBunchingIfIdle) {
 
   for (size_t i = 0; i < 10000; i++) {
     ExecCtx exec_ctx;
-    auto memory_owner = memory_quota.CreateMemoryOwner("bar");
+    auto memory_owner = memory_quota.CreateMemoryOwner();
     memory_owner.PostReclaimer(
         ReclamationPass::kDestructive,
         [&count_reclaimers_called](absl::optional<ReclamationSweep> sweep) {

diff --git a/test/core/transport/chttp2/flow_control_fuzzer.cc b/test/core/transport/chttp2/flow_control_fuzzer.cc
@@ -134,7 +134,7 @@ class FlowControlFuzzer {
   }
 
   MemoryQuotaRefPtr memory_quota_ = MakeMemoryQuota("fuzzer");
-  MemoryOwner memory_owner_ = memory_quota_->CreateMemoryOwner("owner");
+  MemoryOwner memory_owner_ = memory_quota_->CreateMemoryOwner();
   std::unique_ptr<TransportFlowControl> tfc_;
   absl::optional<uint32_t> queued_initial_window_size_;
   absl::optional<uint32_t> queued_send_max_frame_size_;

diff --git a/test/core/transport/chttp2/flow_control_test.cc b/test/core/transport/chttp2/flow_control_test.cc
@@ -83,7 +83,7 @@ class TransportTargetWindowEstimatesMocker
 class FlowControlTest : public ::testing::Test {
  protected:
   MemoryOwner memory_owner_ = MemoryOwner(
-      ResourceQuota::Default()->memory_quota()->CreateMemoryOwner("test"));
+      ResourceQuota::Default()->memory_quota()->CreateMemoryOwner());
 };
 
 TEST_F(FlowControlTest, NoOp) {