[Aptitude-svn-commit] r3980 - in branches/aptitude-0.3/aptitude: .
src/generic/problemresolver
Daniel Burrows
dburrows at costa.debian.org
Mon Aug 29 20:23:22 UTC 2005
Author: dburrows
Date: Mon Aug 29 20:23:19 2005
New Revision: 3980
Modified:
branches/aptitude-0.3/aptitude/ChangeLog
branches/aptitude-0.3/aptitude/src/generic/problemresolver/problemresolver.h
Log:
Drastically reduce the runtime of the search algorithm with a little profiling
and a little thinking: the expensive step that was killing
performance was the process of constructing the new set of broken dependencies,
so the algorithm now postpones that until the last possible moment
(thus avoiding doing it unnecessarily).
Modified: branches/aptitude-0.3/aptitude/ChangeLog
==============================================================================
--- branches/aptitude-0.3/aptitude/ChangeLog (original)
+++ branches/aptitude-0.3/aptitude/ChangeLog Mon Aug 29 20:23:19 2005
@@ -1,5 +1,23 @@
2005-08-29 Daniel Burrows <dburrows at debian.org>
+ * src/generic/problemresolver/problemresolver.h:
+
+ Eliminate the major bottleneck in the resolver code: rather than
+ constructing full solutions while searching for forcings and
+ unresolved dependencies, just construct as much of the solution
+ as is necessary. In particular, the resolver now just
+ constructs a new actions map to search for conflicts.
+
+ With this change, the resolver goes from taking several minutes
+ to return a first result from 'dist-upgrade' to returning it
+ almost instantaneously. The clue to finding this bottleneck was
+ the observation that the profiler reported a huge amount of time
+ being spent in broken_under, but that in fact hardly any time at
+ all was being spent in each individual call to that routine; the
+ problem was that it was being called millions of times to
+ construct the sets of broken dependencies for successor
+ solutions that were being discarded.
+
* src/generic/problemresolver/solution.h:
Dereferencing a revdep_iterator is a bit expensive; cache the
@@ -7,7 +25,7 @@
* src/generic/immset.h:
- Add a convenience routuine to check if a value is in the domain
+ Add a convenience routine to check if a value is in the domain
of a mapping.
2005-08-28 Daniel Burrows <dburrows at debian.org>
Modified: branches/aptitude-0.3/aptitude/src/generic/problemresolver/problemresolver.h
==============================================================================
--- branches/aptitude-0.3/aptitude/src/generic/problemresolver/problemresolver.h (original)
+++ branches/aptitude-0.3/aptitude/src/generic/problemresolver/problemresolver.h Mon Aug 29 20:23:19 2005
@@ -1491,6 +1491,63 @@
}
}
+ /** Generate a solution and push it onto an encapsulated vector of
+ * solutions.
+ */
+ class real_generator
+ {
+ std::vector<solution> ⌖
+ public:
+ real_generator(std::vector<solution> &_target)
+ :target(_target)
+ {
+ }
+
+ template<typename a_iter, class u_iter>
+ void make_successor(const solution &s,
+ const a_iter &abegin, const a_iter &aend,
+ const u_iter &ubegin, const u_iter &uend,
+ const PackageUniverse &universe,
+ const solution_weights &weights) const
+ {
+ target.push_back(solution::successor(s,
+ abegin, aend,
+ ubegin, uend,
+ universe, weights));
+ }
+ };
+
+ /** Don't actually generate solutions; instead, just count how many
+ * *would* be generated. Each time a solution would be generated,
+ * the integer referenced by this object is incremented (it is
+ * never set to 0; if you need it to be initialized to 0, do that
+ * yourself).
+ */
+ class null_generator
+ {
+ int &count;
+ public:
+ null_generator(int &_count)
+ :count(_count)
+ {
+ }
+
+ template<typename a_iter, class u_iter>
+ void make_successor(const solution &s,
+ const a_iter &abegin, const a_iter &aend,
+ const u_iter &ubegin, const u_iter &uend,
+ const PackageUniverse &universe,
+ const solution_weights &weights) const
+ {
+ ++count;
+ }
+
+ int get_count() const
+ {
+ return count;
+ }
+ };
+
/** Convenience routine for the below: try to generate a successor
* by installing a single package version. NB: assumes that the
* solution's actions have dense identifiers (i.e., less than
@@ -1503,14 +1560,17 @@
* of an action on the source of a dependency
* \param conflict a map to which conflictors for this version, if
* any, will be added.
- * \param out a vector onto which the successor will be pushed.
+ *
+ * \param generator an object supporting the make_successor() routine,
+ * as real_generator and null_generator above.
*/
+ template<typename SolutionGenerator>
void generate_single_successor(const solution &s,
const dep &d,
const version &v,
bool from_dep_source,
std::map<package, action> &conflict,
- std::vector<solution> &out) const
+ const SolutionGenerator &generator) const
{
action conflictor;
@@ -1533,16 +1593,21 @@
{
action act(v, d, from_dep_source, newid);
- solution new_sol = solution::successor(s, &act, &act+1,
- (dep *) 0,
- (dep *) 0,
- universe, weights);
+ // Speculatively form the new set of actions; doing this
+ // rather than forming the whole solution allows us to avoid
+ // several rather expensive steps in the successor routine
+ // (most notably the formation of the new broken packages
+ // set).
+ std::map<package, action> new_acts = s.get_actions();
+ new_acts[v.get_package()] = act;
typename std::set<std::map<package, action> >::const_iterator
- found = matches_any_conflict(new_sol.get_actions());
+ found = matches_any_conflict(new_acts);
if(found == conflicts.end())
- out.push_back(new_sol);
+ generator.make_successor(s, &act, &act+1,
+ (dep *) 0, (dep *) 0,
+ universe, weights);
else
{
if(debug)
@@ -1578,10 +1643,11 @@
*
* \param out a vector onto which the successors should be pushed.
*/
+ template<typename SolutionGenerator>
void generate_successors(const solution &s,
const dep &d,
std::map<package, action> &conflict,
- std::vector<solution> &out) const
+ const SolutionGenerator &generator) const
{
version source = d.get_source();
typename std::map<package, action>::const_iterator
@@ -1597,18 +1663,18 @@
for(typename package::version_iterator vi = source.get_package().versions_begin();
!vi.end(); ++vi)
if(*vi != source)
- generate_single_successor(s, d, *vi, true, conflict, out);
+ generate_single_successor(s, d, *vi, true, conflict, generator);
}
// Now try installing each target of the dependency.
for(typename dep::solver_iterator si = d.solvers_begin();
!si.end(); ++si)
- generate_single_successor(s, d, *si, false, conflict, out);
+ generate_single_successor(s, d, *si, false, conflict, generator);
// Finally, maybe we can leave this dependency unresolved.
if(d.is_soft())
- out.push_back(solution::successor(s, (action *) 0, (action *) 0,
- &d, &d+1, universe, weights));
+ generator.make_successor(s, (action *) 0, (action *) 0,
+ &d, &d+1, universe, weights);
}
/** Processes the given solution by enqueuing its successor nodes
@@ -1620,7 +1686,6 @@
// Any forcings are immediately applied to 'curr', so that
// forcings are performed ASAP.
solution curr = s;
- std::vector<std::pair<dep, std::vector<solution> > > workingq;
assert(!s.get_broken().empty());
@@ -1642,7 +1707,6 @@
// Remember the solution whose broken dependencies we're
// iterating over.
solution starting_solution = curr;
- workingq.clear();
for(typename std::set<dep>::const_iterator bi=starting_solution.get_broken().begin();
bi!=starting_solution.get_broken().end(); ++bi)
@@ -1680,14 +1744,14 @@
std::map<package, action> conflict;
- workingq.push_back(std::pair<dep, std::vector<solution> >(*bi, std::vector<solution>()));
- generate_successors(curr, *bi, conflict, workingq.back().second);
+ int num_successors = 0;
+ generate_successors(curr, *bi, conflict,
+ null_generator(num_successors));
- int sz = workingq.back().second.size();
- if(sz == 0)
+ if(num_successors == 0)
{
if(debug)
{
@@ -1703,12 +1767,21 @@
return;
}
- else if(sz == 1)
+ else if(num_successors == 1)
{
if(debug)
std::cout << "Forced resolution of " << *bi << std::endl;
- curr = workingq.back().second.front();
+ std::vector<solution> v;
+ real_generator g(v);
+ // NB: this may do redundant work adding to 'conflict'.
+ // Use a generator object to avoid that?
+ generate_successors(curr, *bi, conflict,
+ real_generator(v));
+
+ assert(v.size() == 1);
+
+ curr = v.back();
done = false;
}
}
@@ -1718,34 +1791,40 @@
// First try to enqueue stuff related to a dependency that the
// user constrained; then just go for a free-for-all.
+ for(typename std::set<dep>::const_iterator bi=curr.get_broken().begin();
+ bi!=curr.get_broken().end() && (nsols == 0 ||
+ nsols < max_successors); ++bi)
- for(typename std::vector<std::pair<dep, std::vector<solution> > >::const_iterator
- wqi = workingq.begin();
- wqi != workingq.end() && (nsols == 0 ||
- nsols < max_successors);
- ++wqi)
- if(impinges_user_constraint(wqi->first))
+ if(impinges_user_constraint(*bi))
{
+ // Is it possible to take this out somehow?
+ std::map<package, action> conflict;
+
if(debug)
- std::cout << "Generating successors for " << wqi->first
+ std::cout << "Generating successors for " << *bi
<< std::endl;
- try_enqueue(wqi->second);
- nsols += wqi->second.size();
+ std::vector<solution> v;
+ generate_successors(curr, *bi, conflict, real_generator(v));
+ try_enqueue(v);
+ nsols += v.size();
}
- for(typename std::vector<std::pair<dep, std::vector<solution> > >::const_iterator
- wqi = workingq.begin();
- wqi != workingq.end() && (nsols == 0 ||
- nsols < max_successors);
- ++wqi)
+ for(typename std::set<dep>::const_iterator bi=curr.get_broken().begin();
+ bi!=curr.get_broken().end() && (nsols == 0 ||
+ nsols < max_successors); ++bi)
{
+ // Is it possible to take this out somehow?
+ std::map<package, action> conflict;
+
if(debug)
- std::cout << "Generating successors for " << wqi->first
+ std::cout << "Generating successors for " << *bi
<< std::endl;
- try_enqueue(wqi->second);
- nsols += wqi->second.size();
+ std::vector<solution> v;
+ generate_successors(curr, *bi, conflict, real_generator(v));
+ try_enqueue(v);
+ nsols += v.size();
}
}
public:
More information about the Aptitude-svn-commit
mailing list