libstore: add explicit in-build-slot-ness to goals

we don't need to expose information about how busy a Worker is if the
worker can instead tell its work items whether they are in a slot. in
the future we might use this to not start items waiting for a slot if
no slots are currently available, but that requires more preparation.

Change-Id: Ibe01ac536da7e6d6f80520164117c43e772f9bd9

src/libstore/build/derivation-goal.cc

@@ -129,9 +129,9 @@ Goal::Finished DerivationGoal::timedOut(Error && ex)
 }
 
 
-Goal::WorkResult DerivationGoal::work()
+Goal::WorkResult DerivationGoal::work(bool inBuildSlot)
 {
-    return (this->*state)();
+    return (this->*state)(inBuildSlot);
 }
 
 void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs)
@@ -155,7 +155,7 @@ void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs)
 }
 
 
-Goal::WorkResult DerivationGoal::getDerivation()
+Goal::WorkResult DerivationGoal::getDerivation(bool inBuildSlot)
 {
     trace("init");
 
@@ -163,7 +163,7 @@ Goal::WorkResult DerivationGoal::getDerivation()
        exists.  If it doesn't, it may be created through a
        substitute. */
     if (buildMode == bmNormal && worker.evalStore.isValidPath(drvPath)) {
-        return loadDerivation();
+        return loadDerivation(inBuildSlot);
     }
 
 
@@ -172,7 +172,7 @@ Goal::WorkResult DerivationGoal::getDerivation()
 }
 
 
-Goal::WorkResult DerivationGoal::loadDerivation()
+Goal::WorkResult DerivationGoal::loadDerivation(bool inBuildSlot)
 {
     trace("loading derivation");
 
@@ -199,11 +199,11 @@ Goal::WorkResult DerivationGoal::loadDerivation()
     }
     assert(drv);
 
-    return haveDerivation();
+    return haveDerivation(inBuildSlot);
 }
 
 
-Goal::WorkResult DerivationGoal::haveDerivation()
+Goal::WorkResult DerivationGoal::haveDerivation(bool inBuildSlot)
 {
     trace("have derivation");
 
@@ -231,7 +231,7 @@ Goal::WorkResult DerivationGoal::haveDerivation()
             });
         }
 
-        return gaveUpOnSubstitution();
+        return gaveUpOnSubstitution(inBuildSlot);
     }
 
     for (auto & i : drv->outputsAndOptPaths(worker.store))
@@ -280,14 +280,14 @@ Goal::WorkResult DerivationGoal::haveDerivation()
         }
 
     if (result.goals.empty()) { /* to prevent hang (no wake-up event) */
-        return outputsSubstitutionTried();
+        return outputsSubstitutionTried(inBuildSlot);
     } else {
         state = &DerivationGoal::outputsSubstitutionTried;
         return result;
     }
 }
 
-Goal::WorkResult DerivationGoal::outputsSubstitutionTried()
+Goal::WorkResult DerivationGoal::outputsSubstitutionTried(bool inBuildSlot)
 {
     trace("all outputs substituted (maybe)");
 
@@ -330,7 +330,7 @@ Goal::WorkResult DerivationGoal::outputsSubstitutionTried()
 
     if (needRestart == NeedRestartForMoreOutputs::OutputsAddedDoNeed) {
         needRestart = NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed;
-        return haveDerivation();
+        return haveDerivation(inBuildSlot);
     }
 
     auto [allValid, validOutputs] = checkPathValidity();
@@ -346,13 +346,13 @@ Goal::WorkResult DerivationGoal::outputsSubstitutionTried()
             worker.store.printStorePath(drvPath));
 
     /* Nothing to wait for; tail call */
-    return gaveUpOnSubstitution();
+    return gaveUpOnSubstitution(inBuildSlot);
 }
 
 
 /* At least one of the output paths could not be
    produced using a substitute.  So we have to build instead. */
-Goal::WorkResult DerivationGoal::gaveUpOnSubstitution()
+Goal::WorkResult DerivationGoal::gaveUpOnSubstitution(bool inBuildSlot)
 {
     WaitForGoals result;
 
@@ -416,7 +416,7 @@ Goal::WorkResult DerivationGoal::gaveUpOnSubstitution()
     }
 
     if (result.goals.empty()) {/* to prevent hang (no wake-up event) */
-        return inputsRealised();
+        return inputsRealised(inBuildSlot);
     } else {
         state = &DerivationGoal::inputsRealised;
         return result;
@@ -487,7 +487,7 @@ Goal::WorkResult DerivationGoal::repairClosure()
 }
 
 
-Goal::WorkResult DerivationGoal::closureRepaired()
+Goal::WorkResult DerivationGoal::closureRepaired(bool inBuildSlot)
 {
     trace("closure repaired");
     if (nrFailed > 0)
@@ -497,7 +497,7 @@ Goal::WorkResult DerivationGoal::closureRepaired()
 }
 
 
-Goal::WorkResult DerivationGoal::inputsRealised()
+Goal::WorkResult DerivationGoal::inputsRealised(bool inBuildSlot)
 {
     trace("all inputs realised");
 
@@ -511,7 +511,7 @@ Goal::WorkResult DerivationGoal::inputsRealised()
 
     if (retrySubstitution == RetrySubstitution::YesNeed) {
         retrySubstitution = RetrySubstitution::AlreadyRetried;
-        return haveDerivation();
+        return haveDerivation(inBuildSlot);
     }
 
     /* Gather information necessary for computing the closure and/or
@@ -659,7 +659,7 @@ Goal::WorkResult DerivationGoal::started()
     return StillAlive{};
 }
 
-Goal::WorkResult DerivationGoal::tryToBuild()
+Goal::WorkResult DerivationGoal::tryToBuild(bool inBuildSlot)
 {
     trace("trying to build");
 
@@ -731,7 +731,7 @@ Goal::WorkResult DerivationGoal::tryToBuild()
         && settings.maxBuildJobs.get() != 0;
 
     if (!buildLocally) {
-        switch (tryBuildHook()) {
+        switch (tryBuildHook(inBuildSlot)) {
             case rpAccept:
                 /* Yes, it has started doing so.  Wait until we get
                    EOF from the hook. */
@@ -759,7 +759,7 @@ Goal::WorkResult DerivationGoal::tryToBuild()
     return ContinueImmediately{};
 }
 
-Goal::WorkResult DerivationGoal::tryLocalBuild() {
+Goal::WorkResult DerivationGoal::tryLocalBuild(bool inBuildSlot) {
     throw Error(
         "unable to build with a primary store that isn't a local store; "
         "either pass a different '--store' or enable remote builds."
@@ -917,7 +917,7 @@ void runPostBuildHook(
     proc.getStdout()->drainInto(sink);
 }
 
-Goal::WorkResult DerivationGoal::buildDone()
+Goal::WorkResult DerivationGoal::buildDone(bool inBuildSlot)
 {
     trace("build done");
 
@@ -1037,7 +1037,7 @@ Goal::WorkResult DerivationGoal::buildDone()
     }
 }
 
-Goal::WorkResult DerivationGoal::resolvedFinished()
+Goal::WorkResult DerivationGoal::resolvedFinished(bool inBuildSlot)
 {
     trace("resolved derivation finished");
 
@@ -1108,7 +1108,7 @@ Goal::WorkResult DerivationGoal::resolvedFinished()
     return done(status, std::move(builtOutputs));
 }
 
-HookReply DerivationGoal::tryBuildHook()
+HookReply DerivationGoal::tryBuildHook(bool inBuildSlot)
 {
     if (!worker.hook.available || !useDerivation) return rpDecline;
 
@@ -1120,7 +1120,7 @@ HookReply DerivationGoal::tryBuildHook()
         /* Send the request to the hook. */
         worker.hook.instance->sink
             << "try"
-            << (worker.getNrLocalBuilds() < settings.maxBuildJobs ? 1 : 0)
+            << (inBuildSlot ? 1 : 0)
             << drv->platform
             << worker.store.printStorePath(drvPath)
             << parsedDrv->getRequiredSystemFeatures();

src/libstore/build/derivation-goal.hh

@@ -191,7 +191,7 @@ struct DerivationGoal : public Goal
      */
     std::optional<DerivationType> derivationType;
 
-    typedef WorkResult (DerivationGoal::*GoalState)();
+    typedef WorkResult (DerivationGoal::*GoalState)(bool inBuildSlot);
     GoalState state;
 
     BuildMode buildMode;
@@ -224,7 +224,7 @@ struct DerivationGoal : public Goal
 
     std::string key() override;
 
-    WorkResult work() override;
+    WorkResult work(bool inBuildSlot) override;
 
     /**
      * Add wanted outputs to an already existing derivation goal.
@@ -234,23 +234,23 @@ struct DerivationGoal : public Goal
     /**
      * The states.
      */
-    WorkResult getDerivation();
+    WorkResult getDerivation(bool inBuildSlot);
-    WorkResult loadDerivation();
+    WorkResult loadDerivation(bool inBuildSlot);
-    WorkResult haveDerivation();
+    WorkResult haveDerivation(bool inBuildSlot);
-    WorkResult outputsSubstitutionTried();
+    WorkResult outputsSubstitutionTried(bool inBuildSlot);
-    WorkResult gaveUpOnSubstitution();
+    WorkResult gaveUpOnSubstitution(bool inBuildSlot);
-    WorkResult closureRepaired();
+    WorkResult closureRepaired(bool inBuildSlot);
-    WorkResult inputsRealised();
+    WorkResult inputsRealised(bool inBuildSlot);
-    WorkResult tryToBuild();
+    WorkResult tryToBuild(bool inBuildSlot);
-    virtual WorkResult tryLocalBuild();
+    virtual WorkResult tryLocalBuild(bool inBuildSlot);
-    WorkResult buildDone();
+    WorkResult buildDone(bool inBuildSlot);
 
-    WorkResult resolvedFinished();
+    WorkResult resolvedFinished(bool inBuildSlot);
 
     /**
      * Is the build hook willing to perform the build?
      */
-    HookReply tryBuildHook();
+    HookReply tryBuildHook(bool inBuildSlot);
 
     virtual int getChildStatus();
 

src/libstore/build/drv-output-substitution-goal.cc

@@ -20,7 +20,7 @@ DrvOutputSubstitutionGoal::DrvOutputSubstitutionGoal(
 }
 
 
-Goal::WorkResult DrvOutputSubstitutionGoal::init()
+Goal::WorkResult DrvOutputSubstitutionGoal::init(bool inBuildSlot)
 {
     trace("init");
 
@@ -30,17 +30,14 @@ Goal::WorkResult DrvOutputSubstitutionGoal::init()
     }
 
     subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>();
-    return tryNext();
+    return tryNext(inBuildSlot);
 }
 
-Goal::WorkResult DrvOutputSubstitutionGoal::tryNext()
+Goal::WorkResult DrvOutputSubstitutionGoal::tryNext(bool inBuildSlot)
 {
     trace("trying next substituter");
 
-    /* Make sure that we are allowed to start a substitution.  Note that even
+    if (!inBuildSlot) {
-       if maxSubstitutionJobs == 0, we still allow a substituter to run. This
-       prevents infinite waiting. */
-    if (worker.runningSubstitutions >= std::max(1U, settings.maxSubstitutionJobs.get())) {
         return WaitForSlot{};
     }
 
@@ -84,7 +81,7 @@ Goal::WorkResult DrvOutputSubstitutionGoal::tryNext()
     return StillAlive{};
 }
 
-Goal::WorkResult DrvOutputSubstitutionGoal::realisationFetched()
+Goal::WorkResult DrvOutputSubstitutionGoal::realisationFetched(bool inBuildSlot)
 {
     worker.childTerminated(this);
     maintainRunningSubstitutions.reset();
@@ -97,7 +94,7 @@ Goal::WorkResult DrvOutputSubstitutionGoal::realisationFetched()
     }
 
     if (!outputInfo) {
-        return tryNext();
+        return tryNext(inBuildSlot);
     }
 
     WaitForGoals result;
@@ -114,7 +111,7 @@ Goal::WorkResult DrvOutputSubstitutionGoal::realisationFetched()
                     worker.store.printStorePath(localOutputInfo->outPath),
                     worker.store.printStorePath(depPath)
                 );
-                return tryNext();
+                return tryNext(inBuildSlot);
             }
             result.goals.insert(worker.makeDrvOutputSubstitutionGoal(depId));
         }
@@ -123,14 +120,14 @@ Goal::WorkResult DrvOutputSubstitutionGoal::realisationFetched()
     result.goals.insert(worker.makePathSubstitutionGoal(outputInfo->outPath));
 
     if (result.goals.empty()) {
-        return outPathValid();
+        return outPathValid(inBuildSlot);
     } else {
         state = &DrvOutputSubstitutionGoal::outPathValid;
         return result;
     }
 }
 
-Goal::WorkResult DrvOutputSubstitutionGoal::outPathValid()
+Goal::WorkResult DrvOutputSubstitutionGoal::outPathValid(bool inBuildSlot)
 {
     assert(outputInfo);
     trace("output path substituted");
@@ -159,9 +156,9 @@ std::string DrvOutputSubstitutionGoal::key()
     return "a$" + std::string(id.to_string());
 }
 
-Goal::WorkResult DrvOutputSubstitutionGoal::work()
+Goal::WorkResult DrvOutputSubstitutionGoal::work(bool inBuildSlot)
 {
-    return (this->*state)();
+    return (this->*state)(inBuildSlot);
 }
 
 

src/libstore/build/drv-output-substitution-goal.hh

@@ -58,20 +58,20 @@ class DrvOutputSubstitutionGoal : public Goal {
 public:
     DrvOutputSubstitutionGoal(const DrvOutput& id, Worker & worker, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt);
 
-    typedef WorkResult (DrvOutputSubstitutionGoal::*GoalState)();
+    typedef WorkResult (DrvOutputSubstitutionGoal::*GoalState)(bool inBuildSlot);
     GoalState state;
 
-    WorkResult init();
+    WorkResult init(bool inBuildSlot);
-    WorkResult tryNext();
+    WorkResult tryNext(bool inBuildSlot);
-    WorkResult realisationFetched();
+    WorkResult realisationFetched(bool inBuildSlot);
-    WorkResult outPathValid();
+    WorkResult outPathValid(bool inBuildSlot);
     WorkResult finished();
 
     Finished timedOut(Error && ex) override { abort(); };
 
     std::string key() override;
 
-    WorkResult work() override;
+    WorkResult work(bool inBuildSlot) override;
 
     JobCategory jobCategory() const override {
         return JobCategory::Substitution;

src/libstore/build/goal.hh

@@ -147,7 +147,7 @@ public:
         trace("goal destroyed");
     }
 
-    virtual WorkResult work() = 0;
+    virtual WorkResult work(bool inBuildSlot) = 0;
 
     virtual void waiteeDone(GoalPtr waitee) { }
 

src/libstore/build/local-derivation-goal.cc

@@ -150,14 +150,13 @@ void LocalDerivationGoal::killSandbox(bool getStats)
 }
 
 
-Goal::WorkResult LocalDerivationGoal::tryLocalBuild()
+Goal::WorkResult LocalDerivationGoal::tryLocalBuild(bool inBuildSlot)
 {
 #if __APPLE__
     additionalSandboxProfile = parsedDrv->getStringAttr("__sandboxProfile").value_or("");
 #endif
 
-    unsigned int curBuilds = worker.getNrLocalBuilds();
+    if (!inBuildSlot) {
-    if (curBuilds >= settings.maxBuildJobs) {
         state = &DerivationGoal::tryToBuild;
         outputLocks.unlock();
         return WaitForSlot{};

src/libstore/build/local-derivation-goal.hh

@@ -211,7 +211,7 @@ struct LocalDerivationGoal : public DerivationGoal
     /**
      * The additional states.
      */
-    WorkResult tryLocalBuild() override;
+    WorkResult tryLocalBuild(bool inBuildSlot) override;
 
     /**
      * Start building a derivation.

src/libstore/build/substitution-goal.cc

@@ -39,13 +39,13 @@ Goal::Finished PathSubstitutionGoal::done(
 }
 
 
-Goal::WorkResult PathSubstitutionGoal::work()
+Goal::WorkResult PathSubstitutionGoal::work(bool inBuildSlot)
 {
-    return (this->*state)();
+    return (this->*state)(inBuildSlot);
 }
 
 
-Goal::WorkResult PathSubstitutionGoal::init()
+Goal::WorkResult PathSubstitutionGoal::init(bool inBuildSlot)
 {
     trace("init");
 
@@ -61,11 +61,11 @@ Goal::WorkResult PathSubstitutionGoal::init()
 
     subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>();
 
-    return tryNext();
+    return tryNext(inBuildSlot);
 }
 
 
-Goal::WorkResult PathSubstitutionGoal::tryNext()
+Goal::WorkResult PathSubstitutionGoal::tryNext(bool inBuildSlot)
 {
     trace("trying next substituter");
 
@@ -97,23 +97,23 @@ Goal::WorkResult PathSubstitutionGoal::tryNext()
         if (sub->storeDir == worker.store.storeDir)
             assert(subPath == storePath);
     } else if (sub->storeDir != worker.store.storeDir) {
-        return tryNext();
+        return tryNext(inBuildSlot);
     }
 
     try {
         // FIXME: make async
         info = sub->queryPathInfo(subPath ? *subPath : storePath);
     } catch (InvalidPath &) {
-        return tryNext();
+        return tryNext(inBuildSlot);
     } catch (SubstituterDisabled &) {
         if (settings.tryFallback) {
-            return tryNext();
+            return tryNext(inBuildSlot);
         }
         throw;
     } catch (Error & e) {
         if (settings.tryFallback) {
             logError(e.info());
-            return tryNext();
+            return tryNext(inBuildSlot);
         }
         throw;
     }
@@ -126,7 +126,7 @@ Goal::WorkResult PathSubstitutionGoal::tryNext()
         } else {
             printError("asked '%s' for '%s' but got '%s'",
                 sub->getUri(), worker.store.printStorePath(storePath), sub->printStorePath(info->path));
-            return tryNext();
+            return tryNext(inBuildSlot);
         }
     }
 
@@ -147,7 +147,7 @@ Goal::WorkResult PathSubstitutionGoal::tryNext()
     {
         warn("ignoring substitute for '%s' from '%s', as it's not signed by any of the keys in 'trusted-public-keys'",
             worker.store.printStorePath(storePath), sub->getUri());
-        return tryNext();
+        return tryNext(inBuildSlot);
     }
 
     /* To maintain the closure invariant, we first have to realise the
@@ -158,7 +158,7 @@ Goal::WorkResult PathSubstitutionGoal::tryNext()
             result.goals.insert(worker.makePathSubstitutionGoal(i));
 
     if (result.goals.empty()) {/* to prevent hang (no wake-up event) */
-        return referencesValid();
+        return referencesValid(inBuildSlot);
     } else {
         state = &PathSubstitutionGoal::referencesValid;
         return result;
@@ -166,7 +166,7 @@ Goal::WorkResult PathSubstitutionGoal::tryNext()
 }
 
 
-Goal::WorkResult PathSubstitutionGoal::referencesValid()
+Goal::WorkResult PathSubstitutionGoal::referencesValid(bool inBuildSlot)
 {
     trace("all references realised");
 
@@ -186,14 +186,11 @@ Goal::WorkResult PathSubstitutionGoal::referencesValid()
 }
 
 
-Goal::WorkResult PathSubstitutionGoal::tryToRun()
+Goal::WorkResult PathSubstitutionGoal::tryToRun(bool inBuildSlot)
 {
     trace("trying to run");
 
-    /* Make sure that we are allowed to start a substitution.  Note that even
+    if (!inBuildSlot) {
-       if maxSubstitutionJobs == 0, we still allow a substituter to run. This
-       prevents infinite waiting. */
-    if (worker.getNrSubstitutions() >= std::max(1U, (unsigned int) settings.maxSubstitutionJobs)) {
         return WaitForSlot{};
     }
 
@@ -231,7 +228,7 @@ Goal::WorkResult PathSubstitutionGoal::tryToRun()
 }
 
 
-Goal::WorkResult PathSubstitutionGoal::finished()
+Goal::WorkResult PathSubstitutionGoal::finished(bool inBuildSlot)
 {
     trace("substitute finished");
 

src/libstore/build/substitution-goal.hh

@@ -66,7 +66,7 @@ struct PathSubstitutionGoal : public Goal
     std::unique_ptr<MaintainCount<uint64_t>> maintainExpectedSubstitutions,
         maintainRunningSubstitutions, maintainExpectedNar, maintainExpectedDownload;
 
-    typedef WorkResult (PathSubstitutionGoal::*GoalState)();
+    typedef WorkResult (PathSubstitutionGoal::*GoalState)(bool inBuildSlot);
     GoalState state;
 
     /**
@@ -94,16 +94,16 @@ public:
         return "a$" + std::string(storePath.name()) + "$" + worker.store.printStorePath(storePath);
     }
 
-    WorkResult work() override;
+    WorkResult work(bool inBuildSlot) override;
 
     /**
      * The states.
      */
-    WorkResult init();
+    WorkResult init(bool inBuildSlot);
-    WorkResult tryNext();
+    WorkResult tryNext(bool inBuildSlot);
-    WorkResult referencesValid();
+    WorkResult referencesValid(bool inBuildSlot);
-    WorkResult tryToRun();
+    WorkResult tryToRun(bool inBuildSlot);
-    WorkResult finished();
+    WorkResult finished(bool inBuildSlot);
 
     /**
      * Callback used by the worker to write to the log.

src/libstore/build/worker.cc

@@ -232,18 +232,6 @@ void Worker::wakeUp(GoalPtr goal)
 }
 
 
-unsigned Worker::getNrLocalBuilds()
-{
-    return nrLocalBuilds;
-}
-
-
-unsigned Worker::getNrSubstitutions()
-{
-    return nrSubstitutions;
-}
-
-
 void Worker::childStarted(GoalPtr goal, const std::set<int> & fds,
     bool inBuildSlot, bool respectTimeouts)
 {
@@ -307,8 +295,8 @@ void Worker::waitForBuildSlot(GoalPtr goal)
 {
     goal->trace("wait for build slot");
     bool isSubstitutionGoal = goal->jobCategory() == JobCategory::Substitution;
-    if ((!isSubstitutionGoal && getNrLocalBuilds() < settings.maxBuildJobs) ||
+    if ((!isSubstitutionGoal && nrLocalBuilds < settings.maxBuildJobs) ||
-        (isSubstitutionGoal && getNrSubstitutions() < settings.maxSubstitutionJobs))
+        (isSubstitutionGoal && nrSubstitutions < settings.maxSubstitutionJobs))
         wakeUp(goal); /* we can do it right away */
     else
         wantingToBuild.insert(goal);
@@ -364,7 +352,12 @@ void Worker::run(const Goals & _topGoals)
             awake.clear();
             for (auto & goal : awake2) {
                 checkInterrupt();
-                handleWorkResult(goal, goal->work());
+                /* Make sure that we are always allowed to run at least one substitution.
+                   This prevents infinite waiting. */
+                const bool inSlot = goal->jobCategory() == JobCategory::Substitution
+                    ? nrSubstitutions < std::max(1U, (unsigned int) settings.maxSubstitutionJobs)
+                    : nrLocalBuilds < settings.maxBuildJobs;
+                handleWorkResult(goal, goal->work(inSlot));
 
                 actDerivations.progress(
                     doneBuilds, expectedBuilds + doneBuilds, runningBuilds, failedBuilds

src/libstore/build/worker.hh

@@ -229,17 +229,6 @@ public:
      */
     GoalPtr makeGoal(const DerivedPath & req, BuildMode buildMode = bmNormal);
 
-    /**
-     * Return the number of local build processes currently running (but not
-     * remote builds via the build hook).
-     */
-    unsigned int getNrLocalBuilds();
-
-    /**
-     * Return the number of substitution processes currently running.
-     */
-    unsigned int getNrSubstitutions();
-
     /**
      * Registers a running child process.  `inBuildSlot` means that
      * the process counts towards the jobs limit.