From e6ab9a88ce980eacc2347fc4134b515263dfc771 Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Tue, 18 Apr 2023 13:40:25 -0700 Subject: [PATCH] Remove locks from settings Settings should be initialized on start-up. This removes the option of lazily loading the settings from file and instead requires all settings needed to be loaded on KiFACE start before requesting data from the settings object --- eeschema/CMakeLists.txt | 1 + eeschema/eeschema.cpp | 8 +++++++ eeschema/eeschema_settings.cpp | 2 +- include/settings/settings_manager.h | 35 +++-------------------------- pcbnew/pcbnew.cpp | 9 +++++++- pcbnew/pcbnew_settings.cpp | 4 ++-- 6 files changed, 23 insertions(+), 36 deletions(-) diff --git a/eeschema/CMakeLists.txt b/eeschema/CMakeLists.txt index 7522daede6..5161d7f936 100644 --- a/eeschema/CMakeLists.txt +++ b/eeschema/CMakeLists.txt @@ -33,6 +33,7 @@ include_directories( ${CMAKE_SOURCE_DIR}/common ${CMAKE_SOURCE_DIR}/common/dialogs ${CMAKE_SOURCE_DIR}/libs/sexpr/include + ${CMAKE_SOURCE_DIR}/3d-viewer ${INC_AFTER} ./dialogs ./libview diff --git a/eeschema/eeschema.cpp b/eeschema/eeschema.cpp index ff5830e0f0..c2c98a8246 100644 --- a/eeschema/eeschema.cpp +++ b/eeschema/eeschema.cpp @@ -61,6 +61,9 @@ #include #include +#include "../3d-viewer/3d_viewer/eda_3d_viewer_settings.h" +#include "../pcbnew/pcbnew_settings.h" + #include // The main sheet of the project @@ -337,6 +340,11 @@ bool IFACE::OnKifaceStart( PGM_BASE* aProgram, int aCtlBits ) InitSettings( new EESCHEMA_SETTINGS ); aProgram->GetSettingsManager().RegisterSettings( KifaceSettings() ); + // Register the symbol editor settings as well because they share a KiFACE and need to be + // loaded prior to use to avoid threading deadlocks + aProgram->GetSettingsManager().RegisterSettings( new SYMBOL_EDITOR_SETTINGS, false ); + aProgram->GetSettingsManager().RegisterSettings( new SYMBOL_EDITOR_SETTINGS, false ); + start_common( aCtlBits ); if( !loadGlobalLibTable() ) diff --git a/eeschema/eeschema_settings.cpp b/eeschema/eeschema_settings.cpp index bdbe80088b..aab2a25505 100644 --- a/eeschema/eeschema_settings.cpp +++ b/eeschema/eeschema_settings.cpp @@ -774,7 +774,7 @@ bool EESCHEMA_SETTINGS::MigrateFromLegacy( wxConfigBase* aCfg ) // LibEdit settings were stored with eeschema. If eeschema is the first app to run, // we need to migrate the LibEdit settings here - auto libedit = Pgm().GetSettingsManager().GetAppSettings( false ); + auto libedit = Pgm().GetSettingsManager().GetAppSettings(); libedit->MigrateFromLegacy( aCfg ); libedit->Load(); diff --git a/include/settings/settings_manager.h b/include/settings/settings_manager.h index edf8dde57b..6b98611fdd 100644 --- a/include/settings/settings_manager.h +++ b/include/settings/settings_manager.h @@ -23,7 +23,6 @@ #include #include -#include #include #include // for wxString hash #include @@ -68,11 +67,7 @@ public: template T* RegisterSettings( T* aSettings, bool aLoadNow = true ) { - // Throws a low chance of conflict with GetAppSettings in threads but let's lock anyway - std::unique_lock writeLock( m_settings_mutex ); - - return static_cast( - registerSettings( aSettings, aLoadNow ) ); + return static_cast( registerSettings( aSettings, aLoadNow ) ); } void Load(); @@ -100,32 +95,17 @@ public: * @return a pointer to a loaded settings object */ template - T* GetAppSettings( bool aLoadNow = true ) + T* GetAppSettings() { T* ret = nullptr; size_t typeHash = typeid( T ).hash_code(); - std::shared_lock readLock( m_settings_mutex ); - if( m_app_settings_cache.count( typeHash ) ) ret = dynamic_cast( m_app_settings_cache.at( typeHash ) ); if( ret ) return ret; - // cache lookup failed, now we shift to an exclusive lock to update the cache - readLock.unlock(); - std::unique_lock writeLock( m_settings_mutex ); - - // In case multiple threads ended up waiting to unique lock to handle the same setting object - // This happens with library loading for example - if( m_app_settings_cache.count( typeHash ) ) - { - T* recheck = dynamic_cast( m_app_settings_cache.at( typeHash ) ); - if( recheck ) - return recheck; - } - auto it = std::find_if( m_settings.begin(), m_settings.end(), []( const std::unique_ptr& aSettings ) { @@ -138,13 +118,7 @@ public: } else { - try - { - ret = static_cast( registerSettings( new T, aLoadNow ) ); - } - catch( ... ) - { - } + throw std::runtime_error( "Tried to GetAppSettings before registering" ); } m_app_settings_cache[typeHash] = ret; @@ -475,9 +449,6 @@ private: /// Lock for loaded project (expand to multiple once we support MDI) std::unique_ptr m_project_lock; - /// Mutex to protect read/write access to m_settings and m_app_settings_cache - std::shared_mutex m_settings_mutex; - static wxString backupDateTimeFormat; }; diff --git a/pcbnew/pcbnew.cpp b/pcbnew/pcbnew.cpp index 3bb656a404..c60498cf79 100644 --- a/pcbnew/pcbnew.cpp +++ b/pcbnew/pcbnew.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -349,6 +350,12 @@ bool IFACE::OnKifaceStart( PGM_BASE* aProgram, int aCtlBits ) InitSettings( new PCBNEW_SETTINGS ); aProgram->GetSettingsManager().RegisterSettings( KifaceSettings() ); + // Register the footprint editor settings as well because they share a KiFACE and need to be + // loaded prior to use to avoid threading deadlocks + aProgram->GetSettingsManager().RegisterSettings( new FOOTPRINT_EDITOR_SETTINGS ); + aProgram->GetSettingsManager().RegisterSettings( new CVPCB_SETTINGS ); + aProgram->GetSettingsManager().RegisterSettings( new EDA_3D_VIEWER_SETTINGS ); + start_common( aCtlBits ); if( !loadGlobalLibTable() ) @@ -505,4 +512,4 @@ void IFACE::SaveFileAs( const wxString& aProjectBasePath, const wxString& aSrcPr int IFACE::HandleJob( JOB* aJob ) { return m_jobHandler->RunJob( aJob ); -} \ No newline at end of file +} diff --git a/pcbnew/pcbnew_settings.cpp b/pcbnew/pcbnew_settings.cpp index 88519ee93f..1ae99836a4 100644 --- a/pcbnew/pcbnew_settings.cpp +++ b/pcbnew/pcbnew_settings.cpp @@ -911,12 +911,12 @@ bool PCBNEW_SETTINGS::MigrateFromLegacy( wxConfigBase* aCfg ) } // Footprint editor settings were stored in pcbnew config file. Migrate them here. - auto fpedit = Pgm().GetSettingsManager().GetAppSettings( false ); + auto fpedit = Pgm().GetSettingsManager().GetAppSettings(); fpedit->MigrateFromLegacy( aCfg ); fpedit->Load(); // Same with 3D viewer - auto viewer3d = Pgm().GetSettingsManager().GetAppSettings( false ); + auto viewer3d = Pgm().GetSettingsManager().GetAppSettings(); viewer3d->MigrateFromLegacy( aCfg ); viewer3d->Load();