From 98cfe491b72f29ac307896e83fb4c93dc464e846 Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Wed, 13 Nov 2024 15:59:48 +0100 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 | 58 ++++++++++++++++++--------------- 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 | 16 ++++++--- 12 files changed, 54 insertions(+), 42 deletions(-) diff --git a/lib/base/atomic.hpp b/lib/base/atomic.hpp index 855850336f7..24c809dd46b 100644 --- a/lib/base/atomic.hpp +++ b/lib/base/atomic.hpp @@ -5,8 +5,6 @@ #include #include -#include -#include namespace icinga { @@ -34,8 +32,10 @@ class Atomic : public std::atomic { } }; +class LockedMutex; + /** - * Wraps any T into a std::atomic-like interface that locks using a mutex. + * Wraps any T into an interface similar to std::atomic, that locks using a mutex. * * In contrast to std::atomic, Locked is also valid for types that are not trivially copyable. * In case T is trivially copyable, std::atomic is almost certainly the better choice. @@ -46,38 +46,44 @@ template class Locked { public: - inline T load() const - { - std::unique_lock lock(m_Mutex); - - return m_Value; - } - - inline void store(T desired) - { - std::unique_lock lock(m_Mutex); - - m_Value = std::move(desired); - } + T load(LockedMutex& mtx) const; + void store(T desired, LockedMutex& mtx); private: - mutable std::mutex m_Mutex; T m_Value; }; /** - * Type alias for std::atomic if possible, otherwise Locked is used as a fallback. + * 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 */ -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; -#else /* defined(__GNUC__) && __GNUC__ < 5 */ - typename std::conditional::value, std::atomic, Locked>::type; -#endif /* defined(__GNUC__) && __GNUC__ < 5 */ +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); + + return m_Value; +} + +template +void Locked::store(T desired, LockedMutex& mtx) +{ + std::unique_lock lock (mtx.m_Mutex); + + m_Value = std::move(desired); +} } 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 8d3b9099bd7..50f95b6a17a 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"); @@ -84,7 +84,7 @@ void IcingaDB::Start(bool runtimeCreated) m_Rcon = new RedisConnection(GetHost(), GetPort(), GetPath(), GetUsername(), 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..2cf5e27f3de 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 << "\treturn 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 << "\tm_" << field.GetFriendlyName() << ".store(value, m_FieldsMutex);" << std::endl; else m_Impl << field.SetAccessor << std::endl << std::endl; @@ -1068,7 +1074,7 @@ void ClassCompiler::HandleClass(const Klass& klass, const ClassDebugInfo&) if (field.Attributes & FANoStorage) continue; - m_Header << "\tAtomicOrLocked<" << field.Type.GetRealType() << "> m_" << field.GetFriendlyName() << ";" << std::endl; + m_Header << "\tLocked<" << field.Type.GetRealType() << "> m_" << field.GetFriendlyName() << ";" << std::endl; } /* signal */