Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make number of parallel substituter queries configurable #5324

Closed
wants to merge 1 commit into from

Conversation

Kha
Copy link
Contributor

@Kha Kha commented Oct 3, 2021

This is the minimal fix for #5118. The default is taken from http-connections, though as stated, multiplexing can mean that a far higher number is even more effective. Still, the new default should be a sizeable improvement over the old one of nproc for most machines while still low enough that time and memory overhead from the additional threads should be insignificant.

@@ -250,6 +250,17 @@ public:
)",
{"build-use-substitutes"}};

Setting<unsigned int> maxQuerySubstitutersJobs{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to just check if the substituter supports http/2 and then significantly increase this setting?

Copy link
Contributor Author

@Kha Kha Oct 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The better solution is not to use threads at all here, which would obviate the need for any parallelism guessing by making that the sole responsibility of libcurl. This here is the trivial fix hopefully in time for Nix 2.4 that at least gives people an option to max out the connections. Any more complex solution should come later.

@domenkozar
Copy link
Member

@edolstra thoughts?

@Kha could you rebase on top of master to fix CI?

@Kha Kha force-pushed the max-query-substituters-jobs branch from 1ef3b45 to b3104c2 Compare October 6, 2021 21:24
@domenkozar
Copy link
Member

@edolstra I'd really love to get this into 2.4 as slow querying of binary caches makes it a really bad experience for bigger projects.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/ignore-offline-substituters/15450/2

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/ignore-offline-substituters/15450/3

@domenkozar
Copy link
Member

@edolstra Any chance you can take a look?

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nix-2-4-release-candidate/15393/2

@edolstra edolstra added this to the nix-2.5 milestone Nov 1, 2021
@domenkozar
Copy link
Member

@edolstra any reason to hold this?

@edolstra
Copy link
Member

It's probably better to rewrite Store::queryMissing() to be single-threaded, similar to what we did with Store::computeFSClosure() in 75989bd. This will probably require some more async versions of APIs like querySubstitutablePathInfos().

@domenkozar
Copy link
Member

@edolstra that would be great. Merging this will fix query performance for low-core counts until we get the async version done.

@edolstra edolstra closed this in 95bd5da Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants