avoid writing the datastore file when values haven't changed

datastore.cpp

@@ -34,11 +34,11 @@ using namespace error;
 
 static string FilePath(const string& file_root, const string& suffix);
 static Result<json> FileLoad(const string& file_path);
-static Error FileStore(int transaction_depth, const string& file_path, const json& json);
+static Error FileStore(const string& file_path, const json& json);
 
 Datastore::Datastore()
         : initialized_(false), explicit_lock_(mutex_, std::defer_lock),
-          transaction_depth_(0), json_(json::object()) {
+          transaction_depth_(0), transaction_dirty_(false), json_(json::object()) {
 }
 
 Error Datastore::Init(const string& file_root, const string& suffix) {
@@ -58,7 +58,8 @@ Error Datastore::Init(const string& file_root, const string& suffix) {
 Error Datastore::Reset(const string& file_path, json new_value) {
     SYNCHRONIZE(mutex_);
     transaction_depth_ = 0;
-    if (auto err = FileStore(transaction_depth_, file_path, new_value)) {
+    transaction_dirty_ = false;
+    if (auto err = FileStore(file_path, new_value)) {
         return PassError(err);
     }
     json_ = new_value;
@@ -80,6 +81,7 @@ void Datastore::BeginTransaction() {
     SYNCHRONIZE(mutex_);
     // We got a local lock, so we know there's no transaction in progress in any other thread.
     if (transaction_depth_ == 0) {
+        transaction_dirty_ = false;
         explicit_lock_.lock();
     }
     transaction_depth_++;
@@ -100,12 +102,18 @@ Error Datastore::EndTransaction(bool commit) {
         return nullerr;
     }
 
-    // We need to release the explicit lock on exit from ths function, no matter what.
+    // We need to release the explicit lock on exit from this function, no matter what.
     // We will "adopt" the lock into this lock_guard to ensure the unlock happens when it goes out of scope.
     std::lock_guard<std::unique_lock<std::recursive_mutex>> lock_releaser(explicit_lock_, std::adopt_lock);
 
+    if (!transaction_dirty_) {
+        // No actual substantive changes were made during this transaction, so we will avoid
+        // writing to disk. Committing and rolling back are no-ops if there are no changes.
+        return nullerr;
+    }
+
     if (commit) {
-        return PassError(FileStore(transaction_depth_, file_path_, json_));
+        return PassError(FileStore(file_path_, json_));
     }
 
     // We're rolling back -- revert to what's on disk
@@ -126,8 +134,24 @@ error::Result<nlohmann::json> Datastore::Get() const {
 Error Datastore::Set(const json::json_pointer& p, json v) {
     SYNCHRONIZE(mutex_);
     MUST_BE_INITIALIZED;
+
+    // We will use the transaction mechanism to do the writing. It will also help prevent
+    // changes to the stored value between the time we check it and the time we set it.
+    BeginTransaction();
+
+    // Avoid modifying the datastore if the value is the same as what's already there.
+    bool changed = true;
+    try {
+        changed = (json_.at(p) != v);
+    }
+    catch (json::out_of_range&) {
+        // The key doesn't exist, so continue to set it.
+    }
+
     json_[p] = v;
-    return PassError(FileStore(transaction_depth_, file_path_, json_));
+    transaction_dirty_ = changed;
+
+    return PassError(EndTransaction(true));
 }
 
 static string FilePath(const string& file_root, const string& suffix) {
@@ -171,7 +195,7 @@ static Result<json> FileLoad(const string& file_path) {
     if (!utils::FileExists(file_path)) {
         // Check that we can write here by trying to store.
         auto empty_object = json::object();
-        if (auto err = FileStore(false, file_path, empty_object)) {
+        if (auto err = FileStore(file_path, empty_object)) {
             return WrapError(err, "file doesn't exist and FileStore failed");
         }
 
@@ -203,11 +227,7 @@ static Result<json> FileLoad(const string& file_path) {
     return json;
 }
 
-static Error FileStore(int transaction_depth, const string& file_path, const json& json) {
-    if (transaction_depth > 0) {
-        return nullerr;
-    }
-
+static Error FileStore(const string& file_path, const json& json) {
     const auto temp_file_path = file_path + TEMP_EXT;
     const auto commit_file_path = file_path + COMMIT_EXT;
 

datastore.hpp

@@ -66,8 +66,8 @@ class Datastore {
     /// EndTransaction is called. Transactions are re-enterable, but not nested.
     /// NOTE: Failing to call EndTransaction will result in undefined behaviour.
     void BeginTransaction();
-    /// Ends an ongoing transaction writing. If commit is true, it writes the changes
-    /// immediately; if false it discards the changes.
+    /// Ends an ongoing transaction. If commit is true, it writes the changes immediately;
+    /// if false it discards the changes.
     /// Committing or rolling back inner transactions does nothing. Any errors during
     /// inner transactions that require the outermost transaction to be rolled back must
     /// be handled by the caller.
@@ -121,6 +121,7 @@ class Datastore {
     mutable std::recursive_mutex mutex_;
     std::unique_lock<std::recursive_mutex> explicit_lock_;
     int transaction_depth_;
+    bool transaction_dirty_;
 
     std::string file_path_;
     json json_;

datastore_test.cpp

@@ -20,6 +20,7 @@
 #include <cstdlib>
 #include <ctime>
 #include <thread>
+#include <filesystem>
 
 #include "gtest/gtest.h"
 #include "test_helpers.hpp"
@@ -511,6 +512,100 @@ TEST_F(TestDatastore, SetTypes)
     ASSERT_EQ(*gotInt, wantInt);
 }
 
+TEST_F(TestDatastore, SetWriteDedup)
+{
+    auto temp_dir = GetTempDir();
+    auto ds_path = DatastoreFilepath(temp_dir, ds_suffix);
+
+    Datastore ds;
+    auto err = ds.Init(temp_dir.c_str(), ds_suffix);
+    ASSERT_FALSE(err);
+
+    auto k = "/k"_json_pointer;
+    err = ds.Set(k, "a");
+    ASSERT_FALSE(err);
+    auto file_time1 = std::filesystem::last_write_time(ds_path);
+
+    // Try setting the same value again
+    err = ds.Set(k, "a");
+    ASSERT_FALSE(err);
+    auto file_time2 = std::filesystem::last_write_time(ds_path);
+
+    // The file time should not have changed after setting the same value
+    ASSERT_EQ(file_time1, file_time2);
+
+    // Change to a different value
+    err = ds.Set(k, "b");
+    ASSERT_FALSE(err);
+    auto file_time3 = std::filesystem::last_write_time(ds_path);
+
+    // The file should have been updated, so its time should be newer
+    ASSERT_GT(file_time3, file_time1);
+}
+
+TEST_F(TestDatastore, TransactionWriteDedup)
+{
+    auto temp_dir = GetTempDir();
+    auto ds_path = DatastoreFilepath(temp_dir, ds_suffix);
+
+    Datastore ds;
+    auto err = ds.Init(temp_dir.c_str(), ds_suffix);
+    ASSERT_FALSE(err);
+
+    auto k1 = "/k1"_json_pointer;
+    auto k2 = "/k2"_json_pointer;
+    err = ds.Set(k1, "a");
+    ASSERT_FALSE(err);
+    err = ds.Set(k2, "a");
+    ASSERT_FALSE(err);
+    auto file_time1 = std::filesystem::last_write_time(ds_path);
+
+    ds.BeginTransaction();
+
+    // Try setting the same values again
+    err = ds.Set(k1, "a");
+    ASSERT_FALSE(err);
+    err = ds.Set(k2, "a");
+    ASSERT_FALSE(err);
+
+    err = ds.EndTransaction(true);
+    ASSERT_FALSE(err);
+    auto file_time2 = std::filesystem::last_write_time(ds_path);
+
+    // The file time should not have changed after setting the same values
+    ASSERT_EQ(file_time1, file_time2);
+
+    ds.BeginTransaction();
+
+    // Change to a different value
+    err = ds.Set(k1, "b");
+    ASSERT_FALSE(err);
+    err = ds.Set(k2, "b");
+    ASSERT_FALSE(err);
+
+    err = ds.EndTransaction(true);
+    ASSERT_FALSE(err);
+    auto file_time3 = std::filesystem::last_write_time(ds_path);
+
+    // The file should have been updated, so its time should be newer
+    ASSERT_GT(file_time3, file_time1);
+
+    ds.BeginTransaction();
+
+    // Changing and then rolling back should still work as expected
+    err = ds.Set(k1, "c");
+    ASSERT_FALSE(err);
+    err = ds.Set(k2, "c");
+    ASSERT_FALSE(err);
+
+    err = ds.EndTransaction(false);
+    ASSERT_FALSE(err);
+    auto file_time4 = std::filesystem::last_write_time(ds_path);
+
+    // Should not have changed
+    ASSERT_EQ(file_time3, file_time4);
+}
+
 TEST_F(TestDatastore, TypeMismatch)
 {
     Datastore ds;

test_helpers.hpp

@@ -68,8 +68,16 @@ class TempDir
         return dev ? ".dev" : ".prod";
     }
 
+    std::string DatastoreFilepath(const std::string& datastore_root, const char* suffix) {
+        return datastore_root + "/psicashdatastore" + suffix;
+    }
+
+    std::string DatastoreFilepath(const std::string& datastore_root, const std::string& suffix) {
+        return DatastoreFilepath(datastore_root, suffix.c_str());
+    }
+
     std::string DatastoreFilepath(const std::string& datastore_root, bool dev) {
-        return datastore_root + "/psicashdatastore" + GetSuffix(dev);
+        return DatastoreFilepath(datastore_root, GetSuffix(dev));
     }
 
     bool Write(const std::string& datastore_root, bool dev, const std::string& s) {