Save optimization (#2152)

* don't resave saved values if they didn't change

* dont resave settings unless changed

* save mod settings on apply

* set dirty if some settings values are not set in save data; also make settings load fail not skip saved value load

* add taken vs dirty system for perma and temp borrows of saved and settings containers

* add spaces to ifs for hater mat

* remove unused func declaration

* fix taken value not being read in mod setting manager

* getsavecontainertemp can probs be private

* document dirty vs taken for saved values

* also document the thought process in getsavecontainer vs temp

* move to enum, use clearer var names

* fix missing auto&

* rename res2 variable to res

* add queueSave to the one config loading failure mode that didnt have it

Author: Cvolton

loader/include/Geode/loader/Mod.hpp

@@ -82,6 +82,8 @@ namespace geode {
         friend void GEODE_CALL ::geode_implicit_load(Mod*);
 
         void settingReact(geode::Function<void()> fn);
+        
+        matjson::Value& getSaveContainerTemp();
     public:
         // no copying
         Mod(Mod const&) = delete;
@@ -242,6 +244,7 @@ namespace geode {
         }
 
         matjson::Value& getSaveContainer();
+        matjson::Value const& getSaveContainerConst() const;
         matjson::Value& getSavedSettingsData();
 
         /**
@@ -286,7 +289,7 @@ namespace geode {
 
         template <class T>
         T getSavedValue(std::string_view key) {
-            auto& saved = this->getSaveContainer();
+            auto& saved = this->getSaveContainerConst();
             if (auto res = saved.get(key).andThen([](auto&& v) {
                 return v.template as<T>();
             }); res.isOk()) {
@@ -297,13 +300,15 @@ namespace geode {
 
         template <class T>
         T getSavedValue(std::string_view key, T const& defaultValue) {
-            auto& saved = this->getSaveContainer();
+            auto& saved = this->getSaveContainerConst();
             if (auto res = saved.get(key).andThen([](auto&& v) {
                 return v.template as<T>();
             }); res.isOk()) {
                 return res.unwrap();
             }
-            saved[key] = matjson::Value(defaultValue);
+
+            auto& savedMutable = this->getSaveContainerTemp();
+            savedMutable[key] = matjson::Value(defaultValue);
             return defaultValue;
         }
 
@@ -316,8 +321,22 @@ namespace geode {
          */
         template <class T>
         T setSavedValue(std::string_view key, T const& value) {
-            auto& saved = this->getSaveContainer();
             auto old = this->getSavedValue<T>(key);
+
+            // optimization: if the value is the same, don't write to the save container
+            // constexpr checks needed to avoid compile errors for types that don't support operator==
+            if constexpr (std::ranges::range<T>) {
+                using Elem = std::ranges::range_value_t<T>;
+                if constexpr (requires(Elem const& a, Elem const& b) { { a == b } -> std::convertible_to<bool>; }) {
+                    if (old == value) return old;
+                }
+            } 
+            else if constexpr (requires(T const& a, T const& b) { { a == b } -> std::convertible_to<bool>; }) {
+                if (old == value) return old;
+            }
+
+            // value changed, write to save container
+            auto& saved = this->getSaveContainer();
             saved[key] = value;
             return old;
         }

loader/include/Geode/loader/ModSettingsManager.hpp

@@ -16,6 +16,8 @@ namespace geode {
         friend class ::geode::Mod;
 
         void markRestartRequired();
+        void queueSave();
+        void saveFinished();
 
     public:
         static ModSettingsManager* from(Mod* mod);
@@ -67,5 +69,7 @@ namespace geode {
          * for this mod, they are also reloaded for the dependant mods
          */
         void addDependant(Mod* mod);
+
+        bool shouldSave() const;
     };
 }

loader/src/loader/Mod.cpp

@@ -45,6 +45,14 @@ matjson::Value& Mod::getSaveContainer() {
     return m_impl->getSaveContainer();
 }
 
+matjson::Value& Mod::getSaveContainerTemp() {
+    return m_impl->getSaveContainerTemp();
+}
+
+matjson::Value const& Mod::getSaveContainerConst() const {
+    return m_impl->getSaveContainerConst();
+}
+
 matjson::Value& Mod::getSavedSettingsData() {
     return m_impl->m_settings->getSaveData();
 }

loader/src/loader/ModImpl.cpp

@@ -155,6 +155,24 @@ VersionInfo Mod::Impl::getVersion() const {
 }
 
 matjson::Value& Mod::Impl::getSaveContainer() {
+    // saved value taken, so we need to save it every time
+    // since we dont know what the caller will do with the container
+    m_saveRequestState = SaveRequestState::SaveUntilExit;
+
+    return m_saved;
+}
+
+matjson::Value& Mod::Impl::getSaveContainerTemp() {
+    // saved value dirty - caller promises to get rid
+    // of its ref before next save
+    if(m_saveRequestState == SaveRequestState::Clean) {
+        m_saveRequestState = SaveRequestState::SaveOnce;
+    }
+
+    return m_saved;
+}
+
+matjson::Value const& Mod::Impl::getSaveContainerConst() const {
     return m_saved;
 }
 
@@ -170,7 +188,7 @@ bool Mod::Impl::needsEarlyLoad(std::vector<Mod*>& checked) const {
     checked.push_back(m_self);
     if (this->getMetadata().needsEarlyLoad()) return true;
     for (auto& dep : m_dependants) {
-        if(std::find(checked.begin(), checked.end(), dep) != checked.end()) continue;
+        if (std::find(checked.begin(), checked.end(), dep) != checked.end()) continue;
         if (dep->m_impl->needsEarlyLoad(checked)) return true;
     }
     return false;
@@ -199,11 +217,19 @@ Result<> Mod::Impl::loadData() {
     // Check if settings exist
     auto settingPath = m_saveDirPath / "settings.json";
     if (std::filesystem::exists(settingPath)) {
-        GEODE_UNWRAP_INTO(auto json, utils::file::readJson(settingPath));
-        auto load = m_settings->load(json);
-        if (!load) {
-            log::warn("Unable to load settings: {}", load.unwrapErr());
+        if (auto json = utils::file::readJson(settingPath)) {
+            auto load = m_settings->load(json.unwrap());
+            if (!load) {
+                m_settings->queueSave();
+                log::warn("Unable to load settings: {}", load.unwrapErr());
+            }
+        } else {
+            // this used to early return but skipping saved values is not great behavior here imo
+            m_settings->queueSave();
+            log::warn("Unable to load settings: {}", json.unwrapErr());
         }
+    } else {
+        m_settings->queueSave();
     }
 
     // Saved values
@@ -233,18 +259,36 @@ Result<> Mod::Impl::saveData() {
     }
 
     // ModSettingsManager keeps track of the whole savedata
-    matjson::Value json = m_settings->save();
+    if (m_settings->shouldSave()) {
+        log::debug("Saving settings for mod {}", m_metadata.getID());
 
-    // saveData is expected to be synchronous, and always called from GD thread
-    ModStateEvent(ModEventType::DataSaved, std::move(m_self)).send();
+        matjson::Value json = m_settings->save();
 
-    auto res = utils::file::writeStringSafe(m_saveDirPath / "settings.json", json.dump());
-    if (!res) {
-        log::error("Unable to save settings: {}", res.unwrapErr());
+        // saveData is expected to be synchronous, and always called from GD thread
+        ModStateEvent(ModEventType::DataSaved, std::move(m_self)).send();
+
+        auto res = utils::file::writeStringSafe(m_saveDirPath / "settings.json", json.dump());
+        if (!res) {
+            log::error("Unable to save settings: {}", res.unwrapErr());
+        } else {
+            m_settings->saveFinished();
+        }
+    } else {
+        // duplicated line to retain old expectations of saveData being called after json dump but before file write
+        ModStateEvent(ModEventType::DataSaved, std::move(m_self)).send();
     }
-    auto res2 = utils::file::writeStringSafe(m_saveDirPath / "saved.json", m_saved.dump());
-    if (!res2) {
-        log::error("Unable to save values: {}", res2.unwrapErr());
+
+    if (m_saveRequestState != SaveRequestState::Clean) {
+        log::debug("Saving values for mod {}", m_metadata.getID());
+
+        auto res = utils::file::writeStringSafe(m_saveDirPath / "saved.json", m_saved.dump());
+        if (!res) {
+            log::error("Unable to save values: {}", res.unwrapErr());
+        }
+
+        if(m_saveRequestState == SaveRequestState::SaveOnce) {
+            m_saveRequestState = SaveRequestState::Clean;
+        }
     }
 
     return Ok();

loader/src/loader/ModImpl.hpp

@@ -8,6 +8,12 @@
 #include <Geode/loader/Signal.hpp>
 
 namespace geode {
+    enum class SaveRequestState {
+        Clean,
+        SaveOnce,
+        SaveUntilExit,
+    };
+
     class Mod::Impl {
     public:
         Mod* m_self;
@@ -49,6 +55,11 @@ namespace geode {
          * Saved values
          */
         matjson::Value m_saved = matjson::Value();
+        /**
+         * Whether the saved values need to be saved to disk one time
+         * (container dirty)
+         */
+        SaveRequestState m_saveRequestState = SaveRequestState::Clean;
         /**
          * Setting values. This is behind unique_ptr for interior mutability
          */
@@ -105,6 +116,8 @@ namespace geode {
         bool isEphemeral() const;
 
         matjson::Value& getSaveContainer();
+        matjson::Value& getSaveContainerTemp();
+        matjson::Value const& getSaveContainerConst() const;
 
 #if defined(GEODE_EXPOSE_SECRET_INTERNALS_IN_HEADERS_DO_NOT_DEFINE_PLEASE)
         void setMetadata(ModMetadata const& metadata);

loader/src/loader/ModSettingsManager.cpp

@@ -188,6 +188,7 @@ class ModSettingsManager::Impl final {
     // update this by calling saveSettingValueToSave
     matjson::Value savedata;
     bool restartRequired = false;
+    SaveRequestState saveRequestState = SaveRequestState::Clean;
 
     bool loadSettingValueFromSave(std::string const& key) {
         if (this->savedata.contains(key) && this->settings.contains(key)) {
@@ -205,6 +206,12 @@ class ModSettingsManager::Impl final {
             return true;
         }
         else {
+            if (!this->savedata.contains(key)) {
+                log::error("Unable to load setting '{}' for mod {} (not found in savedata)", key, this->modID);
+                if(saveRequestState == SaveRequestState::Clean) {
+                    saveRequestState = SaveRequestState::SaveOnce;
+                }
+            }
             return false;
         }
     }
@@ -237,7 +244,9 @@ class ModSettingsManager::Impl final {
             }
             if (auto v3 = (*gen)(key, modID, setting.json)) {
                 setting.v3 = v3.unwrap();
-                this->loadSettingValueFromSave(key);
+
+                // the loading is unnecessary, m_saveData is not initialized yet
+                // this->loadSettingValueFromSave(key);
             }
             else {
                 log::error(
@@ -280,6 +289,22 @@ void ModSettingsManager::markRestartRequired() {
     m_impl->restartRequired = true;
 }
 
+void ModSettingsManager::queueSave() {
+    if(m_impl->saveRequestState == SaveRequestState::Clean) {
+        m_impl->saveRequestState = SaveRequestState::SaveOnce;
+    }
+}
+
+void ModSettingsManager::saveFinished() {
+    if(m_impl->saveRequestState == SaveRequestState::SaveOnce) {
+        m_impl->saveRequestState = SaveRequestState::Clean;
+    }
+}
+
+bool ModSettingsManager::shouldSave() const {
+    return m_impl->saveRequestState != SaveRequestState::Clean;
+}
+
 Result<> ModSettingsManager::registerCustomSettingType(std::string_view type, SettingGenerator generator) {
     GEODE_UNWRAP(SharedSettingTypesPool::get().add(m_impl->modID, type, std::move(generator)));
     m_impl->createSettings();
@@ -310,6 +335,14 @@ Result<> ModSettingsManager::load(matjson::Value const& json) {
                 }
             }
         }
+
+        for (auto const& [key, _] : m_impl->settings) {
+            if (!json.contains(key)) {
+                log::error("Unable to load setting '{}' for mod {} (not found in savedata)", key, m_impl->modID);
+                this->queueSave();
+                break;
+            }
+        }
     }
     return Ok();
 }
@@ -323,6 +356,8 @@ matjson::Value ModSettingsManager::save() {
 }
 
 matjson::Value& ModSettingsManager::getSaveData() {
+    m_impl->saveRequestState = SaveRequestState::SaveUntilExit;
+
     return m_impl->savedata;
 }
 

loader/src/loader/SettingV3.cpp

@@ -575,8 +575,11 @@ Mod* SettingV3::getMod() const {
 
 void SettingV3::markChanged() {
     auto manager = ModSettingsManager::from(this->getMod());
-    if (m_impl->requiresRestart) {
-        manager->markRestartRequired();
+    if (manager) {
+        if (m_impl->requiresRestart) {
+            manager->markRestartRequired();
+        }
+        manager->queueSave();
     }
     SettingChangedEventV3(this->getModID(), this->getKey()).send(shared_from_this());
 }

loader/src/ui/mods/settings/ModSettingsPopup.cpp

@@ -65,7 +65,16 @@ bool ModSettingsPopup::init(Mod* mod, bool forceDisableTheme) {
 
 void ModSettingsPopup::updateState(SettingNode* invoker) {
     BaseSettingsPopup::updateState(invoker);
-    m_restartBtn->setVisible(ModSettingsManager::from(m_mod)->restartRequired());
+
+    auto manager = ModSettingsManager::from(m_mod);
+    m_restartBtn->setVisible(manager->restartRequired());
+
+    // frame delay for debounce (avoids repeating save for every changed setting)
+    Loader::get()->queueInMainThread([mod = m_mod, manager] {
+        if (manager->shouldSave()) {
+            (void) mod->saveData();
+        }
+    });
 }
 
 void ModSettingsPopup::onOpenSaveDirectory(CCObject*) {