From d86b1f29c976c932470d01cfb68a8eb97a79c2ee Mon Sep 17 00:00:00 2001
From: Bikramjeet Vig <bikramjeet@meta.com>
Date: Tue, 30 Apr 2024 09:25:04 -0700
Subject: [PATCH] Disable allocator stats collection by default

Summary:
Allocator stats collection was designed to be a debugging tool and
can be expensive as allocation happens on the hotpath and stats
collection can add a non-trivial amount to it. Therefore we are
disabling it by default.

Additionally this change improves the toString() API for the struct
that accounts for these stats but adding the number of allocations
to it.

Reviewed By: xiaoxmeng

Differential Revision: D56658835

fbshipit-source-id: 3cf55087e441000e7acce179f911afd37965ca2f
---
 velox/common/memory/MemoryAllocator.cpp       | 12 ++++---
 .../memory/tests/MemoryAllocatorTest.cpp      | 32 +++++++++++++++++++
 velox/flag_definitions/flags.cpp              |  2 +-
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/velox/common/memory/MemoryAllocator.cpp b/velox/common/memory/MemoryAllocator.cpp
index 3995b1841929..e4dd46457b3a 100644
--- a/velox/common/memory/MemoryAllocator.cpp
+++ b/velox/common/memory/MemoryAllocator.cpp
@@ -364,15 +364,18 @@ std::string Stats::toString() const {
   std::stringstream out;
   int64_t totalClocks = 0;
   int64_t totalBytes = 0;
+  int64_t totalAllocations = 0;
   for (auto i = 0; i < sizes.size(); ++i) {
     totalClocks += sizes[i].clocks();
     totalBytes += sizes[i].totalBytes;
+    totalAllocations += sizes[i].numAllocations;
   }
   out << fmt::format(
-      "Alloc: {}MB {} Gigaclocks, {}MB advised\n",
+      "Alloc: {}MB {} Gigaclocks {} Allocations, {}MB advised\n",
       totalBytes >> 20,
       totalClocks >> 30,
-      numAdvise >> 8);
+      numAdvise >> 8,
+      totalAllocations);
 
   // Sort the size classes by decreasing clocks.
   std::vector<int32_t> indices(sizes.size());
@@ -386,10 +389,11 @@ std::string Stats::toString() const {
       break;
     }
     out << fmt::format(
-        "Size {}K: {}MB {} Megaclocks\n",
+        "Size {}K: {}MB {} Megaclocks {} Allocations\n",
         sizes[i].size * 4,
         sizes[i].totalBytes >> 20,
-        sizes[i].clocks() >> 20);
+        sizes[i].clocks() >> 20,
+        sizes[i].numAllocations);
   }
   return out.str();
 }
diff --git a/velox/common/memory/tests/MemoryAllocatorTest.cpp b/velox/common/memory/tests/MemoryAllocatorTest.cpp
index 86133a88e2bf..4bfff3f4ffad 100644
--- a/velox/common/memory/tests/MemoryAllocatorTest.cpp
+++ b/velox/common/memory/tests/MemoryAllocatorTest.cpp
@@ -632,10 +632,42 @@ TEST_P(MemoryAllocatorTest, allocationClass2) {
   allocation->clear();
 }
 
+TEST_P(MemoryAllocatorTest, stats) {
+  const std::vector<MachinePageCount>& sizes = instance_->sizeClasses();
+  MachinePageCount capacity = kCapacityPages;
+  for (auto i = 0; i < sizes.size(); ++i) {
+    std::unique_ptr<Allocation> allocation = std::make_unique<Allocation>();
+    auto size = sizes[i];
+    ASSERT_TRUE(allocate(size, *allocation));
+    ASSERT_GT(instance_->numAllocated(), 0);
+    instance_->freeNonContiguous(*allocation);
+    auto stats = instance_->stats();
+    ASSERT_EQ(0, stats.sizes[i].clocks());
+    ASSERT_EQ(stats.sizes[i].totalBytes, 0);
+    ASSERT_EQ(stats.sizes[i].numAllocations, 0);
+  }
+
+  gflags::FlagSaver flagSaver;
+  FLAGS_velox_time_allocations = true;
+  for (auto i = 0; i < sizes.size(); ++i) {
+    std::unique_ptr<Allocation> allocation = std::make_unique<Allocation>();
+    auto size = sizes[i];
+    ASSERT_TRUE(allocate(size, *allocation));
+    ASSERT_GT(instance_->numAllocated(), 0);
+    instance_->freeNonContiguous(*allocation);
+    auto stats = instance_->stats();
+    ASSERT_LT(0, stats.sizes[i].clocks());
+    ASSERT_GE(stats.sizes[i].totalBytes, size * AllocationTraits::kPageSize);
+    ASSERT_GE(stats.sizes[i].numAllocations, 1);
+  }
+}
+
 TEST_P(MemoryAllocatorTest, singleAllocation) {
   if (!useMmap_ && enableReservation_) {
     return;
   }
+  gflags::FlagSaver flagSaver;
+  FLAGS_velox_time_allocations = true;
   const std::vector<MachinePageCount>& sizes = instance_->sizeClasses();
   MachinePageCount capacity = kCapacityPages;
   for (auto i = 0; i < sizes.size(); ++i) {
diff --git a/velox/flag_definitions/flags.cpp b/velox/flag_definitions/flags.cpp
index d25ae44713f1..b26eeb5d4c70 100644
--- a/velox/flag_definitions/flags.cpp
+++ b/velox/flag_definitions/flags.cpp
@@ -30,7 +30,7 @@ DEFINE_int32(
 
 DEFINE_bool(
     velox_time_allocations,
-    true,
+    false,
     "Record time and volume for large allocation/free");
 
 // Used in common/base/VeloxException.cpp