ConfigItem::CommitNewItems(): pre-sort types by their load dependencies once #10003

Al2Klimov · 2024-02-15T10:47:29Z

to avoid complicated nested loops, iterating over the same types and
checking dependencies over and over, skipping already completed ones.

closes #10148

lib/config/configitem.cpp

lib/base/type.cpp

julianbrost · 2024-09-06T08:00:57Z

lib/base/type.hpp

 	static std::vector<Type::Ptr> GetAllTypes();
+	static const std::vector<Ptr>& GetAllTypesSortedByLoadDependencies();


Those two names suggest that the vectors should contain the same elements, just in a different order. However, there's some additional filtering:

icinga2/lib/base/type.cpp

Lines 49 to 51 in e1f5ab8

types.erase(std::remove_if(types.begin(), types.end(), [](auto& type) {

return !ConfigObject::TypeInstance->IsAssignableFrom(type);

}), types.end());

This should be reflected in the name.

julianbrost · 2024-09-06T08:13:36Z

lib/base/type.cpp

+static std::vector<Type::Ptr> l_SortedByLoadDependencies;
+static Atomic l_SortingByLoadDependenciesDone (false);
+
+typedef std::unordered_map<Type*, bool> Visited; // https://stackoverflow.com/a/8942986


IMHO a sentence would be more helpful than a link to a "try typedef" answer, where (at least to me), it's not that obvious why this applies here.¹

Anyway, the way you use this type, I don't see a reason why this couldn't be a std::unordered_set<Type*> instead, and checking if an element is contained instead of checking the bool value.

Footnotes

The question is about SET_TYPE_NAME(std::map<int, int>, "TheMap") which is way simpler what's happening here. There, I'd find it totally plausible if the C preprocessor would not care about the C++ template parameter syntax and treat < and > as separate operators. The lambda used here as a template argument contains multiple , that aren't misinterpreted, so they are not a general problem as the C preprocessor understands some C syntax. The relevant part here is that even though {} are also part of the C syntax, the C preprocessor doesn't seem to care too much about them. I haven't found a definite answer, but it looks like it cares only for string/character literals and (). ↩

julianbrost · 2024-09-06T08:18:15Z

lib/base/type.cpp

+		for (auto& type : types) {
+			visited[type.get()] = false;
+		}


This could also be just visited.clear(), but I why is this done at all, you shouldn't have to revisit anything.

julianbrost · 2024-09-06T08:20:38Z

lib/base/type.cpp

+		}
+
+		bool& alreadyVisited (visited.at(type));
+		VERIFY(!alreadyVisited);


Is that VERIFY() cycle detection in disguise? It would be more helpful if it actually said so as it would give you more of a clue of what happened if you manage to trigger it.

julianbrost · 2024-09-06T08:25:53Z

lib/base/type.cpp

+	}
+
+	VERIFY(sorted.size() == types.size());
+	VERIFY(sorted[0]->GetLoadDependencies().empty());


Looks like a nice sanity check, but (obviously) just checks the first element. What about a test case for the whole order? Like iterate the whole vector and for each element, check that it's dependencies are placed at a lower index.

What about a test case for the whole order?

This is unnecessary IMHO, given that if the orders get messed up, you won't be able to start Icinga 2 anyway, so the startup phase actually takes care of such sanity checks and that function should just do what it is meant to do.

the startup phase actually takes care of such sanity checks

Does it check that explicitly? Or is it just that there's a good chance you'll get some errors then (but maybe it could just be broken in a subtle way)?

No, not explicitly, but the type orders do matter when starting Icinga 2, and if you insist on introducing such explicit sanity checks, well then unittest it, but this function should not be bothered in checking the final result.

What about a test case

I was thinking of a unit test when writing that.

What about a test case

I was thinking of a unit test when writing that.

I thought you'd want to loop through the final result at the end and check their orders!

yhabteab · 2024-09-06T10:47:58Z

lib/base/type.cpp

+	// Depth-first search
+	std::unordered_set<Type*> unsorted;
+	Visited visited;
+	std::vector<Type::Ptr> sorted;


Why do you need to temporarily cache the sorted ones elsewhere than l_SortedByLoadDependencies? It is guarded by VERIFY(l_SortingByLoadDependenciesDone.load()); so why can't you put the sorted ones directly in their final location.

The sorted var and the "atomic" std::swap() at the end (which I consider good practice) has no disadvantages IMAO.

yhabteab · 2024-09-06T10:52:34Z

lib/base/type.cpp

+		if (unsorted.find(type) == unsorted.end()) {
+			return;
+		}


If, for whatever reason, you define in the *.ti files that a type should be loaded after a random type that is not registered in the Types namespace, then you can be sure that you will not get a Type* if you load it with Type#GetLoadDependencies(), but a nullptr instead. So I would drop unsorted entirely and perform a ...

yhabteab · 2024-09-06T10:58:00Z

lib/base/type.cpp

+		alreadyVisited = true;
+
+		for (auto dep : type->GetLoadDependencies()) {
+			visit(dep);


... dep != nullptr instead. Because if you put this in the host.ti file, load_after awegqa24gtw34gtw34; it will try to find this bogus type via GetByName("awegqa24gtw34gtw34").get(), which obviously doesn't exist, and store a nullptr in the load dependencies set that is returned when you call host->GetLoadDependencies().

yhabteab · 2024-09-06T11:01:27Z

lib/base/type.cpp

+	types.erase(std::remove_if(types.begin(), types.end(), [](auto& type) {
+		return !ConfigObject::TypeInstance->IsAssignableFrom(type);
+	}), types.end());


I would also inline this check into the visit callback, since you don't have to remove those unwanted types beforehand.

But I need to VERIFY(sorted.size() == types.size());

…es once to avoid complicated nested loops, iterating over the same types and checking dependencies over and over, skipping already completed ones.

As just discussed offline, this is being superseded by #10148, so please review that PR instead.

Al2Klimov added the core/quality Improve code, libraries, algorithms, inline docs label Feb 15, 2024

Al2Klimov requested a review from yhabteab February 15, 2024 10:47

cla-bot bot added the cla/signed label Feb 15, 2024

yhabteab reviewed Feb 15, 2024

View reviewed changes

lib/config/configitem.cpp Outdated Show resolved Hide resolved

lib/base/type.cpp Outdated Show resolved Hide resolved

Al2Klimov requested a review from yhabteab February 20, 2024 12:38

Al2Klimov force-pushed the Type-SortByLoadDependencies branch from 9726620 to 3ffa26a Compare February 21, 2024 12:06

yhabteab removed their request for review February 21, 2024 12:53

Al2Klimov requested a review from julianbrost February 22, 2024 09:10

Al2Klimov mentioned this pull request Jun 5, 2024

Fix broken downtime comment sync #10000

Open

2 tasks

Al2Klimov self-assigned this Aug 20, 2024

julianbrost self-assigned this Aug 27, 2024

julianbrost reviewed Aug 29, 2024

View reviewed changes

lib/base/type.cpp Outdated Show resolved Hide resolved

julianbrost removed their assignment Aug 29, 2024

Al2Klimov force-pushed the Type-SortByLoadDependencies branch from 3ffa26a to 19aa779 Compare September 2, 2024 15:48

Al2Klimov requested a review from julianbrost September 2, 2024 15:49

Al2Klimov force-pushed the Type-SortByLoadDependencies branch from 19aa779 to e1f5ab8 Compare September 3, 2024 14:47

julianbrost requested changes Sep 6, 2024

View reviewed changes

yhabteab previously requested changes Sep 6, 2024

View reviewed changes

yhabteab mentioned this pull request Sep 10, 2024

Sort config types by their load dependencies once #10148

Merged

Al2Klimov added 2 commits September 16, 2024 12:24

Introduce Type::GetAllConfigTypesSortedByLoadDependencies()

4037b60

ConfigItem::CommitNewItems(): pre-sort types by their load dependenci…

a422419

…es once to avoid complicated nested loops, iterating over the same types and checking dependencies over and over, skipping already completed ones.

Al2Klimov force-pushed the Type-SortByLoadDependencies branch from e1f5ab8 to a422419 Compare September 16, 2024 10:25

Al2Klimov requested a review from yhabteab September 16, 2024 10:38

yhabteab removed their request for review September 16, 2024 10:55

Al2Klimov mentioned this pull request Sep 16, 2024

Type#GetLoadDependencies(): VERIFY() that no nullptr is returned #10155

Merged

yhabteab closed this in #10148 Sep 26, 2024

icinga-probot bot deleted the Type-SortByLoadDependencies branch September 26, 2024 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ConfigItem::CommitNewItems(): pre-sort types by their load dependencies once #10003

ConfigItem::CommitNewItems(): pre-sort types by their load dependencies once #10003

Al2Klimov commented Feb 15, 2024 •

edited

Loading

julianbrost Sep 6, 2024

julianbrost Sep 6, 2024

julianbrost Sep 6, 2024

julianbrost Sep 6, 2024

julianbrost Sep 6, 2024

yhabteab Sep 6, 2024

julianbrost Sep 6, 2024 •

edited

Loading

yhabteab Sep 6, 2024 •

edited

Loading

julianbrost Sep 6, 2024

yhabteab Sep 6, 2024

yhabteab Sep 6, 2024

Al2Klimov Sep 16, 2024

yhabteab Sep 6, 2024

yhabteab Sep 6, 2024

yhabteab Sep 6, 2024

Al2Klimov Sep 16, 2024

		static std::vector<Type::Ptr> GetAllTypes();
		static const std::vector<Ptr>& GetAllTypesSortedByLoadDependencies();

	types.erase(std::remove_if(types.begin(), types.end(), [](auto& type) {
	return !ConfigObject::TypeInstance->IsAssignableFrom(type);
	}), types.end());

ConfigItem::CommitNewItems(): pre-sort types by their load dependencies once #10003

ConfigItem::CommitNewItems(): pre-sort types by their load dependencies once #10003

Conversation

Al2Klimov commented Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianbrost Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

yhabteab Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Al2Klimov commented Feb 15, 2024 •

edited

Loading

julianbrost Sep 6, 2024 •

edited

Loading

yhabteab Sep 6, 2024 •

edited

Loading