libstore: remove Worker::removeGoal

we can use our newfound powers of Goal::work Is A Real Promise to remove
completed goals from continuation promises. apart from being much easier
to follow it's also a lot more efficient because we have the iterator to
the item we are trying to remove, skipping a linear search of the cache.

Change-Id: Ie0190d051c5f4b81304d98db478348b20c209df5
This commit is contained in:
eldritch horrors 2024-10-05 00:38:35 +02:00
parent 9adf6f4568
commit 649d8cd08f
3 changed files with 20 additions and 48 deletions

View file

@ -82,11 +82,6 @@ struct Goal
*/ */
std::string name; std::string name;
struct WorkResult;
// for use by Worker and Goal only. will go away once work() is a promise.
kj::Own<kj::PromiseFulfiller<Result<WorkResult>>> notify;
protected: protected:
AsyncSemaphore::Token slotToken; AsyncSemaphore::Token slotToken;

View file

@ -71,25 +71,37 @@ std::pair<std::shared_ptr<G>, kj::Promise<Result<Goal::WorkResult>>> Worker::mak
// and then we only want to recreate the goal *once*. concurrent accesses // and then we only want to recreate the goal *once*. concurrent accesses
// to the worker are not sound, we want to catch them if at all possible. // to the worker are not sound, we want to catch them if at all possible.
for ([[maybe_unused]] auto _attempt : {1, 2}) { for ([[maybe_unused]] auto _attempt : {1, 2}) {
auto & goal_weak = it->second; auto & cachedGoal = it->second;
auto goal = goal_weak.goal.lock(); auto & goal = cachedGoal.goal;
if (!goal) { if (!goal) {
goal = create(); goal = create();
goal_weak.goal = goal;
// do not start working immediately. if we are not yet running we // do not start working immediately. if we are not yet running we
// may create dependencies as though they were toplevel goals, in // may create dependencies as though they were toplevel goals, in
// which case the dependencies will not report build errors. when // which case the dependencies will not report build errors. when
// we are running we may be called for this same goal more times, // we are running we may be called for this same goal more times,
// and then we want to modify rather than recreate when possible. // and then we want to modify rather than recreate when possible.
goal_weak.promise = kj::evalLater([goal] { return goal->work(); }).fork(); auto removeWhenDone = [goal, &map, it] {
childStarted(goal, goal_weak.promise.addBranch()); // c++ lambda coroutine capture semantics are *so* fucked up.
return [](auto goal, auto & map, auto it) -> kj::Promise<Result<Goal::WorkResult>> {
auto result = co_await goal->work();
// a concurrent call to makeGoalCommon may have reset our
// cached goal and replaced it with a new instance. don't
// remove the goal in this case, otherwise we will crash.
if (goal == it->second.goal) {
map.erase(it);
}
co_return result;
}(goal, map, it);
};
cachedGoal.promise = kj::evalLater(std::move(removeWhenDone)).fork();
childStarted(goal, cachedGoal.promise.addBranch());
} else { } else {
if (!modify(*goal)) { if (!modify(*goal)) {
goal_weak = {}; cachedGoal = {};
continue; continue;
} }
} }
return {goal, goal_weak.promise.addBranch()}; return {goal, cachedGoal.promise.addBranch()};
} }
assert(false && "could not make a goal. possible concurrent worker access"); assert(false && "could not make a goal. possible concurrent worker access");
} }
@ -184,44 +196,14 @@ std::pair<GoalPtr, kj::Promise<Result<Goal::WorkResult>>> Worker::makeGoal(const
} }
template<typename G>
static void removeGoal(std::shared_ptr<G> goal, auto & goalMap)
{
/* !!! inefficient */
for (auto i = goalMap.begin();
i != goalMap.end(); )
if (i->second.goal.lock() == goal) {
auto j = i; ++j;
goalMap.erase(i);
i = j;
}
else ++i;
}
void Worker::goalFinished(GoalPtr goal, Goal::WorkResult & f) void Worker::goalFinished(GoalPtr goal, Goal::WorkResult & f)
{ {
permanentFailure |= f.permanentFailure; permanentFailure |= f.permanentFailure;
timedOut |= f.timedOut; timedOut |= f.timedOut;
hashMismatch |= f.hashMismatch; hashMismatch |= f.hashMismatch;
checkMismatch |= f.checkMismatch; checkMismatch |= f.checkMismatch;
removeGoal(goal);
} }
void Worker::removeGoal(GoalPtr goal)
{
if (auto drvGoal = std::dynamic_pointer_cast<DerivationGoal>(goal))
nix::removeGoal(drvGoal, derivationGoals);
else if (auto subGoal = std::dynamic_pointer_cast<PathSubstitutionGoal>(goal))
nix::removeGoal(subGoal, substitutionGoals);
else if (auto subGoal = std::dynamic_pointer_cast<DrvOutputSubstitutionGoal>(goal))
nix::removeGoal(subGoal, drvOutputSubstitutionGoals);
else
assert(false);
}
void Worker::childStarted(GoalPtr goal, kj::Promise<Result<Goal::WorkResult>> promise) void Worker::childStarted(GoalPtr goal, kj::Promise<Result<Goal::WorkResult>> promise)
{ {
children.add(promise children.add(promise

View file

@ -95,7 +95,7 @@ private:
template<typename G> template<typename G>
struct CachedGoal struct CachedGoal
{ {
std::weak_ptr<G> goal; std::shared_ptr<G> goal;
kj::ForkedPromise<Result<Goal::WorkResult>> promise{nullptr}; kj::ForkedPromise<Result<Goal::WorkResult>> promise{nullptr};
}; };
/** /**
@ -134,11 +134,6 @@ private:
void goalFinished(GoalPtr goal, Goal::WorkResult & f); void goalFinished(GoalPtr goal, Goal::WorkResult & f);
/**
* Remove a dead goal.
*/
void removeGoal(GoalPtr goal);
/** /**
* Registers a running child process. * Registers a running child process.
*/ */