From 666afbbf59c53eed07e0b6d10b99eb19a432873c Mon Sep 17 00:00:00 2001 From: RealTimeChris <40668522+RealTimeChris@users.noreply.github.com> Date: Fri, 22 Sep 2023 02:21:46 -0400 Subject: [PATCH] Improving memory efficiency by eliminating the vtable in the managed class. By making the destructor protected - we can remove the possibility that someone tries to delete one of the derived classes by deleting an instance of the managed class, which should allow for not having a virtual destructor. --- include/dpp/cache.h | 148 +++++++++++++++++++++++++++++++++++--- include/dpp/emoji.h | 2 +- include/dpp/managed.h | 10 +-- src/dpp/cache.cpp | 44 ++---------- src/dpp/discordclient.cpp | 2 +- src/unittest/test.cpp | 7 +- 6 files changed, 156 insertions(+), 57 deletions(-) diff --git a/include/dpp/cache.h b/include/dpp/cache.h index 5895af576d..f590a96ebc 100644 --- a/include/dpp/cache.h +++ b/include/dpp/cache.h @@ -21,20 +21,107 @@ ************************************************************************************/ #pragma once -#include #include +#include +#include #include #include +#include +#include +#include +#include +#include +#include #include -#include namespace dpp { +class cache_registry; + +extern DPP_EXPORT std::unordered_map regs; extern DPP_EXPORT std::unordered_map deletion_queue; extern DPP_EXPORT std::mutex deletion_mutex; +template class cache; + +/** + * @brief The `cache_registry` class defines an abstract base class for cache objects. + * + * It provides a mechanism for registering, managing, and performing garbage collection + * on cache objects. Cache objects must inherit from this class and implement the + * required functions. + */ +class cache_registry { +public: + /** + * @brief Pure virtual function for performing garbage collection. + */ + virtual void garbage_collection() = 0; + + /** + * @brief Pure virtual function to get the type index for a cache. + * @return Type index representing the cache's type. + */ + virtual std::type_index get_type_index() const = 0; + + /** + * @brief Static method to register a cache object. + * @param registry A pointer to the cache_registry object to register. + */ + static void register_cache(cache_registry* registry) { + auto type_index = registry->get_type_index(); + if (registries().find(type_index) != registries().end()) { + throw cache_exception{ "Sorry, but there was already a cache by that type that has been registered." }; + } + registries()[registry->get_type_index()] = registry; + } + + /** + * @brief Static method to call garbageCollection for all registered caches. + * This method invokes garbage collection on all registered cache objects. + */ + static void call_garbage_collection() { + for (auto& [key, value] : registries()) { + value->garbage_collection(); + } + } + + /** + * @brief Static method to retrieve a cache object by type. + * + * @tparam T The type of the cache to retrieve. + * @return A pointer to the cache object of type T if found, or nullptr if not found. + */ + template + static cache* get_cache() { + std::type_index typeIndex(typeid(T)); + for (auto& [key,value]: registries()) { + if (value->get_type_index() == typeIndex) { + return static_cast*>(value); + } + } + // If cache of type T not found, create and register a new one, then return it. + register_cache(new cache{}); + return get_cache(); + } + +private: + /** + * @brief Static vector to store registered cache objects. + * This vector holds pointers to all registered cache objects. + */ + static std::unordered_map& registries() { + return regs; + } +}; + /** forward declaration */ class guild_member; +class channel; +class guild; +class emoji; +class user; +class role; /** * @brief A cache object maintains a cache of dpp::managed objects. @@ -47,7 +134,7 @@ class guild_member; * designed with thread safety in mind. * @tparam T class type to store, which should be derived from dpp::managed. */ -template class cache { +template class cache : public cache_registry { private: /** * @brief Mutex to protect the cache @@ -62,12 +149,18 @@ template class cache { std::unordered_map* cache_map; public: + // Implement the get_type_index function + std::type_index get_type_index() const override { + return std::type_index(typeid(T)); + } + /** * @brief Construct a new cache object. * * Caches must contain classes derived from dpp::managed. */ cache() { + cache_registry::register_cache(this); cache_map = new std::unordered_map; } @@ -241,6 +334,50 @@ template class cache { cache_map = n; } + /** Run garbage collection across all caches removing deleted items + * that have been deleted over 60 seconds ago. + */ + /* Because other threads and systems may run for a short while after an event is received, we don't immediately + * delete pointers when objects are replaced. We put them into a queue, and periodically delete pointers in the + * queue. This also rehashes unordered_maps to ensure they free their memory. + */ + inline void garbage_collection() { + time_t now = time(nullptr); + bool repeat = false; + { + std::lock_guard delete_lock(deletion_mutex); + do { + repeat = false; + for (auto g = deletion_queue.begin(); g != deletion_queue.end(); ++g) { + if (now > g->second + 60) { + delete static_cast(g->first); + deletion_queue.erase(g); + repeat = true; + break; + } + } + } while (repeat); + if (deletion_queue.size() == 0) { + deletion_queue = {}; + } + } + if constexpr (std::is_same_v) { + dpp::get_emoji_cache()->rehash(); + } + else if constexpr (std::is_same_v) { + dpp::get_role_cache()->rehash(); + } + else if constexpr (std::is_same_v) { + dpp::get_channel_cache()->rehash(); + } + else if constexpr (std::is_same_v) { + dpp::get_guild_cache()->rehash(); + } + else if constexpr (std::is_same_v) { + dpp::get_user_cache()->rehash(); + } + } + /** * @brief Get "real" size in RAM of the cached objects * @@ -255,11 +392,6 @@ template class cache { }; -/** Run garbage collection across all caches removing deleted items - * that have been deleted over 60 seconds ago. - */ -void DPP_EXPORT garbage_collection(); - #define cache_decl(type, setter, getter, counter) /** Find an object in the cache by id. @return type* Pointer to the object or nullptr when it's not found */ DPP_EXPORT class type * setter (snowflake id); DPP_EXPORT cache * getter (); /** Get the amount of cached type objects. */ DPP_EXPORT uint64_t counter (); /* Declare major caches */ diff --git a/include/dpp/emoji.h b/include/dpp/emoji.h index ab95736595..e427f41d7d 100644 --- a/include/dpp/emoji.h +++ b/include/dpp/emoji.h @@ -100,7 +100,7 @@ class DPP_EXPORT emoji : public managed, public json_interface { /** * @brief Destroy the emoji object */ - ~emoji() override = default; + ~emoji() = default; /** * @brief Copy assignment operator, copies another emoji's data diff --git a/include/dpp/managed.h b/include/dpp/managed.h index 5e79953517..6baf8670f5 100644 --- a/include/dpp/managed.h +++ b/include/dpp/managed.h @@ -42,10 +42,6 @@ namespace dpp { * @param nid ID to set */ managed(const snowflake nid = 0); - /** - * @brief Destroy the managed object - */ - virtual ~managed() = default; /** * @brief Get the creation time of this object according to Discord. @@ -72,6 +68,12 @@ namespace dpp { * @return false objects are the same id */ bool operator!=(const managed& other) const noexcept; + + protected: + /** + * @brief Destroy the managed object + */ + ~managed() = default; }; } // namespace dpp diff --git a/src/dpp/cache.cpp b/src/dpp/cache.cpp index 715b829dd8..c0e4f772c7 100644 --- a/src/dpp/cache.cpp +++ b/src/dpp/cache.cpp @@ -28,57 +28,21 @@ namespace dpp { +std::unordered_map regs; std::unordered_map deletion_queue; std::mutex deletion_mutex; #define cache_helper(type, cache_name, setter, getter, counter) \ -cache* cache_name = nullptr; \ type * setter (snowflake id) { \ - return cache_name ? ( type * ) cache_name ->find(id) : nullptr; \ + return ( type * ) cache_registry::get_cache()->find(id); \ } \ cache* getter () { \ - if (! cache_name ) { \ - cache_name = new cache(); \ - } \ - return cache_name ; \ + return cache_registry::get_cache(); \ } \ uint64_t counter () { \ - return ( cache_name ? cache_name ->count() : 0 ); \ + return cache_registry::get_cache()->count() ; \ } - -/* Because other threads and systems may run for a short while after an event is received, we don't immediately - * delete pointers when objects are replaced. We put them into a queue, and periodically delete pointers in the - * queue. This also rehashes unordered_maps to ensure they free their memory. - */ -void garbage_collection() { - time_t now = time(nullptr); - bool repeat = false; - { - std::lock_guard delete_lock(deletion_mutex); - do { - repeat = false; - for (auto g = deletion_queue.begin(); g != deletion_queue.end(); ++g) { - if (now > g->second + 60) { - delete g->first; - deletion_queue.erase(g); - repeat = true; - break; - } - } - } while (repeat); - if (deletion_queue.size() == 0) { - deletion_queue = {}; - } - } - dpp::get_user_cache()->rehash(); - dpp::get_channel_cache()->rehash(); - dpp::get_guild_cache()->rehash(); - dpp::get_role_cache()->rehash(); - dpp::get_emoji_cache()->rehash(); -} - - cache_helper(user, user_cache, find_user, get_user_cache, get_user_count); cache_helper(channel, channel_cache, find_channel, get_channel_cache, get_channel_count); cache_helper(role, role_cache, find_role, get_role_cache, get_role_count); diff --git a/src/dpp/discordclient.cpp b/src/dpp/discordclient.cpp index 96f9f52cd4..484b1c94cc 100644 --- a/src/dpp/discordclient.cpp +++ b/src/dpp/discordclient.cpp @@ -505,7 +505,7 @@ void discord_client::one_second_timer() creator->tick_timers(); if ((time(nullptr) % 60) == 0) { - dpp::garbage_collection(); + cache_registry::call_garbage_collection(); } } } diff --git a/src/unittest/test.cpp b/src/unittest/test.cpp index f3adb662f9..4d66e33f74 100644 --- a/src/unittest/test.cpp +++ b/src/unittest/test.cpp @@ -628,8 +628,9 @@ Markdown lol \\|\\|spoiler\\|\\| \\~\\~strikethrough\\~\\~ \\`small \\*code\\* b { set_test(TS, false); - dpp::managed m(189759562910400512); - set_test(TS, ((uint64_t) m.get_creation_time()) == 1465312605); + dpp::user m{}; + m.id = 189759562910400512; + set_test(TS, ((uint64_t)m.get_creation_time()) == 1465312605); } { @@ -1349,7 +1350,7 @@ Markdown lol \\|\\|spoiler\\|\\| \\~\\~strikethrough\\~\\~ \\`small \\*code\\* b bool message_edit_tested = false; bot.on_message_update([&](const dpp::message_update_t &event) { - if (event.msg.author == bot.me.id) { + if (event.msg.author.id == bot.me.id) { if (event.msg.content == "test edit" && !message_edit_tested) { message_edit_tested = true; set_test(EDITEVENT, true);