Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa4adf0
Introduce Type::SortByLoadDependencies()
Al2Klimov Feb 14, 2024
2ddd3d9
Test Type::SortByLoadDependencies()
Al2Klimov Feb 14, 2024
3ffa26a
ConfigItem::CommitNewItems(): pre-sort types by their load dependenci…
Al2Klimov Feb 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions lib/base/type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,43 @@ std::vector<Type::Ptr> Type::GetAllTypes()
return types;
}

std::vector<Type::Ptr> Type::SortByLoadDependencies(const std::vector<Type::Ptr>& types)
{
std::vector<Type::Ptr> sorted;
std::unordered_set<Type*> pending, loaded;
bool ready;

sorted.reserve(types.size());
pending.reserve(types.size());
loaded.reserve(types.size());

for (auto& type : types) {
pending.emplace(type.get());
}

while (!pending.empty()) {
auto type (*pending.begin());

do {
ready = true;

for (auto dep : type->GetLoadDependencies()) {
if (loaded.find(dep) == loaded.end() && pending.find(dep) != pending.end()) {
type = dep;
ready = false;
break;
}
}
} while (!ready);

sorted.emplace_back(type);
loaded.emplace(type);
pending.erase(type);
}

return sorted;
}

String Type::GetPluralName() const
{
String name = GetName();
Expand Down
1 change: 1 addition & 0 deletions lib/base/type.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class Type : public Object
static void Register(const Type::Ptr& type);
static Type::Ptr GetByName(const String& name);
static std::vector<Type::Ptr> GetAllTypes();
static std::vector<Type::Ptr> SortByLoadDependencies(const std::vector<Type::Ptr>& types);

void SetField(int id, const Value& value, bool suppress_events = false, const Value& cookie = Empty) override;
Value GetField(int id) const override;
Expand Down
203 changes: 81 additions & 122 deletions lib/config/configitem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,180 +450,139 @@ bool ConfigItem::CommitNewItems(const ActivationContext::Ptr& context, WorkQueue
<< "Committing " << total << " new items.";
#endif /* I2_DEBUG */

std::set<Type::Ptr> types;
std::set<Type::Ptr> completed_types;
std::vector<Type::Ptr> types;
int itemsCount {0};

for (const Type::Ptr& type : Type::GetAllTypes()) {
if (ConfigObject::TypeInstance->IsAssignableFrom(type))
types.insert(type);
types.emplace_back(type);
Comment on lines 456 to +458
Copy link
Member

@yhabteab yhabteab Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of copying the types around, you can access Type::GetAllTypes() directly within your new method or pass the types without having to copy them if you need the arguments for your unittest. You can then perform this check below when they are actually loaded.

icinga2/lib/config/configitem.cpp

Lines 456 to 459 in 9726620

for (const Type::Ptr& type : Type::GetAllTypes()) {
if (ConfigObject::TypeInstance->IsAssignableFrom(type))
types.emplace_back(type);
}

icinga2/lib/base/type.cpp

Lines 85 to 87 in 9726620

for (auto& type : types) {
pending.emplace(type.get());
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should address such in

one nice day.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't suggesting you to remove this check, I was just pointing out that you are unnecessarily copying around the types and should avoid doing that. It's not something that already existed and simply saying it will be fixed "one nice day" isn't an argument.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But not all types are config types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But not all types are config types.

Why would that matter to you?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CONFIG items get committed here, of course only CONFIG types are relevant here. Btw. that preserved check IS "something that already existed" (just like #10003 (comment)).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you get my point her! My suggestion is to drop this loop entirely and move the check down here instead. Your sort method shouldn't care whether a type is assignable to a config object or not.

}

while (types.size() != completed_types.size()) {
for (const Type::Ptr& type : types) {
if (completed_types.find(type) != completed_types.end())
continue;

bool unresolved_dep = false;

/* skip this type (for now) if there are unresolved load dependencies */
for (auto pLoadDep : type->GetLoadDependencies()) {
if (types.find(pLoadDep) != types.end() && completed_types.find(pLoadDep) == completed_types.end()) {
unresolved_dep = true;
break;
}
}
types = Type::SortByLoadDependencies(types);

if (unresolved_dep)
continue;

std::atomic<int> committed_items(0);
std::mutex newItemsMutex;
for (auto& type : types) {
std::atomic<int> committed_items(0);
std::mutex newItemsMutex;

{
auto items (itemsByType.find(type.get()));
{
auto items (itemsByType.find(type.get()));

if (items != itemsByType.end()) {
upq.ParallelFor(items->second, [&committed_items, &newItems, &newItemsMutex](const ItemPair& ip) {
const ConfigItem::Ptr& item = ip.first;
if (items != itemsByType.end()) {
upq.ParallelFor(items->second, [&committed_items, &newItems, &newItemsMutex](const ItemPair& ip) {
const ConfigItem::Ptr& item = ip.first;

if (!item->Commit(ip.second)) {
if (item->IsIgnoreOnError()) {
item->Unregister();
}

return;
if (!item->Commit(ip.second)) {
if (item->IsIgnoreOnError()) {
item->Unregister();
}

committed_items++;
return;
}

std::unique_lock<std::mutex> lock(newItemsMutex);
newItems.emplace_back(item);
});
committed_items++;

upq.Join();
}
}
std::unique_lock<std::mutex> lock(newItemsMutex);
newItems.emplace_back(item);
});

itemsCount += committed_items;
upq.Join();
}
}

completed_types.insert(type);
itemsCount += committed_items;

#ifdef I2_DEBUG
if (committed_items > 0)
Log(LogDebug, "configitem")
<< "Committed " << committed_items << " items of type '" << type->GetName() << "'.";
if (committed_items > 0)
Log(LogDebug, "configitem")
<< "Committed " << committed_items << " items of type '" << type->GetName() << "'.";
#endif /* I2_DEBUG */

if (upq.HasExceptions())
return false;
}
if (upq.HasExceptions())
return false;
}

#ifdef I2_DEBUG
Log(LogDebug, "configitem")
<< "Committed " << itemsCount << " items.";
#endif /* I2_DEBUG */

completed_types.clear();

while (types.size() != completed_types.size()) {
for (const Type::Ptr& type : types) {
if (completed_types.find(type) != completed_types.end())
continue;

bool unresolved_dep = false;

/* skip this type (for now) if there are unresolved load dependencies */
for (auto pLoadDep : type->GetLoadDependencies()) {
if (types.find(pLoadDep) != types.end() && completed_types.find(pLoadDep) == completed_types.end()) {
unresolved_dep = true;
break;
}
}

if (unresolved_dep)
continue;

std::atomic<int> notified_items(0);
for (const Type::Ptr& type : types) {
std::atomic<int> notified_items(0);

{
auto items (itemsByType.find(type.get()));
{
auto items (itemsByType.find(type.get()));

if (items != itemsByType.end()) {
upq.ParallelFor(items->second, [&notified_items](const ItemPair& ip) {
const ConfigItem::Ptr& item = ip.first;
if (items != itemsByType.end()) {
upq.ParallelFor(items->second, [&notified_items](const ItemPair& ip) {
const ConfigItem::Ptr& item = ip.first;

if (!item->m_Object)
return;
if (!item->m_Object)
return;

try {
item->m_Object->OnAllConfigLoaded();
notified_items++;
} catch (const std::exception& ex) {
if (!item->m_IgnoreOnError)
throw;
try {
item->m_Object->OnAllConfigLoaded();
notified_items++;
} catch (const std::exception& ex) {
if (!item->m_IgnoreOnError)
throw;

Log(LogNotice, "ConfigObject")
<< "Ignoring config object '" << item->m_Name << "' of type '" << item->m_Type->GetName() << "' due to errors: " << DiagnosticInformation(ex);
Log(LogNotice, "ConfigObject")
<< "Ignoring config object '" << item->m_Name << "' of type '" << item->m_Type->GetName() << "' due to errors: " << DiagnosticInformation(ex);

item->Unregister();
item->Unregister();

{
std::unique_lock<std::mutex> lock(item->m_Mutex);
item->m_IgnoredItems.push_back(item->m_DebugInfo.Path);
}
{
std::unique_lock<std::mutex> lock(item->m_Mutex);
item->m_IgnoredItems.push_back(item->m_DebugInfo.Path);
}
});
}
});

upq.Join();
}
upq.Join();
}

completed_types.insert(type);
}

#ifdef I2_DEBUG
if (notified_items > 0)
Log(LogDebug, "configitem")
<< "Sent OnAllConfigLoaded to " << notified_items << " items of type '" << type->GetName() << "'.";
if (notified_items > 0)
Log(LogDebug, "configitem")
<< "Sent OnAllConfigLoaded to " << notified_items << " items of type '" << type->GetName() << "'.";
#endif /* I2_DEBUG */

if (upq.HasExceptions())
return false;
if (upq.HasExceptions())
return false;

notified_items = 0;
for (auto loadDep : type->GetLoadDependencies()) {
auto items (itemsByType.find(loadDep));
notified_items = 0;
for (auto loadDep : type->GetLoadDependencies()) {
auto items (itemsByType.find(loadDep));

if (items != itemsByType.end()) {
upq.ParallelFor(items->second, [&type, &notified_items](const ItemPair& ip) {
const ConfigItem::Ptr& item = ip.first;
if (items != itemsByType.end()) {
upq.ParallelFor(items->second, [&type, &notified_items](const ItemPair& ip) {
const ConfigItem::Ptr& item = ip.first;

if (!item->m_Object)
return;
if (!item->m_Object)
return;

ActivationScope ascope(item->m_ActivationContext);
item->m_Object->CreateChildObjects(type);
notified_items++;
});
}
ActivationScope ascope(item->m_ActivationContext);
item->m_Object->CreateChildObjects(type);
notified_items++;
});
}
}

upq.Join();
upq.Join();

#ifdef I2_DEBUG
if (notified_items > 0)
Log(LogDebug, "configitem")
<< "Sent CreateChildObjects to " << notified_items << " items of type '" << type->GetName() << "'.";
if (notified_items > 0)
Log(LogDebug, "configitem")
<< "Sent CreateChildObjects to " << notified_items << " items of type '" << type->GetName() << "'.";
#endif /* I2_DEBUG */

if (upq.HasExceptions())
return false;
if (upq.HasExceptions())
return false;

// Make sure to activate any additionally generated items
if (!CommitNewItems(context, upq, newItems))
return false;
}
// Make sure to activate any additionally generated items
if (!CommitNewItems(context, upq, newItems))
return false;
}

return true;
Expand Down
19 changes: 19 additions & 0 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,25 @@ add_boost_test(base
base_type/assign
base_type/byname
base_type/instantiate
base_type/sortbyloaddependencies0
base_type/sortbyloaddependencies1
base_type/sortbyloaddependencies2related
base_type/sortbyloaddependencies2unrelated
base_type/sortbyloaddependencies3related
base_type/sortbyloaddependencies3unrelated
base_type/sortbyloaddependencies4related
base_type/sortbyloaddependencies4unrelated
base_type/sortbyloaddependencies5related
base_type/sortbyloaddependencies5unrelated
base_type/sortbyloaddependencies1_withforeign
base_type/sortbyloaddependencies2related_withforeign
base_type/sortbyloaddependencies2unrelated_withforeign
base_type/sortbyloaddependencies3related_withforeign
base_type/sortbyloaddependencies3unrelated_withforeign
base_type/sortbyloaddependencies4related_withforeign
base_type/sortbyloaddependencies4unrelated_withforeign
base_type/sortbyloaddependencies5related_withforeign
base_type/sortbyloaddependencies5unrelated_withforeign
base_utility/parse_version
base_utility/compare_version
base_utility/comparepasswords_works
Expand Down
Loading
Loading