Refactor Statistics::{write,increment} into Statistics::update

diff --git a/src/Statistics.cpp b/src/Statistics.cpp
index 6b268ffb251d8e1600b18b8cdc1a6c424b582360..4d9fd4e9a01328b33f376f4d78194c3e0d29e01c 100644 (file)
--- a/src/Statistics.cpp
+++ b/src/Statistics.cpp
@@ -59,9 +59,19 @@ read(const std::string& path)
   return counters;
 }
 
-void
-write(const std::string& path, const Counters& counters)
+optional<Counters>
+update(const std::string& path,
+       std::function<void(Counters& counters)> function)
 {
+  Lockfile lock(path);
+  if (!lock.acquired()) {
+    log("failed to acquire lock for {}", path);
+    return nullopt;
+  }
+
+  auto counters = Statistics::read(path);
+  function(counters);
+
   AtomicFile file(path, AtomicFile::Mode::text);
   for (size_t i = 0; i < counters.size(); ++i) {
     file.write(fmt::format("{}\n", counters.get_raw(i)));
@@ -69,23 +79,12 @@ write(const std::string& path, const Counters& counters)
   try {
     file.commit();
   } catch (const Error& e) {
-    // Make failure to write a stats file a soft error since it's not important
-    // enough to fail whole the process and also because it is called in the
-    // Context destructor.
+    // Make failure to write a stats file a soft error since it's not
+    // important enough to fail whole the process and also because it is
+    // called in the Context destructor.
     log("Error: {}", e.what());
   }
-}
 
-optional<Counters>
-increment(const std::string& path, const Counters& updates)
-{
-  Lockfile lock(path);
-  if (!lock.acquired()) {
-    return nullopt;
-  }
-  auto counters = Statistics::read(path);
-  counters.increment(updates);
-  Statistics::write(path, counters);
   return counters;
 }
 

diff --git a/src/Statistics.hpp b/src/Statistics.hpp
index 0008da0f10343bb82d01dfd5174ac585d5d7d445..9aa0ace680055a6610c7efb53e022b465462824e 100644 (file)
--- a/src/Statistics.hpp
+++ b/src/Statistics.hpp
@@ -24,6 +24,7 @@
 
 #include "third_party/nonstd/optional.hpp"
 
+#include <functional>
 #include <string>
 
 // Statistics fields in storage order.
@@ -70,13 +71,10 @@ namespace Statistics {
 // Read counters from `path`. No lock is acquired.
 Counters read(const std::string& path);
 
-// Write `counters` to `path`. No lock is acquired.
-void write(const std::string& path, const Counters& counters);
-
-// Acquire a lock, read counters from `path`, apply `updates`, write result to
-// `path` and release the lock. Returns the resulting counters or nullopt on
-// error (e.g. if the lock could not be acquired).
-nonstd::optional<Counters> increment(const std::string& path,
-                                     const Counters& updates);
+// Acquire a lock, read counters from `path`, call `function` with the counters,
+// write the counters to `path` and release the lock. Returns the resulting
+// counters or nullopt on error (e.g. if the lock could not be acquired).
+nonstd::optional<Counters> update(const std::string& path,
+                                  std::function<void(Counters& counters)>);
 
 } // namespace Statistics

diff --git a/src/ccache.cpp b/src/ccache.cpp
index 7dc598b4ffa046c459470831e0789307796b4d15..e025cfc1674db713bfd3f032233a53b647fe79e2 100644 (file)
--- a/src/ccache.cpp
+++ b/src/ccache.cpp
@@ -755,18 +755,19 @@ update_manifest_file(Context& ctx)
   } else {
     const auto st = Stat::stat(ctx.manifest_path(), Stat::OnError::log);
 
-    const int64_t size_delta =
+    const int64_t size_delta_kibibyte =
       (st.size_on_disk() - old_st.size_on_disk()) / 1024;
     const int64_t files_delta = !old_st && st ? 1 : 0;
 
     if (ctx.stats_file() == ctx.manifest_stats_file()) {
-      ctx.counter_updates.increment(Statistic::cache_size_kibibyte, size_delta);
+      ctx.counter_updates.increment(Statistic::cache_size_kibibyte,
+                                    size_delta_kibibyte);
       ctx.counter_updates.increment(Statistic::files_in_cache, files_delta);
     } else {
-      Counters updates;
-      updates.increment(Statistic::cache_size_kibibyte, size_delta);
-      updates.increment(Statistic::files_in_cache, files_delta);
-      Statistics::increment(ctx.manifest_stats_file(), updates);
+      Statistics::update(ctx.manifest_stats_file(), [=](Counters& cs) {
+        cs.increment(Statistic::cache_size_kibibyte, size_delta_kibibyte);
+        cs.increment(Statistic::files_in_cache, files_delta);
+      });
     }
   }
   MTR_END("manifest", "manifest_put");
@@ -1919,7 +1920,9 @@ finalize_stats(const Context& ctx)
   }
 
   const auto counters =
-    Statistics::increment(ctx.stats_file(), ctx.counter_updates);
+    Statistics::update(ctx.stats_file(), [&ctx](Counters& cs) {
+      cs.increment(ctx.counter_updates);
+    });
   if (!counters) {
     return;
   }

diff --git a/src/cleanup.cpp b/src/cleanup.cpp
index cff7d22b23475db2fdb6f2e79d272bac5b7cb36b..f7f465771b9ba5e214476b61e50c127695487b5d 100644 (file)
--- a/src/cleanup.cpp
+++ b/src/cleanup.cpp
@@ -22,7 +22,6 @@
 #include "CacheFile.hpp"
 #include "Config.hpp"
 #include "Context.hpp"
-#include "Lockfile.hpp"
 #include "Logging.hpp"
 #include "Util.hpp"
 #include "stats.hpp"
@@ -61,16 +60,13 @@ update_counters(const std::string& dir,
                 bool cleanup_performed)
 {
   const std::string stats_file = dir + "/stats";
-  Lockfile lock(stats_file);
-  if (lock.acquired()) {
-    Counters counters = Statistics::read(stats_file);
+  Statistics::update(stats_file, [=](Counters& cs) {
     if (cleanup_performed) {
-      counters.increment(Statistic::cleanups_performed);
+      cs.increment(Statistic::cleanups_performed);
     }
-    counters.set(Statistic::files_in_cache, files_in_cache);
-    counters.set(Statistic::cache_size_kibibyte, cache_size / 1024);
-    Statistics::write(stats_file, counters);
-  }
+    cs.set(Statistic::files_in_cache, files_in_cache);
+    cs.set(Statistic::cache_size_kibibyte, cache_size / 1024);
+  });
 }
 
 void

diff --git a/src/compress.cpp b/src/compress.cpp
index 5bed4affd64d851e84585a26f1a1c92c4d2938d1..1326a5fa2bb6eac073bf61e404fe5b94799b2e16 100644 (file)
--- a/src/compress.cpp
+++ b/src/compress.cpp
@@ -204,9 +204,9 @@ recompress_file(Context& ctx,
   if (ctx.stats_file() == stats_file) {
     ctx.counter_updates.increment(Statistic::cache_size_kibibyte, size_delta);
   } else {
-    Counters updates;
-    updates.increment(Statistic::cache_size_kibibyte, size_delta);
-    Statistics::increment(stats_file, updates);
+    Statistics::update(stats_file, [=](Counters& cs) {
+      cs.increment(Statistic::cache_size_kibibyte, size_delta);
+    });
   }
 
   statistics.update(content_size, old_stat.size(), new_stat.size(), 0);

diff --git a/src/stats.cpp b/src/stats.cpp
index 273dbb45372ef31f18227cdc7787e09543c5c020..a6d6129f0d5975dfe6702521930f43407c2953df 100644 (file)
--- a/src/stats.cpp
+++ b/src/stats.cpp
@@ -25,7 +25,6 @@
 #include "AtomicFile.hpp"
 #include "Context.hpp"
 #include "Counters.hpp"
-#include "Lockfile.hpp"
 #include "Logging.hpp"
 #include "Statistics.hpp"
 #include "cleanup.hpp"
@@ -284,17 +283,15 @@ stats_zero(const Context& ctx)
       // No point in trying to reset the stats file if it doesn't exist.
       continue;
     }
-    Lockfile lock(fname);
-    if (lock.acquired()) {
-      Counters counters = Statistics::read(fname);
-      for (size_t i = 0; k_statistics_fields[i].message; i++) {
+
+    Statistics::update(fname, [=](Counters& cs) {
+      for (size_t i = 0; k_statistics_fields[i].message; ++i) {
         if (!(k_statistics_fields[i].flags & FLAG_NOZERO)) {
-          counters.set(k_statistics_fields[i].statistic, 0);
+          cs.set(k_statistics_fields[i].statistic, 0);
         }
       }
-      counters.set(Statistic::stats_zeroed_timestamp, timestamp);
-      Statistics::write(fname, counters);
-    }
+      cs.set(Statistic::stats_zeroed_timestamp, timestamp);
+    });
   }
 }
 

diff --git a/unittest/test_Statistics.cpp b/unittest/test_Statistics.cpp
index 64cd1dca6e8bdecc9f44b196be75ac5fe571d0de..d945848f276e82e65b56578088fcefdfd56bf9af 100644 (file)
--- a/unittest/test_Statistics.cpp
+++ b/unittest/test_Statistics.cpp
@@ -78,40 +78,24 @@ TEST_CASE("Read future counters")
   }
 }
 
-TEST_CASE("Write")
-{
-  TestContext test_context;
-
-  Counters counters;
-  size_t count = static_cast<size_t>(Statistic::END) + 1;
-  for (size_t i = 0; i < count; ++i) {
-    counters.set_raw(i, i);
-  }
-
-  Statistics::write("test", counters);
-  counters = Statistics::read("test");
-
-  REQUIRE(counters.size() == count);
-  for (size_t i = 0; i < count; ++i) {
-    CHECK(counters.get_raw(i) == i);
-  }
-}
-
-TEST_CASE("Increment")
+TEST_CASE("Update")
 {
   TestContext test_context;
 
   Util::write_file("test", "0 1 2 3 27 5\n");
 
-  Counters updates;
-  updates.set(Statistic::internal_error, 1);
-  updates.set(Statistic::cache_miss, 6);
+  auto counters = Statistics::update("test", [](Counters& cs) {
+    cs.increment(Statistic::internal_error, 1);
+    cs.increment(Statistic::cache_miss, 6);
+  });
+  REQUIRE(counters);
 
-  Statistics::increment("test", updates);
+  CHECK(counters->get(Statistic::internal_error) == 4);
+  CHECK(counters->get(Statistic::cache_miss) == 33);
 
-  Counters counters = Statistics::read("test");
-  CHECK(counters.get(Statistic::internal_error) == 4);
-  CHECK(counters.get(Statistic::cache_miss) == 33);
+  counters = Statistics::read("test");
+  CHECK(counters->get(Statistic::internal_error) == 4);
+  CHECK(counters->get(Statistic::cache_miss) == 33);
 }
 
 TEST_SUITE_END();