From 613bc699bb72d32c7d0622bf2996b2ecc8012455 Mon Sep 17 00:00:00 2001 From: Matej Urbas Date: Sun, 30 Apr 2023 09:56:24 +0100 Subject: [PATCH 1/3] `max-substitution-jobs` setting --- src/libstore/build/substitution-goal.cc | 2 +- src/libstore/build/worker.cc | 25 +++++++++++++++++++---- src/libstore/build/worker.hh | 19 ++++++++++++----- src/libstore/globals.cc | 11 ++++++++++ src/libstore/globals.hh | 27 +++++++++++++++++++++++++ 5 files changed, 74 insertions(+), 10 deletions(-) diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 190fb455a..30c196894 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -204,7 +204,7 @@ void PathSubstitutionGoal::tryToRun() if maxBuildJobs == 0 (no local builds allowed), we still allow a substituter to run. This is because substitutions cannot be distributed to another machine via the build hook. */ - if (worker.getNrLocalBuilds() >= std::max(1U, (unsigned int) settings.maxBuildJobs)) { + if (worker.getNrSubstitutions() >= std::max(1U, (unsigned int) settings.maxSubstitutionJobs)) { worker.waitForBuildSlot(shared_from_this()); return; } diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 6ad4a0e2b..85024e4c4 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -18,6 +18,7 @@ Worker::Worker(Store & store, Store & evalStore) { /* Debugging: prevent recursive workers. */ nrLocalBuilds = 0; + nrSubstitutions = 0; lastWokenUp = steady_time_point::min(); permanentFailure = false; timedOut = false; @@ -176,6 +177,12 @@ unsigned Worker::getNrLocalBuilds() } +unsigned Worker::getNrSubstitutions() +{ + return nrSubstitutions; +} + + void Worker::childStarted(GoalPtr goal, const std::set & fds, bool inBuildSlot, bool respectTimeouts) { @@ -187,7 +194,10 @@ void Worker::childStarted(GoalPtr goal, const std::set & fds, child.inBuildSlot = inBuildSlot; child.respectTimeouts = respectTimeouts; children.emplace_back(child); - if (inBuildSlot) nrLocalBuilds++; + if (inBuildSlot) { + if (dynamic_cast(child.goal2)) nrSubstitutions++; + else nrLocalBuilds++; + } } @@ -198,8 +208,13 @@ void Worker::childTerminated(Goal * goal, bool wakeSleepers) if (i == children.end()) return; if (i->inBuildSlot) { - assert(nrLocalBuilds > 0); - nrLocalBuilds--; + if (dynamic_cast(goal)) { + assert(nrSubstitutions > 0); + nrSubstitutions--; + } else { + assert(nrLocalBuilds > 0); + nrLocalBuilds--; + } } children.erase(i); @@ -220,7 +235,9 @@ void Worker::childTerminated(Goal * goal, bool wakeSleepers) void Worker::waitForBuildSlot(GoalPtr goal) { debug("wait for build slot"); - if (getNrLocalBuilds() < settings.maxBuildJobs) + bool isSubstitutionGoal = dynamic_cast(goal.get()); + if ((!isSubstitutionGoal && getNrLocalBuilds() < settings.maxBuildJobs) || + (isSubstitutionGoal && getNrSubstitutions() < settings.maxSubstitutionJobs)) wakeUp(goal); /* we can do it right away */ else addToWeakGoals(wantingToBuild, goal); diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index bb51d641d..63624d910 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -88,11 +88,16 @@ private: std::list children; /** - * Number of build slots occupied. This includes local builds and - * substitutions but not remote builds via the build hook. + * Number of build slots occupied. This includes local builds but does not + * include substitutions or remote builds via the build hook. */ unsigned int nrLocalBuilds; + /** + * Number of substitution slots occupied. + */ + unsigned int nrSubstitutions; + /** * Maps used to prevent multiple instantiations of a goal for the * same derivation / path. @@ -220,12 +225,16 @@ public: void wakeUp(GoalPtr goal); /** - * Return the number of local build and substitution processes - * currently running (but not remote builds via the build - * hook). + * 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. diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 4c66d08ee..46b14cc98 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -249,6 +249,17 @@ unsigned int MaxBuildJobsSetting::parse(const std::string & str) const } } +unsigned int MaxSubstitutionJobsSetting::parse(const std::string & str) const +{ + if (str == "auto") return std::max(1U, std::thread::hardware_concurrency()); + else { + if (auto n = string2Int(str)) + return std::max(1U, *n); + else + throw UsageError("configuration setting '%s' should be 'auto' or an integer", name); + } +} + Paths PluginFilesSetting::parse(const std::string & str) const { diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 609cf53b8..f1e1e4b22 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -29,6 +29,21 @@ struct MaxBuildJobsSetting : public BaseSetting unsigned int parse(const std::string & str) const override; }; +struct MaxSubstitutionJobsSetting : public BaseSetting +{ + MaxSubstitutionJobsSetting(Config * options, + unsigned int def, + const std::string & name, + const std::string & description, + const std::set & aliases = {}) + : BaseSetting(def, true, name, description, aliases) + { + options->addSetting(this); + } + + unsigned int parse(const std::string & str) const override; +}; + struct PluginFilesSetting : public BaseSetting { bool pluginsLoaded = false; @@ -159,6 +174,18 @@ public: )", {"build-max-jobs"}}; + MaxSubstitutionJobsSetting maxSubstitutionJobs{ + this, 16, "max-substitution-jobs", + R"( + This option defines the maximum number of substitution jobs that Nix + will try to run in parallel. The default is `16`. The minimum value + one can choose is `1` and lower values will be interpreted as `1`. The + special value `auto` causes Nix to use the number of CPUs in your + system. It can be overridden using the `--max-substitution-jobs` + command line switch. + )", + {"substitution-max-jobs"}}; + Setting buildCores{ this, getDefaultCores(), From 1ea1e378de22bf32b5ccc1edf66798ab91299ac7 Mon Sep 17 00:00:00 2001 From: Matej Urbas Date: Mon, 8 May 2023 19:21:57 +0100 Subject: [PATCH 2/3] removes MaxSubstitutionJobsSetting --- src/libstore/globals.cc | 11 ----------- src/libstore/globals.hh | 22 ++-------------------- 2 files changed, 2 insertions(+), 31 deletions(-) diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc index 46b14cc98..4c66d08ee 100644 --- a/src/libstore/globals.cc +++ b/src/libstore/globals.cc @@ -249,17 +249,6 @@ unsigned int MaxBuildJobsSetting::parse(const std::string & str) const } } -unsigned int MaxSubstitutionJobsSetting::parse(const std::string & str) const -{ - if (str == "auto") return std::max(1U, std::thread::hardware_concurrency()); - else { - if (auto n = string2Int(str)) - return std::max(1U, *n); - else - throw UsageError("configuration setting '%s' should be 'auto' or an integer", name); - } -} - Paths PluginFilesSetting::parse(const std::string & str) const { diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index f1e1e4b22..c41125523 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -29,21 +29,6 @@ struct MaxBuildJobsSetting : public BaseSetting unsigned int parse(const std::string & str) const override; }; -struct MaxSubstitutionJobsSetting : public BaseSetting -{ - MaxSubstitutionJobsSetting(Config * options, - unsigned int def, - const std::string & name, - const std::string & description, - const std::set & aliases = {}) - : BaseSetting(def, true, name, description, aliases) - { - options->addSetting(this); - } - - unsigned int parse(const std::string & str) const override; -}; - struct PluginFilesSetting : public BaseSetting { bool pluginsLoaded = false; @@ -174,15 +159,12 @@ public: )", {"build-max-jobs"}}; - MaxSubstitutionJobsSetting maxSubstitutionJobs{ + Setting maxSubstitutionJobs{ this, 16, "max-substitution-jobs", R"( This option defines the maximum number of substitution jobs that Nix will try to run in parallel. The default is `16`. The minimum value - one can choose is `1` and lower values will be interpreted as `1`. The - special value `auto` causes Nix to use the number of CPUs in your - system. It can be overridden using the `--max-substitution-jobs` - command line switch. + one can choose is `1` and lower values will be interpreted as `1`. )", {"substitution-max-jobs"}}; From 13185133bcdff751274e55bf29b80b4e600cd973 Mon Sep 17 00:00:00 2001 From: Matej Urbas Date: Mon, 8 May 2023 19:45:46 +0100 Subject: [PATCH 3/3] introduces `Goal::jobCategory` --- src/libstore/build/derivation-goal.hh | 2 ++ src/libstore/build/drv-output-substitution-goal.hh | 4 +++- src/libstore/build/goal.hh | 13 +++++++++++++ src/libstore/build/substitution-goal.hh | 2 ++ src/libstore/build/worker.cc | 6 +++--- 5 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 7033b7a58..223be06f2 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -335,6 +335,8 @@ struct DerivationGoal : public Goal void waiteeDone(GoalPtr waitee, ExitCode result) override; StorePathSet exportReferences(const StorePathSet & storePaths); + + JobCategory jobCategory() override { return JobCategory::Build; }; }; MakeError(NotDeterministic, BuildError); diff --git a/src/libstore/build/drv-output-substitution-goal.hh b/src/libstore/build/drv-output-substitution-goal.hh index 697ddb283..5d1253a71 100644 --- a/src/libstore/build/drv-output-substitution-goal.hh +++ b/src/libstore/build/drv-output-substitution-goal.hh @@ -21,7 +21,7 @@ class Worker; class DrvOutputSubstitutionGoal : public Goal { /** - * The drv output we're trying to substitue + * The drv output we're trying to substitute */ DrvOutput id; @@ -72,6 +72,8 @@ public: void work() override; void handleEOF(int fd) override; + + JobCategory jobCategory() override { return JobCategory::Substitution; }; }; } diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh index c0e12a2ed..a313bf22c 100644 --- a/src/libstore/build/goal.hh +++ b/src/libstore/build/goal.hh @@ -34,6 +34,17 @@ typedef std::set> WeakGoals; */ typedef std::map WeakGoalMap; +/** + * Used as a hint to the worker on how to schedule a particular goal. For example, + * builds are typically CPU- and memory-bound, while substitutions are I/O bound. + * Using this information, the worker might decide to schedule more or fewer goals + * of each category in parallel. + */ +enum struct JobCategory { + Build, + Substitution, +}; + struct Goal : public std::enable_shared_from_this { typedef enum {ecBusy, ecSuccess, ecFailed, ecNoSubstituters, ecIncompleteClosure} ExitCode; @@ -150,6 +161,8 @@ public: void amDone(ExitCode result, std::optional ex = {}); virtual void cleanup() { } + + virtual JobCategory jobCategory() = 0; }; void addToWeakGoals(WeakGoals & goals, GoalPtr p); diff --git a/src/libstore/build/substitution-goal.hh b/src/libstore/build/substitution-goal.hh index c2b7fc95a..9fc041920 100644 --- a/src/libstore/build/substitution-goal.hh +++ b/src/libstore/build/substitution-goal.hh @@ -115,6 +115,8 @@ public: void handleEOF(int fd) override; void cleanup() override; + + JobCategory jobCategory() override { return JobCategory::Substitution; }; }; } diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 85024e4c4..ee334d54a 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -195,7 +195,7 @@ void Worker::childStarted(GoalPtr goal, const std::set & fds, child.respectTimeouts = respectTimeouts; children.emplace_back(child); if (inBuildSlot) { - if (dynamic_cast(child.goal2)) nrSubstitutions++; + if (goal->jobCategory() == JobCategory::Substitution) nrSubstitutions++; else nrLocalBuilds++; } } @@ -208,7 +208,7 @@ void Worker::childTerminated(Goal * goal, bool wakeSleepers) if (i == children.end()) return; if (i->inBuildSlot) { - if (dynamic_cast(goal)) { + if (goal->jobCategory() == JobCategory::Substitution) { assert(nrSubstitutions > 0); nrSubstitutions--; } else { @@ -235,7 +235,7 @@ void Worker::childTerminated(Goal * goal, bool wakeSleepers) void Worker::waitForBuildSlot(GoalPtr goal) { debug("wait for build slot"); - bool isSubstitutionGoal = dynamic_cast(goal.get()); + bool isSubstitutionGoal = goal->jobCategory() == JobCategory::Substitution; if ((!isSubstitutionGoal && getNrLocalBuilds() < settings.maxBuildJobs) || (isSubstitutionGoal && getNrSubstitutions() < settings.maxSubstitutionJobs)) wakeUp(goal); /* we can do it right away */