From 5c74a6147b4b81dc5b173f190f02f6681ec4b0fe Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 11 Oct 2020 17:07:14 +0000 Subject: [PATCH 1/5] Properly type the derivation and substitution goal maps As a bonus, Worker::removeGoal is less inefficient. --- src/libstore/build.hh | 11 +++++------ src/libstore/build/worker.cc | 33 +++++++++++++++++---------------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/libstore/build.hh b/src/libstore/build.hh index 8027c61b1..894feab5b 100644 --- a/src/libstore/build.hh +++ b/src/libstore/build.hh @@ -28,7 +28,6 @@ struct HookInstance; /* A pointer to a goal. */ struct Goal; -class DerivationGoal; typedef std::shared_ptr GoalPtr; typedef std::weak_ptr WeakGoalPtr; @@ -140,6 +139,8 @@ struct Child steady_time_point timeStarted; }; +class DerivationGoal; +class SubstitutionGoal; /* The worker class. */ class Worker @@ -167,8 +168,8 @@ private: /* Maps used to prevent multiple instantiations of a goal for the same derivation / path. */ - WeakGoalMap derivationGoals; - WeakGoalMap substitutionGoals; + std::map> derivationGoals; + std::map> substitutionGoals; /* Goals waiting for busy paths to be unlocked. */ WeakGoals waitingForAnyGoal; @@ -242,7 +243,7 @@ public: const StringSet & wantedOutputs, BuildMode buildMode = bmNormal); /* substitution goal */ - GoalPtr makeSubstitutionGoal(const StorePath & storePath, RepairFlag repair = NoRepair, std::optional ca = std::nullopt); + std::shared_ptr makeSubstitutionGoal(const StorePath & storePath, RepairFlag repair = NoRepair, std::optional ca = std::nullopt); /* Remove a dead goal. */ void removeGoal(GoalPtr goal); @@ -305,8 +306,6 @@ public: typedef enum {rpAccept, rpDecline, rpPostpone} HookReply; -class SubstitutionGoal; - /* Unless we are repairing, we don't both to test validity and just assume it, so the choices are `Absent` or `Valid`. */ enum struct PathStatus { diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 2fc9f6982..47403580e 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -39,16 +39,13 @@ std::shared_ptr Worker::makeDerivationGoalCommon( const StringSet & wantedOutputs, std::function()> mkDrvGoal) { - WeakGoalPtr & abstract_goal_weak = derivationGoals[drvPath]; - GoalPtr abstract_goal = abstract_goal_weak.lock(); // FIXME - std::shared_ptr goal; - if (!abstract_goal) { + std::weak_ptr & goal_weak = derivationGoals[drvPath]; + std::shared_ptr goal = goal_weak.lock(); + if (!goal) { goal = mkDrvGoal(); - abstract_goal_weak = goal; + goal_weak = goal; wakeUp(goal); } else { - goal = std::dynamic_pointer_cast(abstract_goal); - assert(goal); goal->addWantedOutputs(wantedOutputs); } return goal; @@ -73,10 +70,10 @@ std::shared_ptr Worker::makeBasicDerivationGoal(const StorePath } -GoalPtr Worker::makeSubstitutionGoal(const StorePath & path, RepairFlag repair, std::optional ca) +std::shared_ptr Worker::makeSubstitutionGoal(const StorePath & path, RepairFlag repair, std::optional ca) { - WeakGoalPtr & goal_weak = substitutionGoals[path]; - GoalPtr goal = goal_weak.lock(); // FIXME + std::weak_ptr & goal_weak = substitutionGoals[path]; + auto goal = goal_weak.lock(); // FIXME if (!goal) { goal = std::make_shared(path, *this, repair, ca); goal_weak = goal; @@ -85,14 +82,14 @@ GoalPtr Worker::makeSubstitutionGoal(const StorePath & path, RepairFlag repair, return goal; } - -static void removeGoal(GoalPtr goal, WeakGoalMap & goalMap) +template +static void removeGoal(std::shared_ptr goal, std::map> & goalMap) { /* !!! inefficient */ - for (WeakGoalMap::iterator i = goalMap.begin(); + for (typename std::map>::iterator i = goalMap.begin(); i != goalMap.end(); ) if (i->second.lock() == goal) { - WeakGoalMap::iterator j = i; ++j; + typename std::map>::iterator j = i; ++j; goalMap.erase(i); i = j; } @@ -102,8 +99,12 @@ static void removeGoal(GoalPtr goal, WeakGoalMap & goalMap) void Worker::removeGoal(GoalPtr goal) { - nix::removeGoal(goal, derivationGoals); - nix::removeGoal(goal, substitutionGoals); + if (auto drvGoal = std::dynamic_pointer_cast(goal)) + nix::removeGoal(drvGoal, derivationGoals); + else if (auto subGoal = std::dynamic_pointer_cast(goal)) + nix::removeGoal(subGoal, substitutionGoals); + else + assert(false); if (topGoals.find(goal) != topGoals.end()) { topGoals.erase(goal); /* If a top-level goal failed, then kill all other goals From 55592b253f3dddb121c1072ca584e95c37729b6d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 13 Oct 2020 18:04:24 +0000 Subject: [PATCH 2/5] Add some more docs --- src/libstore/build/worker.hh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index 07c0c0f16..f8bacc514 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -11,6 +11,11 @@ namespace nix { class DerivationGoal; class SubstitutionGoal; +/* Workaround for not being able to declare a something like + + class SubstitutionGoal : public Goal; + + even when Goal is a complete type; */ GoalPtr upcast_goal(std::shared_ptr subGoal); typedef std::chrono::time_point steady_time_point; From e6f8ae56d82813edbcf09fda58305de47369f964 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 17 Oct 2020 21:45:31 +0000 Subject: [PATCH 3/5] tab -> space --- src/libstore/build/worker.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 97ffb9a34..4d3df26f3 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -455,7 +455,7 @@ void Worker::markContentsGood(const StorePath & path) GoalPtr upcast_goal(std::shared_ptr subGoal) { - return subGoal; + return subGoal; } } From 57d0432b395cc4d70792d2df5794ff2e0dd02d3d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 17 Oct 2020 21:47:52 +0000 Subject: [PATCH 4/5] Just use `auto` in two places. --- src/libstore/build/worker.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 4d3df26f3..17c10cd71 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -90,10 +90,10 @@ template static void removeGoal(std::shared_ptr goal, std::map> & goalMap) { /* !!! inefficient */ - for (typename std::map>::iterator i = goalMap.begin(); + for (auto i = goalMap.begin(); i != goalMap.end(); ) if (i->second.lock() == goal) { - typename std::map>::iterator j = i; ++j; + auto j = i; ++j; goalMap.erase(i); i = j; } From 7ed46c15744461534478e2ed0aa25a2b2e536c6f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 17 Oct 2020 21:50:12 +0000 Subject: [PATCH 5/5] Explain that `upcast_goal` is still a static cast --- src/libstore/build/worker.hh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index f8bacc514..3a53a8def 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -15,7 +15,11 @@ class SubstitutionGoal; class SubstitutionGoal : public Goal; - even when Goal is a complete type; */ + even when Goal is a complete type. + + This is still a static cast. The purpose of exporting it is to define it in + a place where `SubstitutionGoal` is concrete, and use it in a place where it + is opaque. */ GoalPtr upcast_goal(std::shared_ptr subGoal); typedef std::chrono::time_point steady_time_point;