Guard TimeSeriesTracker against non-positive clocks and align integer types.

tcmalloc/allocation_sampling.h

@@ -15,7 +15,6 @@
 #ifndef TCMALLOC_ALLOCATION_SAMPLING_H_
 #define TCMALLOC_ALLOCATION_SAMPLING_H_
 
-#include <algorithm>
 #include <cstddef>
 #include <memory>
 #include <optional>
@@ -183,18 +182,15 @@ ABSL_ATTRIBUTE_NOINLINE sized_ptr_t SampleifyAllocation(
       case MemoryTag::kSampled:
       case MemoryTag::kSampledP1:
       case MemoryTag::kCold: {
+        // TODO(b/540945006): Reconsider whether to skip the first page.
         const uintptr_t hardware_page_size = GetPageSize();
-        const size_t allocated_size_rounded =
-            (stack_trace.allocated_size + hardware_page_size - 1) &
-            ~(hardware_page_size - 1);
-        const size_t limit =
-            std::min<size_t>(span->bytes_in_span(), allocated_size_rounded);
-        if (limit <= hardware_page_size) {
+        uintptr_t start = reinterpret_cast<uintptr_t>(span->start_address());
+        uintptr_t length = span->bytes_in_span();
+        if (length <= hardware_page_size) {
           break;
         }
-        uintptr_t start = reinterpret_cast<uintptr_t>(span->start_address()) +
-                          hardware_page_size;
-        uintptr_t length = limit - hardware_page_size;
+        start += hardware_page_size;
+        length -= hardware_page_size;
 
         (void)state.system_allocator().Release(reinterpret_cast<void*>(start),
                                                length);

tcmalloc/internal/logging_test.cc

@@ -50,13 +50,13 @@ TEST(InternalLogging, MessageFormatting) {
   TC_LOG("Hello int=%d str=%s", 42, "bar");
   EXPECT_THAT(*log_buffer,
               testing::MatchesRegex(
-                  "[0-9]+ .*tcmalloc/internal/logging_test\\.cc:[0-9]+\\] "
+                  "[0-9]+ .*tcmalloc\\/internal/logging_test\\.cc:[0-9]+\\] "
                   "Hello int=42 str=bar\\n"));
 
   TC_LOG("Long string: %s", long_string.c_str());
   EXPECT_THAT(*log_buffer,
               testing::MatchesRegex(
-                  "[0-9]+ .*tcmalloc/internal/logging_test\\.cc:[0-9]+\\] "
+                  "[0-9]+ .*tcmalloc\\/internal/logging_test\\.cc:[0-9]+\\] "
                   "Long string: the quick brown fox jumped over the lazy "
                   "dogthe quick brown fox jumped over the lazy dog.*"));
 

tcmalloc/internal/timeseries_tracker.h

@@ -56,6 +56,7 @@ class TimeSeriesTracker {
     // See comment in GetCurrentEpoch().
     auto d = static_cast<uint64_t>(absl::ToDoubleSeconds(epoch_length_) *
                                    clock.freq());
+    TC_ASSERT_GT(d, 0);
     div_precision_ = 63 + absl::bit_width(d);
     epoch_ticks_m_ =
         static_cast<uint64_t>(
@@ -95,7 +96,11 @@ class TimeSeriesTracker {
   bool UpdateClock();
 
   // Returns the current epoch number based on the clock.
-  int64_t GetCurrentEpoch() {
+  size_t GetCurrentEpoch() {
+    const int64_t now = clock_.now();
+    if (ABSL_PREDICT_FALSE(now <= 0)) {
+      return 0;
+    }
     // This is equivalent to
     // `clock_.now() / (absl::ToDoubleSeconds(epoch_length_) * clock_.freq())`.
     // We basically follow the technique from
@@ -106,9 +111,9 @@ class TimeSeriesTracker {
     // is <2^63), it shouldn't cause a problem. This way, we don't need to
     // handle overflow so it's simpler. See also:
     // https://lemire.me/blog/2019/02/20/more-fun-with-fast-remainders-when-the-divisor-is-a-constant/.
-    return static_cast<int64_t>(static_cast<absl::uint128>(epoch_ticks_m_) *
-                                    clock_.now() >>
-                                div_precision_);
+    return static_cast<size_t>(static_cast<absl::uint128>(epoch_ticks_m_) *
+                                   static_cast<uint64_t>(now) >>
+                               div_precision_);
   }
   void InitTracker() {
     // Inits the tracker by "create" an record for "now" on slot 0. The record
@@ -188,12 +193,12 @@ void TimeSeriesTracker<T, S, kSlots>::Iter(
     absl::FunctionRef<void(size_t, size_t, const T&)> f) const {
   size_t j = current_slot_ + 1;
   if (j == kSlots) j = 0;
-  for (int sequenc_num = 0; sequenc_num < kSlots; sequenc_num++) {
+  for (size_t sequence_num = 0; sequence_num < kSlots; ++sequence_num) {
     // We would have no empty entries between valid data points hence there is
     // no reason to perform action on them. We think the slot would be all
     // filled shortly after the job started.
     if (ABSL_PREDICT_TRUE(!entries_[j].payload.empty())) {
-      f(sequenc_num, entries_[j].epoch_delta, entries_[j].payload);
+      f(sequence_num, entries_[j].epoch_delta, entries_[j].payload);
     }
     j++;
     if (j == kSlots) j = 0;
@@ -204,7 +209,7 @@ template <class T, class S, size_t kSlots>
 void TimeSeriesTracker<T, S, kSlots>::IterBackwards(
     absl::FunctionRef<void(size_t, size_t, const T&)> f,
     absl::Duration interval) const {
-  if (interval == absl::ZeroDuration()) return;
+  if (interval <= absl::ZeroDuration()) return;
   size_t epochs_to_traverse;
   if (interval == absl::InfiniteDuration()) {
     // InfiniteDuration() means that we are outputting all records.

tcmalloc/internal/timeseries_tracker_test.cc

@@ -345,6 +345,17 @@ TEST_F(TimeSeriesTrackerTest, ClockRegression) {
   EXPECT_THAT(recent_record.data.values_, ElementsAre(4));
 }
 
+TEST_F(TimeSeriesTrackerTest, NegativeClockDoesNotWrap) {
+  Advance(absl::Seconds(2));
+  tracker_.Report(1);
+
+  // Regressing the clock to a negative value should not wrap into a huge epoch.
+  Advance(-absl::Seconds(5));
+  EXPECT_FALSE(tracker_.Report(2));
+  tracker_.UpdateTimeBase();
+  EXPECT_EQ(tracker_.GetMostRecentRecord().epoch_taken, 1);
+}
+
 }  // namespace
 }  // namespace tcmalloc_internal
 }  // namespace tcmalloc

tcmalloc/mock_transfer_cache.h

@@ -418,18 +418,17 @@ class FakeShardedTransferCacheEnvironment {
   explicit FakeShardedTransferCacheEnvironment(int num_shards,
                                                bool use_generic_cache)
       : sharded_manager_(&owner_, &cpu_layout_) {
-    owner_.SetGenericCache(use_generic_cache);
-    owner_.SetCacheForLargeClassesOnly(!use_generic_cache);
+    if (use_generic_cache) {
+      owner_.SetGenericCache(true);
+    } else {
+      owner_.SetCacheForLargeClassesOnly(true);
+    }
 
     cpu_layout_.Init(num_shards);
     sharded_manager_.Init();
   }
 
-  ~FakeShardedTransferCacheEnvironment() {
-    Drain();
-    owner_.SetGenericCache(false);
-    owner_.SetCacheForLargeClassesOnly(false);
-  }
+  ~FakeShardedTransferCacheEnvironment() { Drain(); }
 
   void Remove(int cpu, int n) {
     cpu_layout_.SetCurrentCpu(cpu);

tcmalloc/testing/heap_profiling_test.cc

@@ -261,7 +261,7 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) {
 
   const ScopedProfileSamplingInterval sample_interval(1);
 
-  const size_t kHardwarePageSize = tcmalloc_internal::GetPageSize();
+  const size_t kPageSize = tcmalloc_internal::GetPageSize();
   constexpr int kNumAllocations = 50;
 
   enum class AllocationHeap {
@@ -277,38 +277,24 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) {
     AllocationHeap heap;
     bool expect_madvised;
     bool guarded;
-    size_t alloc_size;
   };
 
   const TestCase kTestCases[] = {
-      {"disabled_sampled_large", MadviseSampledAllocations::kDisabled,
+      {"disabled_sampled", MadviseSampledAllocations::kDisabled,
        AllocationHeap::kSampled,
-       /*expect_madvised=*/false, /*guarded=*/false,
-       /*alloc_size=*/2 * kHardwarePageSize},
-      {"enabled_sampled_large", MadviseSampledAllocations::kEnabled,
+       /*expect_madvised=*/false, /*guarded=*/false},
+      {"enabled_sampled", MadviseSampledAllocations::kEnabled,
        AllocationHeap::kSampled,
-       /*expect_madvised=*/true, /*guarded=*/false,
-       /*alloc_size=*/2 * kHardwarePageSize},
-      {"disabled_sampled_small", MadviseSampledAllocations::kDisabled,
-       AllocationHeap::kSampled,
-       /*expect_madvised=*/false, /*guarded=*/false,
-       /*alloc_size=*/1024},
-      {"enabled_sampled_small", MadviseSampledAllocations::kEnabled,
-       AllocationHeap::kSampled,
-       /*expect_madvised=*/false, /*guarded=*/false,
-       /*alloc_size=*/1024},
+       /*expect_madvised=*/true, /*guarded=*/false},
       {"enabled_guarded", MadviseSampledAllocations::kEnabled,
        AllocationHeap::kSampled,
-       /*expect_madvised=*/true, /*guarded=*/true,
-       /*alloc_size=*/32},
+       /*expect_madvised=*/true, /*guarded=*/true},
       {"enabled_cold", MadviseSampledAllocations::kEnabled,
        AllocationHeap::kCold,
-       /*expect_madvised=*/true, /*guarded=*/false,
-       /*alloc_size=*/tcmalloc_internal::kMaxSize + 2 * kHardwarePageSize},
+       /*expect_madvised=*/true, /*guarded=*/false},
       {"enabled_normal", MadviseSampledAllocations::kEnabled,
        AllocationHeap::kNormal,
-       /*expect_madvised=*/false, /*guarded=*/false,
-       /*alloc_size=*/tcmalloc_internal::kMaxSize + 2 * kHardwarePageSize},
+       /*expect_madvised=*/false, /*guarded=*/false},
   };
 
   tcmalloc_internal::ResidencyPageMap residency;
@@ -321,18 +307,17 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) {
              tcmalloc_internal::HeapPartitioningMode::kFull)) {
       continue;
     }
-    if (test_case.alloc_size <= kHardwarePageSize &&
-        tcmalloc_internal::kPageSize <= kHardwarePageSize) {
-      continue;
-    }
 
     ScopedMadviseSampledAllocations s(test_case.madvise_sampled);
 
     const int num_allocations = test_case.guarded ? 1 : kNumAllocations;
     const ScopedGuardedSamplingInterval guarded_interval(
         test_case.guarded ? 1 : -1);
 
-    const size_t alloc_size = test_case.alloc_size;
+    const size_t alloc_size = (test_case.heap == AllocationHeap::kNormal ||
+                               test_case.heap == AllocationHeap::kCold)
+                                  ? tcmalloc_internal::kMaxSize + 2 * kPageSize
+                                  : (test_case.guarded ? 32 : 2 * kPageSize);
 
     auto allocate = [&]() -> void* {
       if (test_case.heap == AllocationHeap::kCold) {
@@ -341,11 +326,6 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) {
       return ::operator new(alloc_size);
     };
 
-    const size_t touch_all_size =
-        (test_case.heap == AllocationHeap::kSampled && !test_case.guarded)
-            ? std::max(alloc_size, tcmalloc_internal::kPageSize)
-            : alloc_size;
-
     void* allocs[kNumAllocations];
     for (int i = 0; i < num_allocations; ++i) {
       allocs[i] = allocate();
@@ -361,40 +341,31 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) {
           EXPECT_TRUE(tcmalloc_internal::IsNormalMemory(allocs[i]));
           break;
       }
-      memset(allocs[i], 0xAB, touch_all_size);
+      memset(allocs[i], 0xAB, alloc_size);
     }
     for (int i = 0; i < num_allocations; ++i) {
       sized_delete(allocs[i], alloc_size);
     }
 
     // Reallocate and touch only the first page.
-    const size_t touch_size = std::min(alloc_size, kHardwarePageSize);
+    const size_t touch_size = std::min(alloc_size, kPageSize);
     for (int i = 0; i < num_allocations; ++i) {
       allocs[i] = allocate();
       memset(allocs[i], 0xCD, touch_size);
     }
 
-    if (test_case.alloc_size > kHardwarePageSize) {
-      size_t total_resident = 0;
-      for (int i = 0; i < num_allocations; ++i) {
-        auto info = residency.Get(allocs[i], alloc_size);
-        ASSERT_TRUE(info.has_value());
-        total_resident += info->bytes_resident;
-      }
-
-      if (test_case.expect_madvised) {
-        EXPECT_LE(total_resident, num_allocations * touch_size);
-        EXPECT_GE(total_resident, (num_allocations - 1) * touch_size);
-      } else {
-        EXPECT_GT(total_resident, num_allocations * touch_size);
-      }
-    }
-
-    size_t total_allocated_resident = 0;
+    size_t total_resident = 0;
     for (int i = 0; i < num_allocations; ++i) {
       auto info = residency.Get(allocs[i], alloc_size);
       ASSERT_TRUE(info.has_value());
-      total_allocated_resident += info->bytes_resident;
+      total_resident += info->bytes_resident;
+    }
+
+    if (test_case.expect_madvised) {
+      EXPECT_LE(total_resident, num_allocations * touch_size);
+      EXPECT_GE(total_resident, (num_allocations - 1) * touch_size);
+    } else {
+      EXPECT_GT(total_resident, num_allocations * touch_size);
     }
 
     auto converted_or = tcmalloc_internal::MakeProfileProto(
@@ -424,7 +395,7 @@ TEST(HeapProfilingTest, MadviseSampledAllocations) {
     for (const auto& sample : converted.sample()) {
       profile_resident += sample.value(*resident_value_index);
     }
-    EXPECT_GE(profile_resident, total_allocated_resident * 9 / 10);
+    EXPECT_GE(profile_resident, total_resident * 9 / 10);
 
     for (int i = 0; i < num_allocations; ++i) {
       sized_delete(allocs[i], alloc_size);