Freeze registries at startup, when everything has been registered #10388
Conversation
Al2Klimov
added
enhancement
| std::shared_lock<std::shared_mutex> ReadLockUnlessFrozen() const | ||
| { | ||
| bool old_item = false; | ||
|
|
||
| if (m_Items.erase(name) > 0) | ||
| old_item = true; | ||
|
|
||
| m_Items[name] = item; | ||
|
|
||
| lock.unlock(); | ||
|
|
||
| if (old_item) | ||
| OnUnregistered(name); | ||
| if (m_Frozen.load(std::memory_order_relaxed)) { | ||
| return {}; | ||
| } | ||
|
|
||
| OnRegistered(name, item); | ||
| return std::shared_lock(m_Mutex); | ||
| } |
There was a problem hiding this comment.
lib/base/registry.hpp
Outdated
| */ | ||
| void Freeze() | ||
| { | ||
| std::shared_lock lock (m_Mutex); |
There was a problem hiding this comment.
Why do you need lock this here? Isn't m_Frozen atomic?
There was a problem hiding this comment.
- Namespace: use rwlock and disable read locking after freeze #9627 also kept the lock in Freeze() 🤷♂️
| if (m_Frozen) { | ||
| BOOST_THROW_EXCEPTION(std::logic_error("Registry is read-only and must not be modified.")); | ||
| } |
There was a problem hiding this comment.
You can also perform this check before even locking the mutex. I don't understand this, if you need the mutex for m_Frozen as well, why make it atomic?
There was a problem hiding this comment.
- I did it exactly like Namespace: use rwlock and disable read locking after freeze #9627 (24b57f0) 🤷♂️
There was a problem hiding this comment.
This doesn't answer the questions! And no, you didn’t do exactly the same.
icinga2/lib/base/namespace.cpp
Lines 52 to 56 in 0613381
icinga2/lib/base/namespace.cpp
Lines 94 to 98 in 0613381
icinga2/lib/base/namespace.cpp
Lines 120 to 122 in 0613381
There was a problem hiding this comment.
Actually, it IS the same: Register/Freeze
- locks a mutex
- operates on m_Frozen
It's critical to synchronize them! Imagine:
- Register sees not frozen instance
- Freeze freezes it
- ReadLockUnlessFrozen doesn't lock it
- Register modifies it
- 🤯
There was a problem hiding this comment.
Actually, it IS the same
No, it's not!
It's critical to synchronize them! Imagine:
I don't get it! It's for a reason atomic and if you can't synchronise those without a mutex why make it atomic then?
There was a problem hiding this comment.
Actually, it IS the same
No, it's not!
We could argue for centuries...
I don't get it! It's for a reason atomic and if you can't synchronise those without a mutex why make it atomic then?
I can't synchronise write ops without a mutex. m_Frozen is atomic to speed up read ops.
|
@Al2Klimov I'd suggest you to answer the questions in the previous review first before requesting another review. |
Al2Klimov
force-pushed
the
Registry-Freeze
branch
from
5407de3 to
e90a62a
Compare
in derived classes and inline them, as side effect, to speed up calls.
Al2Klimov
force-pushed
the
Registry-Freeze
branch
from
e90a62a to
e4bfbf2
Compare
to query them at runtime lock-freely.