Introduce Counters::get/set/increment methods, replacing operator[]

The primary motivation for this is that it’s now possible to detect and
block underflow when incrementing a value in the unlikely but possible
event that a, say, cache_size_kibibyte is decremented below zero.

diff --git a/src/Counters.cpp b/src/Counters.cpp
index a680fd773575c1cf25db90d8c3f1837e3fca5602..01db7356c03a7aba087aab120d0f27e261c6bfb6 100644 (file)
--- a/src/Counters.cpp
+++ b/src/Counters.cpp
@@ -26,25 +26,35 @@ Counters::Counters() : m_counters(static_cast<size_t>(Statistic::END))
 {
 }
 
-// clang-format off
-uint64_t&
-Counters::operator[](Statistic index)
-// clang-format on
+uint64_t
+Counters::get(Statistic statistic) const
 {
-  const size_t i = static_cast<size_t>(index);
+  assert(static_cast<size_t>(statistic) < static_cast<size_t>(Statistic::END));
+  const size_t i = static_cast<size_t>(statistic);
+  return i < m_counters.size() ? m_counters[i] : 0;
+}
+
+void
+Counters::set(Statistic statistic, uint64_t value)
+{
+  const auto i = static_cast<size_t>(statistic);
   if (i >= m_counters.size()) {
     m_counters.resize(i + 1);
   }
-  return m_counters.at(i);
+  m_counters[i] = value;
 }
 
-// clang-format off
-uint64_t
-Counters::operator[](Statistic index) const
-// clang-format on
+void
+Counters::increment(Statistic statistic, int64_t value)
 {
-  const size_t i = static_cast<size_t>(index);
-  return i < m_counters.size() ? m_counters.at(i) : 0;
+  const auto i = static_cast<size_t>(statistic);
+  if (i >= m_counters.size()) {
+    m_counters.resize(i + 1);
+  }
+  auto& counter = m_counters[i];
+  counter =
+    std::max(static_cast<int64_t>(0), static_cast<int64_t>(counter + value));
+  }
 }
 
 size_t

diff --git a/src/Counters.hpp b/src/Counters.hpp
index cdd6c151501b5feff9ddcfb889d550163e503ca9..cc8177b9c35c5889a2c72864e74e239c4c23028a 100644 (file)
--- a/src/Counters.hpp
+++ b/src/Counters.hpp
@@ -31,8 +31,9 @@ class Counters
 public:
   Counters();
 
-  uint64_t& operator[](Statistic index);
-  uint64_t operator[](Statistic index) const;
+  uint64_t get(Statistic statistic) const;
+  void set(Statistic statistic, uint64_t value);
+  void increment(Statistic statistic, int64_t value = 1);
 
   size_t size() const;
 

diff --git a/src/Result.cpp b/src/Result.cpp
index ca3c06689ef431ac28d41eb1655b0b9789548247..96174f3edd85d7ace8df21dc8d60d82640a331d1 100644 (file)
--- a/src/Result.cpp
+++ b/src/Result.cpp
@@ -411,8 +411,8 @@ Result::Writer::write_raw_file_entry(const std::string& path,
   const auto size_delta =
     (new_stat.size_on_disk() - old_stat.size_on_disk()) / 1024;
   const auto files_delta = (new_stat ? 1 : 0) - (old_stat ? 1 : 0);
-  m_ctx.counter_updates[Statistic::cache_size_kibibyte] += size_delta;
-  m_ctx.counter_updates[Statistic::files_in_cache] += files_delta;
+  m_ctx.counter_updates.increment(Statistic::cache_size_kibibyte, size_delta);
+  m_ctx.counter_updates.increment(Statistic::files_in_cache, files_delta);
 }
 
 } // namespace Result

diff --git a/src/ccache.cpp b/src/ccache.cpp
index 4ef4a1c7ddb8cfcff9a348fd82b458b485b2a1ea..2f6b2d7ced3999d82dbf435643e9119c4a875e8e 100644 (file)
--- a/src/ccache.cpp
+++ b/src/ccache.cpp
@@ -757,12 +757,12 @@ update_manifest_file(Context& ctx)
     const int64_t files_delta = !old_st && st ? 1 : 0;
 
     if (ctx.stats_file() == ctx.manifest_stats_file()) {
-      ctx.counter_updates[Statistic::cache_size_kibibyte] += size_delta;
-      ctx.counter_updates[Statistic::files_in_cache] += files_delta;
+      ctx.counter_updates.increment(Statistic::cache_size_kibibyte, size_delta);
+      ctx.counter_updates.increment(Statistic::files_in_cache, files_delta);
     } else {
       Counters counters;
-      counters[Statistic::cache_size_kibibyte] += size_delta;
-      counters[Statistic::files_in_cache] += files_delta;
+      counters.increment(Statistic::cache_size_kibibyte, size_delta);
+      counters.increment(Statistic::files_in_cache, files_delta);
       stats_flush_to_file(ctx.config, ctx.manifest_stats_file(), counters);
     }
   }
@@ -977,8 +977,9 @@ to_cache(Context& ctx,
   }
   const auto size_delta =
     (new_dest_stat.size_on_disk() - orig_dest_stat.size_on_disk()) / 1024;
-  ctx.counter_updates[Statistic::cache_size_kibibyte] += size_delta;
-  ctx.counter_updates[Statistic::files_in_cache] += orig_dest_stat ? 0 : 1;
+  ctx.counter_updates.increment(Statistic::cache_size_kibibyte, size_delta);
+  ctx.counter_updates.increment(Statistic::files_in_cache,
+                                orig_dest_stat ? 0 : 1);
 
   MTR_END("file", "file_put");
 
@@ -1953,11 +1954,11 @@ cache_compilation(int argc, const char* const* argv)
 
   try {
     Statistic statistic = do_cache_compilation(*ctx, argv);
-    ctx->counter_updates[statistic] += 1;
+    ctx->counter_updates.increment(statistic);
     return EXIT_SUCCESS;
   } catch (const Failure& e) {
     if (e.statistic() != Statistic::none) {
-      ctx->counter_updates[e.statistic()] += 1;
+      ctx->counter_updates.increment(e.statistic());
     }
 
     if (e.exit_code()) {

diff --git a/src/compress.cpp b/src/compress.cpp
index 5ed62aee2ece7238688a04f28fa41989c7649ca4..961faab765577d225d3451cd33dc8e92bef87a35 100644 (file)
--- a/src/compress.cpp
+++ b/src/compress.cpp
@@ -201,10 +201,10 @@ recompress_file(Context& ctx,
   const size_t size_delta =
     (new_stat.size_on_disk() - old_stat.size_on_disk()) / 1024;
   if (ctx.stats_file() == stats_file) {
-    ctx.counter_updates[Statistic::cache_size_kibibyte] += size_delta;
+    ctx.counter_updates.increment(Statistic::cache_size_kibibyte, size_delta);
   } else {
     Counters counters;
-    counters[Statistic::cache_size_kibibyte] += size_delta;
+    counters.increment(Statistic::cache_size_kibibyte, size_delta);
     stats_flush_to_file(ctx.config, stats_file, counters);
   }
 

diff --git a/src/stats.cpp b/src/stats.cpp
index 53aa5438c9cd7ca904787daddfd72729ff649cfd..70350f82884a299a5f3f3ef88333a5cbbaa2569c 100644 (file)
--- a/src/stats.cpp
+++ b/src/stats.cpp
@@ -161,7 +161,7 @@ parse_stats(Counters& counters, const std::string& buf)
     if (p2 == p) {
       break;
     }
-    counters[static_cast<Statistic>(i)] += val;
+    counters.increment(static_cast<Statistic>(i), val);
     i++;
     p = p2;
   }
@@ -173,7 +173,7 @@ stats_write(const std::string& path, const Counters& counters)
 {
   AtomicFile file(path, AtomicFile::Mode::text);
   for (size_t i = 0; i < counters.size(); ++i) {
-    file.write(fmt::format("{}\n", counters[static_cast<Statistic>(i)]));
+    file.write(fmt::format("{}\n", counters.get(static_cast<Statistic>(i))));
   }
   try {
     file.commit();
@@ -188,10 +188,10 @@ stats_write(const std::string& path, const Counters& counters)
 static double
 stats_hit_rate(const Counters& counters)
 {
-  uint64_t direct = counters[Statistic::direct_cache_hit];
-  uint64_t preprocessed = counters[Statistic::preprocessed_cache_hit];
+  uint64_t direct = counters.get(Statistic::direct_cache_hit);
+  uint64_t preprocessed = counters.get(Statistic::preprocessed_cache_hit);
   uint64_t hit = direct + preprocessed;
-  uint64_t miss = counters[Statistic::cache_miss];
+  uint64_t miss = counters.get(Statistic::cache_miss);
   uint64_t total = hit + miss;
   return total > 0 ? (100.0 * hit) / total : 0.0;
 }
@@ -213,17 +213,17 @@ stats_collect(const Config& config, Counters& counters, time_t* last_updated)
       fname = fmt::format("{}/{:x}/stats", config.cache_dir(), dir);
     }
 
-    counters[Statistic::stats_zeroed_timestamp] = 0; // Don't add
+    counters.set(Statistic::stats_zeroed_timestamp, 0); // Don't add
     stats_read(fname, counters);
     zero_timestamp =
-      std::max(counters[Statistic::stats_zeroed_timestamp], zero_timestamp);
+      std::max(counters.get(Statistic::stats_zeroed_timestamp), zero_timestamp);
     auto st = Stat::stat(fname);
     if (st && st.mtime() > *last_updated) {
       *last_updated = st.mtime();
     }
   }
 
-  counters[Statistic::stats_zeroed_timestamp] = zero_timestamp;
+  counters.set(Statistic::stats_zeroed_timestamp, zero_timestamp);
 }
 
 // Read in the stats from one directory and add to the counters.
@@ -256,7 +256,7 @@ stats_flush_to_file(const Config& config,
 
   if (!config.log_file().empty() || config.debug()) {
     for (auto& field : k_statistics_fields) {
-      if (updates[field.statistic] != 0 && !(field.flags & FLAG_NOZERO)) {
+      if (updates.get(field.statistic) != 0 && !(field.flags & FLAG_NOZERO)) {
         log("Result: {}", field.message);
       }
     }
@@ -276,7 +276,8 @@ stats_flush_to_file(const Config& config,
 
     stats_read(sfile, counters);
     for (size_t i = 0; i < static_cast<size_t>(Statistic::END); ++i) {
-      counters[static_cast<Statistic>(i)] += updates[static_cast<Statistic>(i)];
+      counters.increment(static_cast<Statistic>(i),
+                         updates.get(static_cast<Statistic>(i)));
     }
     stats_write(sfile, counters);
   }
@@ -285,19 +286,19 @@ stats_flush_to_file(const Config& config,
   bool need_cleanup = false;
 
   if (config.max_files() != 0
-      && counters[Statistic::files_in_cache] > config.max_files() / 16) {
+      && counters.get(Statistic::files_in_cache) > config.max_files() / 16) {
     log("Need to clean up {} since it holds {} files (limit: {} files)",
         subdir,
-        counters[Statistic::files_in_cache],
+        counters.get(Statistic::files_in_cache),
         config.max_files() / 16);
     need_cleanup = true;
   }
   if (config.max_size() != 0
-      && counters[Statistic::cache_size_kibibyte]
+      && counters.get(Statistic::cache_size_kibibyte)
            > config.max_size() / 1024 / 16) {
     log("Need to clean up {} since it holds {} KiB (limit: {} KiB)",
         subdir,
-        counters[Statistic::cache_size_kibibyte],
+        counters.get(Statistic::cache_size_kibibyte),
         config.max_size() / 1024 / 16);
     need_cleanup = true;
   }
@@ -349,16 +350,16 @@ stats_summary(const Context& ctx)
     if (k_statistics_fields[i].flags & FLAG_NEVER) {
       continue;
     }
-    if (counters[statistic] == 0
+    if (counters.get(statistic) == 0
         && !(k_statistics_fields[i].flags & FLAG_ALWAYS)) {
       continue;
     }
 
     std::string value;
     if (k_statistics_fields[i].format) {
-      value = k_statistics_fields[i].format(counters[statistic]);
+      value = k_statistics_fields[i].format(counters.get(statistic));
     } else {
-      value = fmt::format("{:8}", counters[statistic]);
+      value = fmt::format("{:8}", counters.get(statistic));
     }
     if (!value.empty()) {
       fmt::print("{:31} {}\n", k_statistics_fields[i].message, value);
@@ -394,7 +395,7 @@ stats_print(const Config& config)
     if (!(k_statistics_fields[i].flags & FLAG_NEVER)) {
       fmt::print("{}\t{}\n",
                  k_statistics_fields[i].id,
-                 counters[k_statistics_fields[i].statistic]);
+                 counters.get(k_statistics_fields[i].statistic));
     }
   }
 }
@@ -420,10 +421,10 @@ stats_zero(const Context& ctx)
       stats_read(fname, counters);
       for (size_t i = 0; k_statistics_fields[i].message; i++) {
         if (!(k_statistics_fields[i].flags & FLAG_NOZERO)) {
-          counters[k_statistics_fields[i].statistic] = 0;
+          counters.set(k_statistics_fields[i].statistic, 0);
         }
       }
-      counters[Statistic::stats_zeroed_timestamp] = timestamp;
+      counters.set(Statistic::stats_zeroed_timestamp, timestamp);
       stats_write(fname, counters);
     }
   }
@@ -441,8 +442,8 @@ stats_get_obsolete_limits(const std::string& dir,
   Counters counters;
   std::string sname = dir + "/stats";
   stats_read(sname, counters);
-  *maxfiles = counters[Statistic::obsolete_max_files];
-  *maxsize = counters[Statistic::obsolete_max_size] * 1024;
+  *maxfiles = counters.get(Statistic::obsolete_max_files);
+  *maxsize = counters.get(Statistic::obsolete_max_size) * 1024;
 }
 
 // Set the per-directory sizes.
@@ -454,8 +455,8 @@ stats_set_sizes(const std::string& dir, uint64_t num_files, uint64_t total_size)
   Lockfile lock(statsfile);
   if (lock.acquired()) {
     stats_read(statsfile, counters);
-    counters[Statistic::files_in_cache] = num_files;
-    counters[Statistic::cache_size_kibibyte] = total_size / 1024;
+    counters.set(Statistic::files_in_cache, num_files);
+    counters.set(Statistic::cache_size_kibibyte, total_size / 1024);
     stats_write(statsfile, counters);
   }
 }
@@ -469,7 +470,7 @@ stats_add_cleanup(const std::string& dir, uint64_t count)
   Lockfile lock(statsfile);
   if (lock.acquired()) {
     stats_read(statsfile, counters);
-    counters[Statistic::cleanups_performed] += count;
+    counters.increment(Statistic::cleanups_performed, count);
     stats_write(statsfile, counters);
   }
 }