libstore: change `max-jobs` default to `auto` #9039

Enzime · 2023-09-25T13:24:30Z

Motivation

Non-NixOS users who install Nix will have a poor experience as the default is 1 especially when building derivations with many dependencies.

Priorities

Add 👍 to pull requests you find important.

src/libstore/globals.cc

roberth

Good to stay in sync with NixOS, which has provided a better out of the box experience with respect to the max-jobs option.

src/libutil/util.hh

edolstra · 2023-09-26T13:32:42Z

src/libstore/globals.hh

@@ -147,15 +147,14 @@ public:
        "the log to show if a build fails."};

    MaxBuildJobsSetting maxBuildJobs{
-        this, 1, "max-jobs",
+        this, getMaxThreads(), "max-jobs",


A default of getMaxThreads() makes documentation generation non-reproducible. It may be better to represent the "auto" value explicitly, e.g. by changing the type of this option to std::optional<unsigned int>, with "auto" encoded as std::nullopt.

src/libutil/util.hh

src/libstore/globals.hh

roberth · 2023-10-03T17:20:25Z

src/libstore/globals.cc

 {
-    if (str == "auto") return std::max(1U, std::thread::hardware_concurrency());
+    return value.value_or(getMaxThreads());


Suggested change

return value.value_or(getMaxThreads());

// Settings should be fast to retrieve. Ignore changes to core count - rare, and process lifespan is often short.

unsigned int cachedThreads = getMaxThreads();

return value.value_or(cachedThreads);

fricklerhandwerk · 2023-10-20T17:10:43Z

Triaged in Nix team meeting 2023-10-13:

This is good to go in principle, assigned to @roberth
Would be nice to generalize the auto mechanism as a follow-up
@roberth: Subclassing Setting<T> is hard when the parsing implementation for T does not exist in a related type class about parsing, or if you want a custom parser for the type.

nixos-discourse · 2023-10-20T17:30:36Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-10-13-nix-team-meeting-minutes-94/34395/1

Enzime · 2023-11-04T16:07:58Z

Instead of improving docs reproducibility by changing the type (which I tried but ran into some gnarly template related issues), I decided to extend Settings to have a defaultText parameter which allows you to add a hardcoded string as the default value in the docs.

This latter part would've most likely been necessary either way as we would've wanted some way to render std::nullopt as auto in the docs.

src/libstore/globals.cc

tfc · 2023-11-04T16:55:11Z

src/libstore/globals.hh

-        const std::set<std::string> & aliases = {})
-        : BaseSetting<unsigned int>(def, true, name, description, aliases)
+        const std::set<std::string> & aliases = {},
+        const std::optional<std::string> defaultText = std::nullopt)


all these constructor parameters should ideally be by value and then std::moved further. Then the way the caller calls it (with lvalues or rvalues) unnecessary copies can be avoided.

tfc · 2023-11-04T16:55:18Z

src/libstore/globals.hh

-        : BaseSetting<Paths>(def, true, name, description, aliases)
+        const std::set<std::string> & aliases = {},
+        const std::optional<std::string> defaultText = std::nullopt)
+        : BaseSetting<Paths>(def, name, description, aliases, defaultText)


tfc · 2023-11-04T16:57:10Z

src/libutil/config.cc

-    : BaseSetting<Path>(def, true, name, description, aliases)
+    const std::set<std::string> & aliases,
+    const std::optional<std::string> defaultText)
+    : BaseSetting<Path>(def, name, description, aliases, defaultText)


ok there are many possibilities to do call-by-value optimizations in constructors here. maybe just leave it as is and i open a pr on that on another occasion.

Oh, maybe... wait, is this meant to be const references to string literals in the code where all the settings are defined, and none of them are dynamic? @Ericson2314

tfc · 2023-11-04T16:59:25Z

src/libutil/config.hh

        std::optional<ExperimentalFeature> experimentalFeature = std::nullopt)
        : AbstractSetting(name, description, aliases, experimentalFeature)
        , value(def)
        , defaultValue(def)
-        , documentDefault(documentDefault)
+        , defaultText(defaultText)


plz use std::move here as this is by value

tfc · 2023-11-04T16:59:34Z

src/libutil/config.hh

        std::optional<ExperimentalFeature> experimentalFeature = std::nullopt)
-        : BaseSetting<T>(def, documentDefault, name, description, aliases, experimentalFeature)
+        : BaseSetting<T>(def, name, description, aliases, defaultText, experimentalFeature)


DavHau · 2023-12-27T12:54:37Z

What's the status of this? I'd really like to see this change deployed to the masses. Thanks for the work so far!

roberth · 2024-03-10T08:12:24Z

Oh no, I dropped the ball on this.
The solution lgtm, but there's a bunch of conflicts. @Enzime would you be willing to merge/rebase?

Enzime · 2024-03-21T08:00:28Z

@roberth I'm quite busy lately so I don't have the time to rebase this at the moment, if anyone's interested in picking up this PR, feel free to, otherwise I'll come back to it eventually when I get more time

Enzime requested review from thufschmitt and fricklerhandwerk as code owners September 25, 2023 13:24

roberth reviewed Sep 25, 2023

View reviewed changes

src/libstore/globals.cc Outdated Show resolved Hide resolved

Enzime force-pushed the update/max-jobs branch from 024f4b6 to f25f19a Compare September 25, 2023 23:33

Enzime requested a review from edolstra as a code owner September 25, 2023 23:33

roberth approved these changes Sep 26, 2023

View reviewed changes

src/libutil/util.hh Outdated Show resolved Hide resolved

Enzime force-pushed the update/max-jobs branch from f25f19a to c740ade Compare September 26, 2023 09:21

edolstra requested changes Sep 26, 2023

View reviewed changes

fricklerhandwerk reviewed Sep 26, 2023

View reviewed changes

src/libutil/util.hh Outdated Show resolved Hide resolved

src/libstore/globals.hh Outdated Show resolved Hide resolved

Enzime force-pushed the update/max-jobs branch from c740ade to 3fb783a Compare September 28, 2023 01:38

github-actions bot added the store Issues and pull requests concerning the Nix store label Sep 28, 2023

Enzime force-pushed the update/max-jobs branch from 3fb783a to a405bf5 Compare September 28, 2023 01:43

roberth reviewed Oct 3, 2023

View reviewed changes

thufschmitt assigned roberth Oct 13, 2023

Enzime force-pushed the update/max-jobs branch from a405bf5 to 43db31d Compare November 4, 2023 14:21

github-actions bot added documentation with-tests Issues related to testing. PRs with tests have some priority and removed store Issues and pull requests concerning the Nix store labels Nov 4, 2023

Enzime force-pushed the update/max-jobs branch from 43db31d to fb4388d Compare November 4, 2023 14:23

Allow specifying defaultText for settings

c13f005

Enzime force-pushed the update/max-jobs branch from fb4388d to 13742f9 Compare November 4, 2023 15:52

github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Nov 4, 2023

tfc reviewed Nov 4, 2023

View reviewed changes

libstore: change max-jobs default to auto

6ca89c7

Enzime force-pushed the update/max-jobs branch from 13742f9 to 6ca89c7 Compare November 5, 2023 07:56

fricklerhandwerk requested a review from roberth March 9, 2024 18:58

roberth added the settings Settings, global flags, nix.conf label Mar 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libstore: change `max-jobs` default to `auto` #9039

libstore: change `max-jobs` default to `auto` #9039

Enzime commented Sep 25, 2023

roberth left a comment

edolstra Sep 26, 2023

roberth Oct 3, 2023

fricklerhandwerk commented Oct 20, 2023 •

edited

Loading

nixos-discourse commented Oct 20, 2023

Enzime commented Nov 4, 2023

tfc Nov 4, 2023

tfc Nov 4, 2023

tfc Nov 4, 2023

tfc Nov 5, 2023

tfc Nov 4, 2023

tfc Nov 4, 2023

DavHau commented Dec 27, 2023

roberth commented Mar 10, 2024

Enzime commented Mar 21, 2024

-    return value.value_or(getMaxThreads());
+    // Settings should be fast to retrieve. Ignore changes to core count - rare, and process lifespan is often short.
+    unsigned int cachedThreads = getMaxThreads();
+    return value.value_or(cachedThreads);

libstore: change max-jobs default to auto #9039

Are you sure you want to change the base?

libstore: change max-jobs default to auto #9039

Conversation

Enzime commented Sep 25, 2023

Motivation

Priorities

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fricklerhandwerk commented Oct 20, 2023 • edited Loading

nixos-discourse commented Oct 20, 2023

Enzime commented Nov 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavHau commented Dec 27, 2023

roberth commented Mar 10, 2024

Enzime commented Mar 21, 2024

libstore: change `max-jobs` default to `auto` #9039

libstore: change `max-jobs` default to `auto` #9039

fricklerhandwerk commented Oct 20, 2023 •

edited

Loading