Skip to content

Commit

Permalink
Resolve goal according to strict setting first
Browse files Browse the repository at this point in the history 
It should help to force solver to prefer latest solution rather
then minimalistic solution skipping something.

Performance improvement for operation that does not generate
any problem, because transaction is resolved only once with
strict setting.

Related: https://bugzilla.redhat.com/show_bug.cgi?id=2272257
  • Loading branch information
@j-mracek
j-mracek committed Apr 22, 2024
1 parent 077c7b1 commit e170d80
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 37 deletions.
4 changes: 1 addition & 3 deletions libdnf5/base/goal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2462,7 +2462,7 @@ base::Transaction Goal::resolve() {
}
}

ret |= p_impl->rpm_goal.resolve();
transaction.p_impl->resolve_and_set_transaction(p_impl->rpm_goal, module_sack, ret);

// Write debug solver data
// Note: Modules debug data are handled separately when resolving module goal in ModuleSack::Impl::module_solve()
Expand Down Expand Up @@ -2490,8 +2490,6 @@ base::Transaction Goal::resolve() {

//TODO(amatej): Add conditional check that no extra packages were added by the solver

transaction.p_impl->set_transaction(p_impl->rpm_goal, module_sack, ret);

return transaction;
}

Expand Down
72 changes: 40 additions & 32 deletions libdnf5/base/transaction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -379,32 +379,38 @@ std::vector<std::string> Transaction::get_gpg_signature_problems() const noexcep
return p_impl->signature_problems;
}

void Transaction::Impl::set_transaction(
rpm::solv::GoalPrivate & solved_goal, module::ModuleSack & module_sack, GoalProblem problems) {
auto solver_problems = process_solver_problems(base, solved_goal);
if (!solver_problems.empty()) {
add_resolve_log(GoalProblem::SOLVER_ERROR, solver_problems);
} else {
// TODO(jmracek) To improve performance add a test whether it make sense to resolve transaction in strict mode
// Test whether there were skipped jobs or used not the best candidates due to broken dependencies
rpm::solv::GoalPrivate solved_goal_copy(solved_goal);
solved_goal_copy.set_run_in_strict_mode(true);
solved_goal_copy.resolve();
auto solver_problems_strict = process_solver_problems(base, solved_goal_copy);
if (!solver_problems_strict.empty()) {
void Transaction::Impl::resolve_and_set_transaction(
rpm::solv::GoalPrivate & goal_to_resolve, module::ModuleSack & module_sack, GoalProblem problems) {
this->problems = problems;

// TODO(jmracek) To improve performance add a test whether it make sense to resolve transaction in strict mode
// Test whether there were skipped jobs or used not the best candidates due to broken dependencies
goal_to_resolve.set_run_in_strict_mode(true);
GoalProblem resolve_problems = goal_to_resolve.resolve();

auto solver_problems_strict = process_solver_problems(base, goal_to_resolve);
if (!solver_problems_strict.empty()) {
goal_to_resolve.set_run_in_strict_mode(false);
resolve_problems = goal_to_resolve.resolve();

auto solver_problems = process_solver_problems(base, goal_to_resolve);
// Report problems from strict transaction, because it might contain more information
if (solver_problems.empty()) {
add_resolve_log(GoalProblem::SOLVER_PROBLEM_STRICT_RESOLVEMENT, solver_problems_strict);
} else {
add_resolve_log(GoalProblem::SOLVER_ERROR, solver_problems_strict);
}
}
this->problems = problems;
this->problems |= resolve_problems;

if ((problems & GoalProblem::MODULE_SOLVER_ERROR) != GoalProblem::NO_PROBLEM ||
((problems & GoalProblem::MODULE_SOLVER_ERROR_LATEST) != GoalProblem::NO_PROBLEM &&
if ((this->problems & GoalProblem::MODULE_SOLVER_ERROR) != GoalProblem::NO_PROBLEM ||
((this->problems & GoalProblem::MODULE_SOLVER_ERROR_LATEST) != GoalProblem::NO_PROBLEM &&
base->get_config().get_best_option().get_value())) {
// There is a fatal error in resolving modules
return;
}

auto transaction = solved_goal.get_transaction();
auto transaction = goal_to_resolve.get_transaction();
libsolv_transaction = transaction ? transaction_create_clone(transaction) : nullptr;
if (!libsolv_transaction) {
return;
Expand All @@ -418,29 +424,31 @@ void Transaction::Impl::set_transaction(

// The order of packages in the vector matters, we rely on outbound actions
// being at the end in Transaction::Impl::run()
for (auto id : solved_goal.list_installs()) {
packages.emplace_back(
make_transaction_package(id, TransactionPackage::Action::INSTALL, solved_goal, replaced, installed_query));
for (auto id : goal_to_resolve.list_installs()) {
packages.emplace_back(make_transaction_package(
id, TransactionPackage::Action::INSTALL, goal_to_resolve, replaced, installed_query));
}

for (auto id : solved_goal.list_reinstalls()) {
for (auto id : goal_to_resolve.list_reinstalls()) {
packages.emplace_back(make_transaction_package(
id, TransactionPackage::Action::REINSTALL, solved_goal, replaced, installed_query));
id, TransactionPackage::Action::REINSTALL, goal_to_resolve, replaced, installed_query));
}

for (auto id : solved_goal.list_upgrades()) {
packages.emplace_back(
make_transaction_package(id, TransactionPackage::Action::UPGRADE, solved_goal, replaced, installed_query));
for (auto id : goal_to_resolve.list_upgrades()) {
packages.emplace_back(make_transaction_package(
id, TransactionPackage::Action::UPGRADE, goal_to_resolve, replaced, installed_query));
}

for (auto id : solved_goal.list_downgrades()) {
for (auto id : goal_to_resolve.list_downgrades()) {
packages.emplace_back(make_transaction_package(
id, TransactionPackage::Action::DOWNGRADE, solved_goal, replaced, installed_query));
id, TransactionPackage::Action::DOWNGRADE, goal_to_resolve, replaced, installed_query));
}

for (auto id : solved_goal.list_removes()) {
for (auto id : goal_to_resolve.list_removes()) {
packages.emplace_back(TransactionPackage(
rpm::Package(base, rpm::PackageId(id)), TransactionPackage::Action::REMOVE, solved_goal.get_reason(id)));
rpm::Package(base, rpm::PackageId(id)),
TransactionPackage::Action::REMOVE,
goal_to_resolve.get_reason(id)));
}

// Add replaced packages to transaction
Expand All @@ -454,13 +462,13 @@ void Transaction::Impl::set_transaction(
}

// Add environmental groups to the transaction
for (auto & [environment, action, reason, with_optional] : solved_goal.list_environments()) {
for (auto & [environment, action, reason, with_optional] : goal_to_resolve.list_environments()) {
TransactionEnvironment tsenv(environment, action, reason, with_optional);
environments.emplace_back(std::move(tsenv));
}

// Add groups to the transaction
for (auto & [group, action, reason, package_types] : solved_goal.list_groups()) {
for (auto & [group, action, reason, package_types] : goal_to_resolve.list_groups()) {
TransactionGroup tsgrp(group, action, reason, package_types);
groups.emplace_back(std::move(tsgrp));
}
Expand Down Expand Up @@ -493,7 +501,7 @@ void Transaction::Impl::set_transaction(
}

// Add reason change actions to the transaction
for (auto & [pkg, reason, group_id] : solved_goal.list_reason_changes()) {
for (auto & [pkg, reason, group_id] : goal_to_resolve.list_reason_changes()) {
TransactionPackage tspkg(pkg, TransactionPackage::Action::REASON_CHANGE, reason, group_id);
packages.emplace_back(std::move(tspkg));
}
Expand Down
6 changes: 4 additions & 2 deletions libdnf5/base/transaction_impl.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,10 @@ class Transaction::Impl {

Impl & operator=(const Impl & other);

/// Set transaction according resolved goal and problems to EventLog
void set_transaction(rpm::solv::GoalPrivate & solved_goal, module::ModuleSack & module_sack, GoalProblem problems);
/// Resolve goal_to_resolve first using strict setting and then user defined setting,
/// then set transaction according the resolved goal and report problems to EventLog
void resolve_and_set_transaction(
rpm::solv::GoalPrivate & goal_to_resolve, module::ModuleSack & module_sack, GoalProblem problems);

TransactionPackage make_transaction_package(
Id id,
Expand Down

0 comments on commit e170d80

Please sign in to comment.