Make the const-random dependency optional

[koute]

Not everyone needs this, and the const-random adds a few extra relatively heavy dependencies which does cost build time, so I'd be nice to be able to disable it.

This also has the side effect of enabling reproducible builds, though, obviously, at a cost.

[tkaitchuck]

What I don't like about this is that it makes all the constants the same. If you're willing to wait for it I am working on a chance in constrandom to allow it to derive a number from the file name and row/column of call. That way it will still be random but consistent from one machine to the next.

[koute]

I completely agree that it'd be nice if the constants weren't the same. However, the main point of this is mostly to get rid of the extra dependencies. Will your changes in const-random make is so that there won't be any more extra dependencies pulled it?

Basically, if someone either:

1. doesn't care about randomizing the seeds at all (e.g. not using this to key a hash map, or the app is not user facing, etc.),
2. or wants to reduce the amount of dependencies during development,

then the extra dependencies are just dead weight, and I think it'd be nice to have a way to disable them. (And of course there is also rust-lang/cargo#5730.)

All of those transient dependencies can add up; as an example, one of my personal apps has 57 total dependencies when compiled in development mode and.... 436 dependencies when compiled in production mode. It works almost exactly the same, just without some production-only features and without a few nice-to-have-but-in-this-case-unnecessary features (like ahash's const-random).

[tkaitchuck]

While I certainly won't add any dependencies. It's already a dev dependency so it shouldn't bring anything at runtime. I suspect your concern is around the fact that it is downloading another thing, and it is in turn using random, which also needs to be downloaded, and has its own dependencies.

I can remove the dependency on Random via a the flag that triggers deterministic behaviour. So at that point cargo should only have to download one other thing (const-random). I don't however see a way to avoid const-random being separate because it is a procedural macro which currently requires it to be in it's own crate. And I don't know any way to make a macro-rules style macro emit different values at different invocation sites.

[tkaitchuck]

Thinking about it more, it might be possible to do as a macro-rules macro. I'll look into this.

[koute]

Well, to put it simply my main concern is that when I depend on ahash these are the extra dependencies I'm getting (as shown by cargo-tree in a clean checkout of current master):

ahash v0.2.12
└── const-random v0.1.6
    ├── const-random-macro v0.1.6
    │   ├── proc-macro-hack v0.5.9
    │   │   ├── proc-macro2 v1.0.3
    │   │   │   └── unicode-xid v0.2.0
    │   │   ├── quote v1.0.2
    │   │   │   └── proc-macro2 v1.0.3 (*)
    │   │   └── syn v1.0.5
    │   │       ├── proc-macro2 v1.0.3 (*)
    │   │       ├── quote v1.0.2 (*)
    │   │       └── unicode-xid v0.2.0 (*)
    │   └── rand v0.7.1
    │       ├── getrandom v0.1.12
    │       │   ├── cfg-if v0.1.9
    │       │   └── libc v0.2.62
    │       ├── libc v0.2.62 (*)
    │       ├── rand_chacha v0.2.1
    │       │   ├── c2-chacha v0.2.2
    │       │   │   └── ppv-lite86 v0.2.5
    │       │   └── rand_core v0.5.1
    │       │       └── getrandom v0.1.12 (*)
    │       └── rand_core v0.5.1 (*)

That is a lot of extra dependencies just for a feature that - while I agree is the default (and should be the default) for a very good reason - is completely unnecessary in my case. Yes, I know that at runtime it doesn't matter, however what I care about here is what happens at build time, as every extra transitive dependency that I have increases my compile times.

[koute]

Alternatively if you don't want to introduce a possibility of using non-random keys then perhaps we could instead have a feature which would control whenever ABuildHasher and impl Default for AHasher are defined? If those wouldn't be defined then there wouldn't be a need for the dependency on const-random since no random numbers would be necessary at compile time anymore.

[tkaitchuck]

Interesting, I can definitely replace rand with getrandom even in the case where we do want const-random values. That should help.

Right now there are two places const-random is used:

• The seed: In terms of security the initial seed for the key generator is not that important because it's pulling in the memory location where the map is located anyway. Which with ASLR should be a decent seed.
• Keys for implementing Default. This in my mind is genuinely questionable as really it is much better to invoke new.

So the easiest thing to do is via flag replace the seed with a constant, and just not implement Default

[koute]

Okay, so I've made the impl Default for AHasher conditional based on the compile-time-rng feature. Is this okay now?

One extra thing I've noticed - the fallback hasher exports only this constructor:

pub fn new_with_key(key: u64) -> AHasher

while the AES hasher exports only this constructor:

pub fn new_with_keys(key0: u64, key1: u64) -> Self

This means that depending on whenever target_feature = "aes" is true or not the AHasher will have a different public API. So while I was at it I've also quickly aligned both so that they export the same API. Not sure if this is how you'd like this to be fixed? Also, unfortunately this produces an unused method warning when compiling with target_feature = "aes" since the fallback AHasher is not exported then.

[tkaitchuck]

Yeah, I saw that too. I pushed a fix.