Skip to content

Commit ea6e3cc

Browse files
committed
Simplify dependency (un)registration path
Co-Authored-By: Julian Brost <[email protected]> Since @jbrost suggested simplifying the code for (un)registering the dependencies and even created a prototype of how he envisioned it (see 20faf1c), I have now redone everthing on that basis. Even if the end result doesn't quite match Julian's commit, I think it makes sense not to implement it 1-1 as Julian suggested for a variaty reasons. Most of the code where I personally think a clarifying comment is needed, I have provided detailed ones justifying why something was done that way.
1 parent dbe0eaa commit ea6e3cc

File tree

6 files changed

+213
-140
lines changed

6 files changed

+213
-140
lines changed

lib/icinga/checkable-dependency.cpp

Lines changed: 149 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,24 @@
77

88
using namespace icinga;
99

10-
void Checkable::AddDependencyGroup(const DependencyGroup::Ptr& dependencyGroup)
10+
/**
11+
* Register all the dependency groups of the current Checkable to the global dependency group registry.
12+
*
13+
* Initially, each Checkable object tracks locally its own dependency groups on Icinga 2 startup, and once the start
14+
* signal of that Checkable is emitted, it pushes all the local tracked dependency groups to the global registry.
15+
* Once the global registry is populated with all the local dependency groups, this Checkable may not necessarily
16+
* contain the exact same dependency groups as it did before, as identical groups are merged together in the registry,
17+
* but it's guaranteed that it's going to have the same *number* of dependency groups as before.
18+
*/
19+
void Checkable::PushDependencyGroupsToRegistry()
1120
{
12-
std::unique_lock lock(m_DependencyMutex);
13-
m_DependencyGroups.insert(dependencyGroup);
14-
}
21+
std::lock_guard lock(m_DependencyMutex);
22+
decltype(m_DependencyGroups) dependencyGroups{0, HashDependencyGroup, CompareDependencyGroup};
23+
m_DependencyGroups.swap(dependencyGroups);
1524

16-
void Checkable::RemoveDependencyGroup(const DependencyGroup::Ptr& dependencyGroup)
17-
{
18-
std::unique_lock lock(m_DependencyMutex);
19-
m_DependencyGroups.erase(dependencyGroup);
25+
for (auto& dependencyGroup : dependencyGroups) {
26+
m_DependencyGroups.emplace(DependencyGroup::Register(dependencyGroup));
27+
}
2028
}
2129

2230
std::vector<DependencyGroup::Ptr> Checkable::GetDependencyGroups() const
@@ -25,6 +33,97 @@ std::vector<DependencyGroup::Ptr> Checkable::GetDependencyGroups() const
2533
return {m_DependencyGroups.begin(), m_DependencyGroups.end()};
2634
}
2735

36+
void Checkable::AddDependency(const Dependency::Ptr& dependency, bool refreshGlobalRegistry)
37+
{
38+
std::lock_guard lock(m_DependencyMutex);
39+
DependencyGroup::Ptr newGroup(new DependencyGroup(dependency->GetRedundancyGroup(), dependency));
40+
if (auto it(m_DependencyGroups.find(newGroup)); it == m_DependencyGroups.end()) {
41+
m_DependencyGroups.emplace(refreshGlobalRegistry ? DependencyGroup::Register(newGroup) : newGroup);
42+
} else if (!refreshGlobalRegistry) {
43+
// If we're not going to refresh the global registry, we just need to add the dependency to the existing group.
44+
// Meaning, the dependency group itself isn't registered globally yet, so we don't need to re-register it.
45+
(*it)->AddMember(dependency);
46+
} else {
47+
if (auto existingGroup(*it); existingGroup->HasIdenticalMember(dependency)) {
48+
// There's already an identical member in the group and this is likely an exact duplicate of it,
49+
// so it won't change the identity of the group after registration, i.e. regardless whether we're
50+
// supposed to refresh the global registry or not it's identity will remain the same.
51+
existingGroup->AddMember(dependency);
52+
} else {
53+
// We're going to either replace the existing group with "newGroup" or merge the two groups together.
54+
// Either way, it's identity will change, so we need to decouple it from the current Checkable.
55+
m_DependencyGroups.erase(it);
56+
57+
if (auto members(existingGroup->GetMembers(this)); members.size() == existingGroup->GetMemberCount()) {
58+
// We need to unregister the existing group from the global registry, as it's hash might change
59+
// after adding the new dependency to it down below, and we want to re-register it afterwards.
60+
// This way, we'll also be able to eliminate the possibility of having two identical groups in
61+
// the registry that might occur due to the registration of the new dependency object below.
62+
DependencyGroup::Unregister(existingGroup);
63+
// The current Checkable is the only member of the group, so nothing to move around, just
64+
// add the _duplicate_ dependency to the existing group. Duplicate in the sense that it's
65+
// not identical to any of the existing members but similar enough to be part of the same
66+
// group, i.e. same parent, maybe different period, state filter, etc.
67+
existingGroup->AddMember(dependency);
68+
m_DependencyGroups.emplace(DependencyGroup::Register(existingGroup));
69+
} else {
70+
// Obviously, the current Checkable is not the only member of the existing group, and it's going to
71+
// have more members than the other child Checkables in that group after adding the new dependency
72+
// to it. So, we need to move all the members this Checkable depends on to newGroup and leave the
73+
// existing group as-is, i.e. it's identity won't change afterwards.
74+
for (auto& member : members) {
75+
existingGroup->RemoveMember(member);
76+
newGroup->AddMember(member);
77+
}
78+
m_DependencyGroups.emplace(DependencyGroup::Register(newGroup));
79+
}
80+
}
81+
}
82+
}
83+
84+
void Checkable::RemoveDependency(const Dependency::Ptr& dependency)
85+
{
86+
std::lock_guard lock(m_DependencyMutex);
87+
DependencyGroup::Ptr newGroup(new DependencyGroup(dependency->GetRedundancyGroup(), dependency));
88+
if (auto it(m_DependencyGroups.find(newGroup)); it != m_DependencyGroups.end()) {
89+
// Obviously, this method is concerned with removing a dependency from the current Checkable, but we've
90+
// initiated a new group with the _to be removed_ dependency in it mainly to perform lookups in the set.
91+
// Now, that we've found the existing group it's member of, we need to remove the dependency from it first.
92+
newGroup->RemoveMember(dependency);
93+
94+
auto existingGroup(*it);
95+
// We're going to either replace or re-register the existing group down below, so we need to decouple it.
96+
// However, only after we've created another reference to it (see existingGroup above), so that we don't
97+
// get a dangling pointer to it in case this was the only reference to it.
98+
m_DependencyGroups.erase(it);
99+
100+
if (auto members(existingGroup->GetMembers(this)); members.size() == existingGroup->GetMemberCount()) {
101+
// This is a very similar case to the one in AddDependency, but we're going to remove the dependency
102+
// from the group instead of adding it. Therefore, most of the reasoning given there apply here as well.
103+
DependencyGroup::Unregister(existingGroup);
104+
existingGroup->RemoveMember(dependency);
105+
newGroup = existingGroup;
106+
} else {
107+
// The current Checkable is not the only member of the existing group, and it's going to have
108+
// (more or less) fewer members than the other child Checkables within that group after unregistering the
109+
// dependency from it. Meaning, in case the current Checkable have more than one identical dependency in
110+
// the group, it'll end up having the very same dependency group as before, but with one less member.
111+
// However, instead of determining such edge cases manually, we're taking the easy way out, i.e. just move
112+
// all the members (except the one to be removed) to newGroup and re-register it in the global registry.
113+
for (auto& member : members) {
114+
existingGroup->RemoveMember(member);
115+
if (member != dependency) {
116+
newGroup->AddMember(member);
117+
}
118+
}
119+
}
120+
121+
if (newGroup->HasMembers()) {
122+
m_DependencyGroups.emplace(DependencyGroup::Register(newGroup));
123+
}
124+
}
125+
}
126+
28127
std::vector<Dependency::Ptr> Checkable::GetDependencies() const
29128
{
30129
std::unique_lock<std::mutex> lock(m_DependencyMutex);
@@ -208,3 +307,45 @@ void Checkable::GetAllChildrenInternal(std::set<Checkable::Ptr>& children, int l
208307

209308
children.insert(localChildren.begin(), localChildren.end());
210309
}
310+
311+
/**
312+
* Generate the hash of the provided dependency group.
313+
*
314+
* Note, the resulted hash is used to uniquely identify the dependency group on a per Checkable basis and not globally.
315+
* Therefore, for redundancy groups, the hash is generated based on the group name, for non-redundant groups, on the
316+
* parent Checkable name of its members.
317+
*
318+
* @param dependencyGroup The dependency group to generate the hash for.
319+
*
320+
* @return size_t - Returns the hash of the provided dependency group.
321+
*/
322+
size_t Checkable::HashDependencyGroup(const DependencyGroup::Ptr& dependencyGroup)
323+
{
324+
return std::hash<String>()(
325+
dependencyGroup->IsRedundancyGroup() ? dependencyGroup->GetName() : dependencyGroup->PeekParentCheckableName()
326+
);
327+
}
328+
329+
/**
330+
* Check the equality of the provided dependency groups.
331+
*
332+
* Please note that the equality check is based on criteria that uniquely identify the dependency group
333+
* within a given Checkable only and aren't meant to be used globally.
334+
*
335+
* @param lhs The left-hand side dependency group to compare.
336+
* @param rhs The right-hand side dependency group to compare.
337+
*
338+
* @return bool - Returns true if the provided dependency groups turned to be equal, otherwise false.
339+
*/
340+
bool Checkable::CompareDependencyGroup(const DependencyGroup::Ptr& lhs, const DependencyGroup::Ptr& rhs)
341+
{
342+
if (lhs->IsRedundancyGroup() != rhs->IsRedundancyGroup()) {
343+
return false;
344+
}
345+
346+
if (lhs->IsRedundancyGroup()) {
347+
return lhs->GetName() == rhs->GetName();
348+
}
349+
350+
return lhs->PeekParentCheckableName() == rhs->PeekParentCheckableName();
351+
}

lib/icinga/checkable.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,8 @@ void Checkable::OnAllConfigLoaded()
8080

8181
void Checkable::Start(bool runtimeCreated)
8282
{
83+
PushDependencyGroupsToRegistry();
84+
8385
double now = Utility::GetTime();
8486

8587
{

lib/icinga/checkable.hpp

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -185,9 +185,10 @@ class Checkable : public ObjectImpl<Checkable>
185185
bool IsFlapping() const;
186186

187187
/* Dependencies */
188-
void AddDependencyGroup(const intrusive_ptr<DependencyGroup>& dependencyGroup);
189-
void RemoveDependencyGroup(const intrusive_ptr<DependencyGroup>& dependencyGroup);
188+
void PushDependencyGroupsToRegistry();
190189
std::vector<intrusive_ptr<DependencyGroup>> GetDependencyGroups() const;
190+
void AddDependency(const intrusive_ptr<Dependency>& dependency, bool refreshGlobalRegistry = false);
191+
void RemoveDependency(const intrusive_ptr<Dependency>& dependency);
191192
std::vector<intrusive_ptr<Dependency> > GetDependencies() const;
192193
bool HasAnyDependencies() const;
193194

@@ -247,10 +248,17 @@ class Checkable : public ObjectImpl<Checkable>
247248
std::set<Notification::Ptr> m_Notifications;
248249
mutable std::mutex m_NotificationMutex;
249250

251+
static size_t HashDependencyGroup(const intrusive_ptr<DependencyGroup>& dependencyGroup);
252+
static bool CompareDependencyGroup(const intrusive_ptr<DependencyGroup>& lhs, const intrusive_ptr<DependencyGroup>& rhs);
253+
250254
/* Dependencies */
251255
mutable std::mutex m_DependencyMutex;
252-
std::set<intrusive_ptr<DependencyGroup>> m_DependencyGroups;
253256
std::set<intrusive_ptr<Dependency> > m_ReverseDependencies;
257+
std::unordered_set<
258+
intrusive_ptr<DependencyGroup>,
259+
decltype(&HashDependencyGroup),
260+
decltype(&CompareDependencyGroup)
261+
> m_DependencyGroups{0, HashDependencyGroup, CompareDependencyGroup};
254262

255263
void GetAllChildrenInternal(std::set<Checkable::Ptr>& children, int level = 0) const;
256264

lib/icinga/dependency-group.cpp

Lines changed: 29 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -9,108 +9,32 @@ using namespace icinga;
99
boost::signals2::signal<void (const Dependency::Ptr&, const std::vector<DependencyGroup::Ptr>&)> DependencyGroup::OnMembersChanged;
1010

1111
std::mutex DependencyGroup::m_RegistryMutex;
12-
DependencyGroup::RegistryType DependencyGroup::m_Registry;
12+
DependencyGroup::RegistryType DependencyGroup::m_Registry{0, DependencyGroup::Hash, DependencyGroup::Equal};
1313

1414
/**
15-
* Refresh the global registry of dependency groups.
15+
* Register the provided dependency group to the global dependency group registry.
1616
*
17-
* Registers the provided dependency object to an existing dependency group with the same redundancy
18-
* group name (if any), or creates a new one and registers it to the child Checkable and the registry.
19-
*
20-
* Note: This is a helper function intended for internal use only, and you should acquire the global registry mutex
21-
* before calling this function.
22-
*
23-
* @param dependency The dependency object to refresh the registry for.
24-
* @param unregister A flag indicating whether the provided dependency object should be unregistered from the registry.
17+
* @param dependencyGroup The dependency group to register.
2518
*/
26-
void DependencyGroup::RefreshRegistry(const Dependency::Ptr& dependency, bool unregister)
27-
{
28-
auto registerRedundancyGroup = [](const DependencyGroup::Ptr& dependencyGroup) {
29-
if (auto [it, inserted](m_Registry.insert(dependencyGroup.get())); !inserted) {
30-
DependencyGroup::Ptr existingGroup(*it);
31-
dependencyGroup->MoveMembersTo(existingGroup);
32-
}
33-
};
34-
35-
// Retrieve all the dependency groups with the same redundancy group name of the provided dependency object.
36-
// This allows us to shorten the lookup for the _one_ optimal group to (un)register the dependency from/to.
37-
auto [begin, end] = m_Registry.get<1>().equal_range(dependency->GetRedundancyGroup());
38-
for (auto it(begin); it != end; ++it) {
39-
DependencyGroup::Ptr existingGroup(*it);
40-
auto child(dependency->GetChild());
41-
if (auto members(existingGroup->GetMembers(child.get())); !members.empty()) {
42-
m_Registry.erase(existingGroup->GetCompositeKey()); // Will be re-registered when needed down below.
43-
if (unregister) {
44-
existingGroup->RemoveMember(dependency);
45-
// Remove the connection between the child Checkable and the dependency group if it has no members
46-
// left or the above removed member was the only member of the group that the child depended on.
47-
if (!existingGroup->HasMembers() || members.size() == 1) {
48-
child->RemoveDependencyGroup(existingGroup);
49-
}
50-
}
51-
52-
size_t totalMembers(existingGroup->GetMemberCount());
53-
// If the existing dependency group has an identical member already, or the child Checkable of the
54-
// dependency object is the only member of it (totalMembers == members.size()), we can simply add the
55-
// dependency object to the existing group.
56-
if (!unregister && (existingGroup->HasIdenticalMember(dependency) || totalMembers == members.size())) {
57-
existingGroup->AddMember(dependency);
58-
} else if (!unregister || (members.size() > 1 && totalMembers >= members.size())) {
59-
// The child Checkable is going to have a new dependency group, so we must detach the existing one.
60-
child->RemoveDependencyGroup(existingGroup);
61-
62-
Ptr replacementGroup(unregister ? nullptr : new DependencyGroup(existingGroup->GetName(), dependency));
63-
for (auto& member : members) {
64-
if (member != dependency) {
65-
existingGroup->RemoveMember(member);
66-
if (replacementGroup) {
67-
replacementGroup->AddMember(member);
68-
} else {
69-
replacementGroup = new DependencyGroup(existingGroup->GetName(), member);
70-
}
71-
}
72-
}
73-
74-
child->AddDependencyGroup(replacementGroup);
75-
registerRedundancyGroup(replacementGroup);
76-
}
77-
78-
if (existingGroup->HasMembers()) {
79-
registerRedundancyGroup(existingGroup);
80-
}
81-
return;
82-
}
83-
}
84-
85-
if (!unregister) {
86-
// We couldn't find any existing dependency group to register the dependency to, so we must
87-
// initiate a new one and attach it to the child Checkable and register to the global registry.
88-
DependencyGroup::Ptr newGroup(new DependencyGroup(dependency->GetRedundancyGroup(), dependency));
89-
dependency->GetChild()->AddDependencyGroup(newGroup);
90-
registerRedundancyGroup(newGroup);
91-
}
92-
}
93-
94-
/**
95-
* Register the provided dependency to the global dependency group registry.
96-
*
97-
* @param dependency The dependency to register.
98-
*/
99-
void DependencyGroup::Register(const Dependency::Ptr& dependency)
19+
DependencyGroup::Ptr DependencyGroup::Register(const DependencyGroup::Ptr& dependencyGroup)
10020
{
10121
std::lock_guard lock(m_RegistryMutex);
102-
RefreshRegistry(dependency, false);
22+
if (auto [it, inserted] = m_Registry.insert(dependencyGroup); !inserted) {
23+
dependencyGroup->MoveMembersTo(*it);
24+
return *it;
25+
}
26+
return dependencyGroup;
10327
}
10428

10529
/**
106-
* Unregister the provided dependency from the dependency group it was member of.
30+
* Unregister the provided dependency group from the global dependency group registry.
10731
*
108-
* @param dependency The dependency to unregister.
32+
* @param dependencyGroup The dependency group to unregister.
10933
*/
110-
void DependencyGroup::Unregister(const Dependency::Ptr& dependency)
34+
void DependencyGroup::Unregister(const DependencyGroup::Ptr& dependencyGroup)
11135
{
11236
std::lock_guard lock(m_RegistryMutex);
113-
RefreshRegistry(dependency, true);
37+
m_Registry.erase(dependencyGroup);
11438
}
11539

11640
/**
@@ -273,17 +197,10 @@ void DependencyGroup::MoveMembersTo(const DependencyGroup::Ptr& dest)
273197
VERIFY(this != dest); // Prevent from doing something stupid, i.e. deadlocking ourselves.
274198

275199
std::lock_guard lock(m_Mutex);
276-
DependencyGroup::Ptr thisPtr(this); // Just in case the Checkable below was our last reference.
277200
for (auto& [_, members] : m_Members) {
278-
Checkable::Ptr previousChild;
279-
for (auto& [checkable, dependency] : members) {
280-
dest->AddMember(dependency);
281-
if (!previousChild || previousChild != checkable) {
282-
previousChild = dependency->GetChild();
283-
previousChild->RemoveDependencyGroup(thisPtr);
284-
previousChild->AddDependencyGroup(dest);
285-
}
286-
}
201+
std::for_each(members.begin(), members.end(), [&dest](const auto& member) {
202+
dest->AddMember(member.second);
203+
});
287204
}
288205
}
289206

@@ -357,6 +274,19 @@ String DependencyGroup::GetCompositeKey()
357274
return m_CompositeKey;
358275
}
359276

277+
/**
278+
* Peek the name of the first parent Checkable in the member list.
279+
*
280+
* Returns an empty string if it doesn't have any members.
281+
*
282+
* @return String - The name of the first parent Checkable in the member list.
283+
*/
284+
String DependencyGroup::PeekParentCheckableName() const
285+
{
286+
std::lock_guard lock(m_Mutex);
287+
return m_Members.empty() ? "" : std::get<0>(m_Members.begin()->first);
288+
}
289+
360290
/**
361291
* Retrieve the state of the current dependency group.
362292
*

0 commit comments

Comments
 (0)