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

Feature for disabling system randomness? #74

Closed
reknih opened this issue Mar 18, 2022 · 4 comments
Closed

Feature for disabling system randomness? #74

reknih opened this issue Mar 18, 2022 · 4 comments

Comments

@reknih
Copy link
Contributor

reknih commented Mar 18, 2022

Hi,
for my use case of lipsum, I exclusively call the ..._from_seed functions. Hence, I do not need the system-provided randomness through rand::thread_rng and the likes. The rand crate collects its seeds through its getrandom dependency.

I'm a big fan of lean binaries so I wondered whether you'd be willing to create a feature for system randomness (enabled by default). This way, users like me could turn this feature off and drop not only getrandom but also cfg_if (transitively).

Do you think this is a good idea, and if so, what should this feature be called? ("Randomness", "system-seeded", "getrandom" are a few examples coming to my mind.) I'd be happy to submit a PR.

@mgeisler
Copy link
Owner

Hey Martin,

I would be happy with such a feature — I expect it will just be a few #[cfg(feature = "...")] lines sprinkled here and there :-) If it makes things more flexible for you and others, then I'm for it.

However, I wonder if you would see any actual improvement in binary size. I went through an exercise recently to measure the binary size in my Textwrap crate. See the binary-sizes example. If you clone Textwrap and do

$ cd examples/binary-sizes
$ cargo run --example make-table

then you see the size of an example program using different feature flags. I would suggest you write a similar tiny program and compile it with and without the dependencies (you don't need to do the whole table thing, we can always look at that later if we want).

My thesis is that it won't make a big difference: if you only call ..._from_seed functions, then the linker should strip out the dead code and you get a lean binary already. Especially when you use setting like lto = true when compiling.

@reknih
Copy link
Contributor Author

reknih commented Apr 25, 2022

I ran a few of my own experiments. You are right, the impact on file size is negligible. However, another metric has turned out to be more interesting: compile time.

I ran four compiles of each of the configurations in the table with a clean target directory using time cargo build. The table reports the real (wall-clock) time.

debug release
normal 9.8s - 13.5s 7.4s - 8.6s
--no-default-features 3.9s - 04.1s 2.7s - 3.3s
average change -35% -37%

(weirdly, release mode is faster here; the benchmarks were run on Debian Stretch, on WSL 2 in the mounted Windows file system, so disk I/O might be slow)

With the --no-default-features configuration, the features std, std_rng, and getrandom were omitted from rand. This reduces the number of total crates compiled from 8 to 5, dropping libc, getrandom, and cfg-if.

In theory, this would also enable a version of this crate that is no-std and instead just uses alloc but I don't really see a use case for that.

Do you think these compile time gains justify adding a new default feature? If yes, what do you think should be its name? (I called it getrandom but since it enables more than just that feature flag in rand it might not be especially apt)

@laurmaedje
Copy link

It think this feature would be nice to have because right now the library does not compile to WebAssembly. You can work around the error by adding getrandom as a direct dependency and enabling its "js" feature. But just disabling this library's randomness feature would be much nicer.

@mgeisler
Copy link
Owner

mgeisler commented Jun 9, 2022

Hey both, I'm sorry for not looking at this sooner! It had slipped my mind.

@reknih, thanks for actually testing it out! I'm personally not very upset about compile times in dependencies since I normally only compile them once per project. But I know that lots of people care about it :-)

@laurmaedje, thanks, I had no idea that it could not compile for WebAssembly! That's something I would love to fix!

@reknih, would you want to put up a PR with your changes? Then we can discuss names etc on that.

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

No branches or pull requests

3 participants