Merge pull request #16 from adam-p/dedup-writes

Reduce datastore writes

Author: adam-p

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;

psicash.cpp

@@ -161,9 +161,17 @@ void PsiCash::SetHTTPRequestFn(MakeHTTPRequestFn make_http_request_fn) {
     make_http_request_fn_ = std::move(make_http_request_fn);
 }
 
-Error PsiCash::SetRequestMetadataItem(const string& key, const string& value) {
+Error PsiCash::SetRequestMetadataItems(const std::map<std::string, std::string>& items) {
     MUST_BE_INITIALIZED;
-    return PassError(user_data_->SetRequestMetadataItem(key, value));
+    UserData::Transaction transaction(*user_data_);
+    for (const auto& it : items) {
+        // Errors won't manifest until we commit
+        (void)user_data_->SetRequestMetadataItem(it.first, it.second);
+    }
+    if (auto err = transaction.Commit()) {
+        return WrapError(err, "user data write failed");
+    }
+    return nullerr;
 }
 
 Error PsiCash::SetLocale(const string& locale) {

psicash.hpp

@@ -194,7 +194,7 @@ class PsiCash {
 
     /// Set values that will be included in the request metadata. This includes
     /// client_version, client_region, sponsor_id, and propagation_channel_id.
-    error::Error SetRequestMetadataItem(const std::string& key, const std::string& value);
+    error::Error SetRequestMetadataItems(const std::map<std::string, std::string>& items);
 
     /// Set current UI locale.
     error::Error SetLocale(const std::string& locale);

psicash_test.cpp

@@ -279,19 +279,27 @@ TEST_F(TestPsiCash, SetHTTPRequestFn) {
     }
 }
 
-TEST_F(TestPsiCash, SetRequestMetadataItem) {
+TEST_F(TestPsiCash, SetRequestMetadataItems) {
     PsiCashTester pc;
     auto err = pc.Init(TestPsiCash::UserAgent(), GetTempDir().c_str(), nullptr, false);
     ASSERT_FALSE(err);
 
     auto j = pc.user_data().GetRequestMetadata();
     ASSERT_EQ(j.size(), 0);
 
-    err = pc.SetRequestMetadataItem("k", "v");
+    err = pc.SetRequestMetadataItems({{"k", "v"}});
     ASSERT_FALSE(err);
 
     j = pc.user_data().GetRequestMetadata();
     ASSERT_EQ(j["k"], "v");
+
+    err = pc.SetRequestMetadataItems({{"a", "b"}, {"x", "y"}});
+    ASSERT_FALSE(err);
+
+    j = pc.user_data().GetRequestMetadata();
+    ASSERT_EQ(j["k"], "v");
+    ASSERT_EQ(j["a"], "b");
+    ASSERT_EQ(j["x"], "y");
 }
 
 TEST_F(TestPsiCash, SetLocale) {
@@ -934,15 +942,15 @@ TEST_F(TestPsiCash, ModifyLandingPage) {
     // With metadata
     //
 
-    err = pc.SetRequestMetadataItem("k", "v");
+    err = pc.SetRequestMetadataItems({{"k", "v"}, {"x", "y"}});
     ASSERT_FALSE(err);
     url_in = {"https://asdf.sadf.gf", "", ""};
     res = pc.ModifyLandingPage(url_in.ToString());
     ASSERT_TRUE(res);
     url_out.Parse(*res);
     ASSERT_EQ(url_out.scheme_host_path_, url_in.scheme_host_path_);
     ASSERT_EQ(url_out.fragment_, url_in.fragment_);
-    ASSERT_THAT(TokenPayloadsMatch(url_out.query_.substr(key_part.length()), R"({"metadata":{"k":"v"},"tokens":"kEarnerTokenType"})"_json), IsEmpty());
+    ASSERT_THAT(TokenPayloadsMatch(url_out.query_.substr(key_part.length()), R"({"metadata":{"k":"v","x":"y"},"tokens":"kEarnerTokenType"})"_json), IsEmpty());
 
     //
     // Errors
@@ -1077,7 +1085,7 @@ TEST_F(TestPsiCash, GetRewardedActivityData) {
     ASSERT_TRUE(res);
     ASSERT_EQ(*res, base64::B64Encode(utils::Stringer(R"({"metadata":{"user_agent":")", TestPsiCash::UserAgent(), R"(","v":1},"tokens":"kEarnerTokenType","v":1})")));
 
-    err = pc.SetRequestMetadataItem("k", "v");
+    err = pc.SetRequestMetadataItems({{"k", "v"}});
     ASSERT_FALSE(err);
     res = pc.GetRewardedActivityData();
     ASSERT_TRUE(res);

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) {