Enhancement: Improving memory efficiency by eliminating the vtable in the managed class. #876
Conversation
✅ Deploy Preview for dpp-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
why are you so obsessed with getting rid of the vtable here? |
Because I tend to fixate on things periodically - and I disagree that it's over engineered and hard to read - I've barely changed anything. Is it slightly more complex? Yes. Is it worth the memory savings? I would say it is - though I suppose that's what is up for debate here. The final say is ultimately yours though. |
include/dpp/cache.h
Outdated
| deletion_queue = {}; | ||
| } | ||
| } | ||
| if constexpr (std::is_same_v<emoji, value_type>) { |
There was a problem hiding this comment.
Why this btw, and making this function a template? Can't this function rehash every cache like how it was before?
There was a problem hiding this comment.
So we can static cast the managed pointers to their derived types and avoid falling into not being able to delete them, given that we've made the managed destructor protected. And we're doing the if-else here to avoid rehashing each of the caches unnecessarily, given that we now have to do this here:
Lines 508 to 512 in 03d126b
There was a problem hiding this comment.
But for example dpp::get_emoji_cache() returns a dpp::cache<emoji> *, right? So when doing dpp::get_emoji_cache()->rehash(); like how it was before, that would already call the function with the correct type, as dpp::cache<emoji>::rehash uses emoji anyways (see https://github.com/brainboxdotcc/DPP/blob/master/include/dpp/cache.h#L233)
There was a problem hiding this comment.
Right but when we call garbage_collection repeatedly - one for each of the types, we don't want to rehash it for each of the calls.
There was a problem hiding this comment.
Well you wouldn't need to call garbage_collection multiple times, just one like how it was before
There was a problem hiding this comment.
No because we need access to the derived type to static_cast the managed pointer into, as seen here:
Line 283 in 03d126b
There was a problem hiding this comment.
I see. Another thing however, how are we certain deletion_queue only contains objects of that type? The way i see it in
Line 137 in 03d126b
There was a problem hiding this comment.
There I've solved the "cache of any types" issue. It might be too over engineered, let me know what you think.
github-actions
bot
added
the
documentation
…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.
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.
Code change checklist