Disable threading when on single core hardware

[faern]

An improvement that will (at least temporarily) fix #2365.

No need to run a thread pool if the number of available CPUs/cores is one anyway. So it can fall back to the special single threaded mode in that case. The single threaded mode uses less memory so it's really useful on low end machines, such as the Raspberry Pi zero and similar.

Since setting the RUSTUP_IO_THREADS environment variable to zero on one now produces the same result as "disabled" did previously the "disabled" value does not need to exist any longer, and the code can be simpler if it just needs to be an integer directly. Since the variable was regarded as unstable in the docs I thought it was fine to change how it behaved.

This new code also prints a warning on stderr if RUSTUP_IO_THREADS is set to an invalid value, instead of just silently falling back to automatically detecting the number of cores.

I'm still working on trying to get this tested locally. But the Rpi zero is incredibly slow at compiling such a big project 😅 At least it builds. So I figured I could submit the PR and get some feedback.

[faern]

Seems to work as intended on a machine with many cores. So testing successful. But I have not tried on the Raspberry pi yet to see if it actually solves #2365 without having to set custom variables.

[rbtcollins]

I suggest cross-compiling for the pi, it will be a lot faster, even with the time to get setup :).

[faern]

Can you show me a guide for getting that set up on a macOS machine and I'd be super happy.

[rbtcollins]

Can you show me a guide for getting that set up on a macOS machine and I'd be super happy.

https://dev.to/jeikabu/raspberry-pi-zero-raspbian-rust-primer-3aj6

[rbtcollins]

@kinnison seeking your thoughts: this is backwards incompatible with the disabled value, but RUSTUP_IO_THREADS is markedunstable, so I'm ok taking the heat if folk do find they have scripts they have to update: depending on unstable interfaces should be done by version locking the dependencies anyway.

[rbtcollins]

I'm ok with this - thank you.

There's various bits of code golf that could be done differently but nothing consequential enough to be worth asking for it to be changed. (e.g. chain_err can take a string, for instance).

[faern]

Ok. I know chain_err can take a string. But you had so insanely many ErrorKind variants so I assumed the preferred style was to have explicit error kinds for everything. I did not spend that much time researching all existing chain_err instances. I can easily change it if you prefer an "opaque" string error.

[faern]

There's various bits of code golf that could be done differently

Feel free to tell me your opinions. I do care deeply about code cleanliness and I thought this was the nicest way of writing this. Separating getting the value out and actually creating the executor etc.

[rbtcollins]

Ok then :) - I try to balance obsession with contributor time :). And Rustup is a quite old codebase with lots of hair on it :). I think taking a string is generally good when an error comes from just one location, making an explicit enum entry for reuse is only worth it when well, we need to reuse it. Or for matching, which is very rare.

The Ok here could be factored to the outside of the expression:

 Ok(match thread_count {
        0 | 1 => Box::new(immediate::ImmediateUnpacker::new()),
        n => Box::new(threaded::Threaded::new(notify_handler, n)),
    })

Generally with error-chain the pattern is to import @ from errors :

use crate::errors::{...} -> use crate::errors::* - its one of the reasons we're trying to move away from it :).

[rbtcollins]

Anyhow, this is ready to merge - I've chatted with @kinnison about the env var change on discord. If you could rebase your changes to remove the exploratory changes, I'll merge this. Thanks!

[faern]

The Ok here could be factored to the outside of the expression

Yeah.. I did not try that since it usually leads to not so nice formatting. But in this case it actually did not add an extra level of indentation so it's fine! I changed it :)

use crate::errors::{...} -> use crate::errors::* - its one of the reasons we're trying to move away from it :).

I'm not a fan of wildcard imports. Makes it super hard to know what comes from where. And yeah, error-chain is pretty ancient at this point :/

I also made the error into just a string, as you wished. Now it should be ready for merge.

[rbtcollins]

Thank yoiu