Icinga / icinga2

The core of our monitoring platform with a powerful configuration language and REST API.
https://icinga.com/docs/icinga2/latest
GNU General Public License v2.0
2.03k stars 578 forks source link

Don't generate a mutex for each Locked<T>, share one per object #10117

Open Al2Klimov opened 3 months ago

Al2Klimov commented 3 months ago

This reduces RAM usage per object by sizeof(mutex)*(FIELDS-1).

The atomic_load(const std::shared_ptr<T>*) idea would, without doubt, also consume less RAM. However, it would scale worse, the more objects you have, with a fixed number of mutexes. Neither one SpinMutex per property, nor one std::mutex per object suffer from this limitation.

julianbrost commented 2 months ago

This misses part of what #10113 asked for:

  1. Figure out how much of an effect this has on the total memory use of Icinga 2.

We have a vague "Icinga 2 uses more memory in newer versions", so how much does this affect the total memory use of the whole process? Might this be the answer or is it just a small puzzle piece?

Also, what made you choose adding that m_FieldsMutex and passing it around over what I suggested in #10113? I'm not a big fan of how random code (see the changes to lib/icingadb/ now has to access some internal attribute generated by mkclass. A nice aspect of the current implementation is that it's very easy to reason that it won't deadlock, all the magic happens here: https://github.com/Icinga/icinga2/blob/585b357c3f912957bfdfaa1efad0b850f05f8741/lib/base/atomic.hpp#L52-L73

Whereas with this PR, exposing the underlying mutex, this gets more spread out in the code base.

Al2Klimov commented 2 months ago

Also, what made you choose adding that m_FieldsMutex and passing it around over what I suggested in #10113?

sharing the mutex between objects, this would reduce the memory requirements

The more fields, especially objects, share one mutex, the slower they can proceed. I prefer 1+ mutex per separate object.

I'm not a big fan of how random code (see the changes to lib/icingadb/ now has to access some internal attribute generated by mkclass. A nice aspect of the current implementation is that it's very easy to reason that it won't deadlock, all the magic happens here:

https://github.com/Icinga/icinga2/blob/585b357c3f912957bfdfaa1efad0b850f05f8741/lib/base/atomic.hpp#L52-L73

Whereas with this PR, exposing the underlying mutex, this gets more spread out in the code base.

You're right. One wrong move on the mutex and... 🙈

julianbrost commented 1 month ago

Please use a search engine of your choice to search for the keywords "userspace" and "spinlock". Seems like the majority isn't too positive about that idea. And as already said, Icinga 2 already had a negative experience with spinlocks itself. What makes you believe that now is a good time to use a spinlock in userspace?

By the way, Linus Torvalds' rant post (the one linked from cppreference.com) even has a follow-up from him on yield.

Apart from that, the current version of the PR seems to restrict the use of that suggested spinlock to just intrusive_ptr, so each string attribute would still use its own std::mutex. So the memory use will be somewhere in the middle (I haven't looked into the distribution of attribute types), but why do it that way if it can be done better?

The atomic_load(const std::shared_ptr<T>*) idea would, without doubt, also consume less RAM. However, it would scale worse, the more objects you have, with a fixed number of mutexes.

However, while there may be a large number of objects, the number of CPU cores and thereby threads we start is typically much smaller. And that actually limits the total contention. There won't be a thread for each single object trying to lock it.