Avoid consulting clock twice between HugePageFiller and TimeSeriesTracker.

tcmalloc/huge_page_filler.h

@@ -1000,7 +1000,7 @@ class HugePageFiller {
   static constexpr size_t kLifetimeBuckets =
       huge_page_filler_internal::UsageInfo::kLifetimeBuckets;
   using LifetimeHisto = huge_page_filler_internal::UsageInfo::LifetimeHisto;
-  void RecordLifetime(const TrackerType* pt);
+  void RecordLifetime(const TrackerType* pt, double now);
   void PrintLifetimeHisto(Printer& out, LifetimeHisto h,
                           AccessDensityPrediction type,
                           absl::string_view blurb) const;
@@ -1060,7 +1060,7 @@ class HugePageFiller {
   Length unmapping_unaccounted_;
 
   // Functionality related to time series tracking.
-  void UpdateFillerStatsTracker();
+  void UpdateFillerStatsTracker(int64_t now);
   using StatsTrackerType = SubreleaseStatsTracker<600>;
   StatsTrackerType fillerstats_tracker_;
 
@@ -1254,13 +1254,16 @@ HugePageFiller<TrackerType>::TryGet(Length n, SpanAllocInfo span_alloc_info) {
   // We're being used for an allocation, so we are no longer considered
   // donated by this point.
   TC_ASSERT(!pt->donated());
-  UpdateFillerStatsTracker();
+  UpdateFillerStatsTracker(now);
   return {pt, page_allocation.page, was_released};
 }
 
 template <class TrackerType>
-void HugePageFiller<TrackerType>::RecordLifetime(const TrackerType* pt) {
-  const double now = clock_.now();
+void HugePageFiller<TrackerType>::RecordLifetime(const TrackerType* pt,
+                                                 double now) {
+#ifdef TCMALLOC_INTERNAL_LEGACY_LOCKING
+  now = clock_.now();
+#endif
   const double frequency = clock_.freq();
   const double elapsed = std::max<double>(now - pt->alloctime(), 0);
   const absl::Duration lifetime =
@@ -1313,6 +1316,11 @@ void HugePageFiller<TrackerType>::PrintLifetimeHistoInPbtxt(
 template <class TrackerType>
 inline TrackerType* HugePageFiller<TrackerType>::Put(
     TrackerType* pt, Range r, SpanAllocInfo span_alloc_info) {
+#ifndef TCMALLOC_INTERNAL_LEGACY_LOCKING
+  const int64_t now = clock_.now();
+#else
+  const int64_t now = 0;
+#endif
   RemoveFromFillerList(pt);
   pt->Put(r, span_alloc_info);
   if (pt->HasDenseSpans()) {
@@ -1359,8 +1367,8 @@ inline TrackerType* HugePageFiller<TrackerType>::Put(
     }
 
     if (!pt->DontFreeTracker()) {
-      RecordLifetime(pt);
-      UpdateFillerStatsTracker();
+      RecordLifetime(pt, now);
+      UpdateFillerStatsTracker(now);
       if (pt->GetTagState().sampled_for_tagging) {
         // Set the default region name if the tracked was sampled.
         pt->SetAnonVmaName(set_anon_vma_name_, /*name=*/std::nullopt);
@@ -1369,7 +1377,7 @@ inline TrackerType* HugePageFiller<TrackerType>::Put(
     }
   }
   AddToFillerList(pt);
-  UpdateFillerStatsTracker();
+  UpdateFillerStatsTracker(now);
   return nullptr;
 }
 
@@ -1398,7 +1406,7 @@ inline void HugePageFiller<TrackerType>::Contribute(
   }
 
   ++size_;
-  UpdateFillerStatsTracker();
+  UpdateFillerStatsTracker(clock_.now());
 }
 
 template <class TrackerType>
@@ -1532,7 +1540,7 @@ inline Length HugePageFiller<TrackerType>::GetDesiredSubreleasePages(
   if (!intervals.SkipSubreleaseEnabled()) {
     return desired;
   }
-  UpdateFillerStatsTracker();
+  UpdateFillerStatsTracker(clock_.now());
   Length required_pages;
   // As mentioned above, there are two ways to calculate the demand
   // requirement. We give priority to using the peak if peak_interval is set.
@@ -2392,13 +2400,18 @@ inline void HugePageFiller<TrackerType>::PrintInPbtxt(
 }
 
 template <class TrackerType>
-inline void HugePageFiller<TrackerType>::UpdateFillerStatsTracker() {
+inline void HugePageFiller<TrackerType>::UpdateFillerStatsTracker(
+    [[maybe_unused]] int64_t now) {
   StatsTrackerType::SubreleaseStats stats;
   stats.num_pages = pages_allocated();
   stats.free_pages = free_pages();
   stats.unmapped_pages = unmapped_pages();
   stats.num_pages_subreleased = subrelease_stats_.num_pages_subreleased;
-  fillerstats_tracker_.Report(stats);
+#ifdef TCMALLOC_INTERNAL_LEGACY_LOCKING
+  fillerstats_tracker_.Report(stats, clock_.now());
+#else
+  fillerstats_tracker_.Report(stats, now);
+#endif
   subrelease_stats_.reset();
 }
 

tcmalloc/huge_page_subrelease.h

@@ -87,12 +87,12 @@ class SkippedSubreleaseCorrectnessTracker {
     tracker_.Report(update);
   }
 
-  void ReportUpdatedPeak(Length current_peak) {
+  void ReportUpdatedPeak(Length current_peak, int64_t now) {
     // Record this peak for the current epoch (so we don't double-count correct
     // predictions later) and advance the tracker.
     SkippedSubreleaseUpdate update;
     update.confirmed_peak = current_peak;
-    if (tracker_.Report(update)) {
+    if (tracker_.Report(update, now)) {
       // Also keep track of the largest peak we have confirmed this epoch.
       last_confirmed_peak_ = Length(0);
     }
@@ -283,8 +283,8 @@ class SubreleaseStatsTracker {
   SubreleaseStatsTracker(const SubreleaseStatsTracker&) = delete;
   SubreleaseStatsTracker& operator=(const SubreleaseStatsTracker&) = delete;
 
-  void Report(const SubreleaseStats& stats) {
-    if (ABSL_PREDICT_FALSE(tracker_.Report(stats))) {
+  void Report(const SubreleaseStats& stats, int64_t now) {
+    if (ABSL_PREDICT_FALSE(tracker_.Report(stats, now))) {
       if (ABSL_PREDICT_FALSE(pending_skipped().count > 0)) {
         Length peak = stats.num_pages;
         // Consider the peak in the most recent record that reported within the
@@ -295,7 +295,7 @@ class SubreleaseStatsTracker {
           peak = std::max(
               peak, most_recent_record.data.stats[kStatsAtMaxDemand].num_pages);
         }
-        skipped_subrelease_correctness_.ReportUpdatedPeak(peak);
+        skipped_subrelease_correctness_.ReportUpdatedPeak(peak, now);
       }
     }
   }

tcmalloc/huge_page_subrelease_test.cc

@@ -92,7 +92,7 @@ void StatsTrackerTest::GenerateInterestingPoints(Length num_pages,
       stats.num_pages = num_pages + Length((i == 0) ? 4 : 8 * j);
       stats.free_pages = num_free_pages + Length(10 * i + j);
       stats.unmapped_pages = Length(10);
-      tracker_.Report(stats);
+      tracker_.Report(stats, clock_);
     }
   }
 }
@@ -103,7 +103,7 @@ void StatsTrackerTest::GenerateDemandPoint(Length num_pages,
   stats.num_pages = num_pages;
   stats.free_pages = num_free_pages;
   stats.unmapped_pages = Length(0);
-  tracker_.Report(stats);
+  tracker_.Report(stats, clock_);
 }
 
 // Tests that the tracker aggregates all data correctly. The output is tested by

tcmalloc/internal/timeseries_tracker.h

@@ -64,7 +64,8 @@ class TimeSeriesTracker {
     InitTracker();
   }
 
-  bool Report(const S& val);
+  bool Report(const S& val, int64_t now);
+  bool Report(const S& val) { return Report(val, clock_.now()); }
 
   // Iterates over the time series, starting from the oldest entry. The callback
   // receives the sequence number of the entry, the epoch_delta (i.e., number of
@@ -92,40 +93,28 @@ class TimeSeriesTracker {
 
  private:
   // Returns true if the tracker moved to a different epoch.
-  bool UpdateClock();
+  bool UpdateClock(int64_t now);
+  bool UpdateClock() { return UpdateClock(clock_.now()); }
 
   // Returns the current epoch number based on the clock.
-  int64_t GetCurrentEpoch() {
-    // This is equivalent to
-    // `clock_.now() / (absl::ToDoubleSeconds(epoch_length_) * clock_.freq())`.
-    // We basically follow the technique from
-    // https://ridiculousfish.com/blog/posts/labor-of-division-episode-i.html,
-    // except that we use one fewer bit of precision than necessary to always
-    // get the correct answer if the numerator were a 64-bit unsigned number. In
-    // this case, because clock_.now() returns a signed 64-bit number (i.e. max
-    // 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/.
+  int64_t GetCurrentEpoch(int64_t now) const {
     return static_cast<int64_t>(static_cast<absl::uint128>(epoch_ticks_m_) *
-                                    clock_.now() >>
+                                    std::max<int64_t>(0, now) >>
                                 div_precision_);
   }
-  void InitTracker() {
-    // Inits the tracker by "create" an record for "now" on slot 0. The record
-    // serves as the first valid record in the tracker, with epoch coverage
-    // (delta) 1 and an empty payload. In this way, we would know when was the
-    // first real data point taken, as it would be the time diff between "now"
-    // and "then". If the first data point is taken immediately, this record
-    // will be updated with real data.
+  int64_t GetCurrentEpoch() const { return GetCurrentEpoch(clock_.now()); }
+
+  void InitTracker(int64_t now) {
     current_slot_ = 0;
     for (auto& entry : entries_) {
       entry = TimeSeriesContent();
     }
     size_t delta = 1;
-    last_epoch_ = GetCurrentEpoch();
+    last_epoch_ = GetCurrentEpoch(now);
     entries_[current_slot_] = TimeSeriesContent(delta);
     covered_epochs_ = delta;
   }
+  void InitTracker() { InitTracker(clock_.now()); }
 
   struct TimeSeriesContent {
     size_t epoch_delta;  // Time gap (in epoch) between this slot and
@@ -155,14 +144,14 @@ class TimeSeriesTracker {
 // Advances the current slot if the clock had advanced >= 1 epoch; sets the
 // epoch_delta for how many epoch had passed sice the previous clock update.
 template <class T, class S, size_t kSlots>
-bool TimeSeriesTracker<T, S, kSlots>::UpdateClock() {
-  const size_t epoch = GetCurrentEpoch();
+bool TimeSeriesTracker<T, S, kSlots>::UpdateClock(int64_t now) {
+  const size_t epoch = GetCurrentEpoch(now);
   if (ABSL_PREDICT_FALSE(epoch < last_epoch_)) {
     // If the clock has regressed (e.g., across snapshot restore or container
     // migration to a host with a lower monotonic clock), reset the tracker to
     // prevent unsigned underflow in delta calculations and discard stale
     // history.
-    InitTracker();
+    InitTracker(now);
     return false;
   }
   if (epoch == last_epoch_) {
@@ -243,8 +232,8 @@ TimeSeriesTracker<T, S, kSlots>::GetMostRecentRecord() const {
 }
 
 template <class T, class S, size_t kSlots>
-bool TimeSeriesTracker<T, S, kSlots>::Report(const S& val) {
-  bool updated_clock = UpdateClock();
+bool TimeSeriesTracker<T, S, kSlots>::Report(const S& val, int64_t now) {
+  bool updated_clock = UpdateClock(now);
   entries_[current_slot_].payload.Report(val);
   return updated_clock;
 }

tcmalloc/internal/timeseries_tracker_test.cc

@@ -345,6 +345,31 @@ TEST_F(TimeSeriesTrackerTest, ClockRegression) {
   EXPECT_THAT(recent_record.data.values_, ElementsAre(4));
 }
 
+TEST_F(TimeSeriesTrackerTest, ExplicitTimestamp) {
+  const int64_t t0 = 0;
+  tracker_.Report(1, t0);
+  const int64_t t1 = absl::ToDoubleNanoseconds(absl::Seconds(1));
+  tracker_.Report(2, t1);
+
+  std::vector<std::vector<int>> all_values;
+  tracker_.Iter([&](size_t offset, size_t epoch_delta, const TestEntry& e) {
+    all_values.push_back(e.values_);
+  });
+  EXPECT_THAT(all_values, ElementsAre(ElementsAre(1), ElementsAre(2)));
+}
+
+TEST_F(TimeSeriesTrackerTest, ClampedNegativeClock) {
+  tracker_.Report(1, -1000);
+  tracker_.Report(2, -1);
+  tracker_.Report(3, 0);
+
+  std::vector<std::vector<int>> all_values;
+  tracker_.Iter([&](size_t offset, size_t epoch_delta, const TestEntry& e) {
+    all_values.push_back(e.values_);
+  });
+  EXPECT_THAT(all_values, ElementsAre(ElementsAre(1, 2, 3)));
+}
+
 }  // namespace
 }  // namespace tcmalloc_internal
 }  // namespace tcmalloc