From d33f5ef0c0f88754cb895baa260ab6865e99c864 Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Tue, 20 Aug 2024 17:09:50 +0200 Subject: [PATCH] Don't generate a mutex for each Locked, share one per object This reduces RAM usage per object by sizeof(mutex)*(FIELDS-1). --- lib/base/atomic.hpp | 75 ++++++++++++++++++++++++++------- lib/base/configobject.ti | 2 +- lib/icinga/host.ti | 2 +- lib/icinga/hostgroup.ti | 2 +- lib/icinga/service.ti | 2 +- lib/icinga/servicegroup.ti | 2 +- lib/icinga/timeperiod.ti | 2 +- lib/icinga/user.ti | 2 +- lib/icinga/usergroup.ti | 2 +- lib/icingadb/icingadb.cpp | 4 +- lib/icingadb/icingadb.hpp | 2 +- tools/mkclass/classcompiler.cpp | 14 ++++-- 12 files changed, 81 insertions(+), 30 deletions(-) diff --git a/lib/base/atomic.hpp b/lib/base/atomic.hpp index c8f169c0b72..0e731326bf6 100644 --- a/lib/base/atomic.hpp +++ b/lib/base/atomic.hpp @@ -41,6 +41,8 @@ class Atomic : public std::atomic { } }; +class LockedMutex; + /** * Wraps any T into a std::atomic-like interface that locks using a mutex. * @@ -53,27 +55,70 @@ template class Locked { public: - inline T load() const - { - std::unique_lock lock(m_Mutex); + T load(LockedMutex& mtx) const; + void store(T desired, LockedMutex& mtx); - return m_Value; - } +private: + T m_Value; +}; - inline void store(T desired) - { - std::unique_lock lock(m_Mutex); +/** + * Wraps std::mutex, so that only Locked can (un)lock it. + * + * The latter tiny lock scope is enforced this way to prevent deadlocks while passing around mutexes. + * + * @ingroup base + */ +class LockedMutex +{ + template + friend class Locked; + +private: + std::mutex m_Mutex; +}; + +template +T Locked::load(LockedMutex& mtx) const +{ + std::unique_lock lock (mtx.m_Mutex); - m_Value = std::move(desired); + return m_Value; +} + +template +void Locked::store(T desired, LockedMutex& mtx) +{ + std::unique_lock lock (mtx.m_Mutex); + + m_Value = std::move(desired); +} + +/** + * Extends std::atomic with a Locked-like interface. + * + * @ingroup base + */ +template +class AtomicPseudoLocked : public std::atomic { +public: + using std::atomic::atomic; + using std::atomic::load; + using std::atomic::store; + + T load(LockedMutex&) const + { + return load(); } -private: - mutable std::mutex m_Mutex; - T m_Value; + void store(T desired, LockedMutex&) + { + store(desired); + } }; /** - * Type alias for std::atomic if possible, otherwise Locked is used as a fallback. + * Type alias for AtomicPseudoLocked if possible, otherwise Locked is used as a fallback. * * @ingroup base */ @@ -81,9 +126,9 @@ template using AtomicOrLocked = #if defined(__GNUC__) && __GNUC__ < 5 // GCC does not implement std::is_trivially_copyable until version 5. - typename std::conditional::value || std::is_pointer::value, std::atomic, Locked>::type; + typename std::conditional::value || std::is_pointer::value, AtomicPseudoLocked, Locked>::type; #else /* defined(__GNUC__) && __GNUC__ < 5 */ - typename std::conditional::value, std::atomic, Locked>::type; + typename std::conditional::value, AtomicPseudoLocked, Locked>::type; #endif /* defined(__GNUC__) && __GNUC__ < 5 */ } diff --git a/lib/base/configobject.ti b/lib/base/configobject.ti index ea67dfa7ba8..419140ead57 100644 --- a/lib/base/configobject.ti +++ b/lib/base/configobject.ti @@ -59,7 +59,7 @@ abstract class ConfigObject : ConfigObjectBase < ConfigType [config, no_user_modify] String __name (Name); [config, no_user_modify, required] String "name" (ShortName) { get {{{ - String shortName = m_ShortName.load(); + String shortName = m_ShortName.load(m_FieldsMutex); if (shortName.IsEmpty()) return GetName(); else diff --git a/lib/icinga/host.ti b/lib/icinga/host.ti index f6624e30764..f49c33aaf7a 100644 --- a/lib/icinga/host.ti +++ b/lib/icinga/host.ti @@ -21,7 +21,7 @@ class Host : Checkable [config] String display_name { get {{{ - String displayName = m_DisplayName.load(); + String displayName = m_DisplayName.load(m_FieldsMutex); if (displayName.IsEmpty()) return GetName(); else diff --git a/lib/icinga/hostgroup.ti b/lib/icinga/hostgroup.ti index b679344aabd..e1ed94f118e 100644 --- a/lib/icinga/hostgroup.ti +++ b/lib/icinga/hostgroup.ti @@ -11,7 +11,7 @@ class HostGroup : CustomVarObject { [config] String display_name { get {{{ - String displayName = m_DisplayName.load(); + String displayName = m_DisplayName.load(m_FieldsMutex); if (displayName.IsEmpty()) return GetName(); else diff --git a/lib/icinga/service.ti b/lib/icinga/service.ti index 12c2d8c66c9..51343c6d141 100644 --- a/lib/icinga/service.ti +++ b/lib/icinga/service.ti @@ -33,7 +33,7 @@ class Service : Checkable < ServiceNameComposer [config] String display_name { get {{{ - String displayName = m_DisplayName.load(); + String displayName = m_DisplayName.load(m_FieldsMutex); if (displayName.IsEmpty()) return GetShortName(); else diff --git a/lib/icinga/servicegroup.ti b/lib/icinga/servicegroup.ti index 7daf9d419b3..48f19f840b0 100644 --- a/lib/icinga/servicegroup.ti +++ b/lib/icinga/servicegroup.ti @@ -11,7 +11,7 @@ class ServiceGroup : CustomVarObject { [config] String display_name { get {{{ - String displayName = m_DisplayName.load(); + String displayName = m_DisplayName.load(m_FieldsMutex); if (displayName.IsEmpty()) return GetName(); else diff --git a/lib/icinga/timeperiod.ti b/lib/icinga/timeperiod.ti index bba272e814e..5ec0d594b82 100644 --- a/lib/icinga/timeperiod.ti +++ b/lib/icinga/timeperiod.ti @@ -12,7 +12,7 @@ class TimePeriod : CustomVarObject { [config] String display_name { get {{{ - String displayName = m_DisplayName.load(); + String displayName = m_DisplayName.load(m_FieldsMutex); if (displayName.IsEmpty()) return GetName(); else diff --git a/lib/icinga/user.ti b/lib/icinga/user.ti index 8b8c43a14d4..a83c39c94cc 100644 --- a/lib/icinga/user.ti +++ b/lib/icinga/user.ti @@ -13,7 +13,7 @@ class User : CustomVarObject { [config] String display_name { get {{{ - String displayName = m_DisplayName.load(); + String displayName = m_DisplayName.load(m_FieldsMutex); if (displayName.IsEmpty()) return GetName(); else diff --git a/lib/icinga/usergroup.ti b/lib/icinga/usergroup.ti index e955c5e5ed1..37b2b3f7e5d 100644 --- a/lib/icinga/usergroup.ti +++ b/lib/icinga/usergroup.ti @@ -11,7 +11,7 @@ class UserGroup : CustomVarObject { [config] String display_name { get {{{ - String displayName = m_DisplayName.load(); + String displayName = m_DisplayName.load(m_FieldsMutex); if (displayName.IsEmpty()) return GetName(); else diff --git a/lib/icingadb/icingadb.cpp b/lib/icingadb/icingadb.cpp index 6d5ded9cfa9..d684a02cf38 100644 --- a/lib/icingadb/icingadb.cpp +++ b/lib/icingadb/icingadb.cpp @@ -32,7 +32,7 @@ REGISTER_TYPE(IcingaDB); IcingaDB::IcingaDB() : m_Rcon(nullptr) { - m_RconLocked.store(nullptr); + m_RconLocked.store(nullptr, m_FieldsMutex); m_WorkQueue.SetName("IcingaDB"); @@ -80,7 +80,7 @@ void IcingaDB::Start(bool runtimeCreated) m_Rcon = new RedisConnection(GetHost(), GetPort(), GetPath(), GetPassword(), GetDbIndex(), GetEnableTls(), GetInsecureNoverify(), GetCertPath(), GetKeyPath(), GetCaPath(), GetCrlPath(), GetTlsProtocolmin(), GetCipherList(), GetConnectTimeout(), GetDebugInfo()); - m_RconLocked.store(m_Rcon); + m_RconLocked.store(m_Rcon, m_FieldsMutex); for (const Type::Ptr& type : GetTypes()) { auto ctype (dynamic_cast(type.get())); diff --git a/lib/icingadb/icingadb.hpp b/lib/icingadb/icingadb.hpp index 6652d9c1f4d..3812a02ea5a 100644 --- a/lib/icingadb/icingadb.hpp +++ b/lib/icingadb/icingadb.hpp @@ -48,7 +48,7 @@ class IcingaDB : public ObjectImpl inline RedisConnection::Ptr GetConnection() { - return m_RconLocked.load(); + return m_RconLocked.load(m_FieldsMutex); } template diff --git a/tools/mkclass/classcompiler.cpp b/tools/mkclass/classcompiler.cpp index 6082cbed6ed..2a2d3087a07 100644 --- a/tools/mkclass/classcompiler.cpp +++ b/tools/mkclass/classcompiler.cpp @@ -454,8 +454,14 @@ void ClassCompiler::HandleClass(const Klass& klass, const ClassDebugInfo&) m_Header << "template<>" << std::endl << "class ObjectImpl<" << klass.Name << ">" << " : public " << (klass.Parent.empty() ? "Object" : klass.Parent) << std::endl - << "{" << std::endl - << "public:" << std::endl + << "{" << std::endl; + + if (klass.Parent.empty()) { + m_Header << "protected:" << std::endl + << "\tmutable LockedMutex m_FieldsMutex;" << std::endl << std::endl; + } + + m_Header << "public:" << std::endl << "\t" << "DECLARE_PTR_TYPEDEFS(ObjectImpl<" << klass.Name << ">);" << std::endl << std::endl; /* Validate */ @@ -815,7 +821,7 @@ void ClassCompiler::HandleClass(const Klass& klass, const ClassDebugInfo&) << "{" << std::endl; if (field.GetAccessor.empty() && !(field.Attributes & FANoStorage)) - m_Impl << "\t" << "return m_" << field.GetFriendlyName() << ".load();" << std::endl; + m_Impl << "\t" << "return m_" << field.GetFriendlyName() << ".load(m_FieldsMutex);" << std::endl; else m_Impl << field.GetAccessor << std::endl; @@ -853,7 +859,7 @@ void ClassCompiler::HandleClass(const Klass& klass, const ClassDebugInfo&) << "\t" << "auto *dobj = dynamic_cast(this);" << std::endl; if (field.SetAccessor.empty() && !(field.Attributes & FANoStorage)) - m_Impl << "\t" << "m_" << field.GetFriendlyName() << ".store(value);" << std::endl; + m_Impl << "\t" << "m_" << field.GetFriendlyName() << ".store(value, m_FieldsMutex);" << std::endl; else m_Impl << field.SetAccessor << std::endl << std::endl;