search

dhardy / rand

http://doc.rust-lang.org/rand
Other
2 stars 2 forks source link

Replace `ReadRng` with `read_exact in `OsRng` #48

Closed pitdicker closed 6 years ago

pitdicker commented 6 years ago

I feel the abstraction was not worth it.

dhardy commented 6 years ago

Pertinent question: do we want to share file-handles instead of re-opening?

nixpulvis commented 6 years ago

My gut reaction is that sharing file descriptors is only a win, since it'll take some load off the OS. I know there are some considerations around having a bunch of descriptors allocated at once, but I'm not an expert here.

dhardy commented 6 years ago

@nixpulvis that was a rhetorical question (we already discussed to some extent). PRs welcome BTW.

pitdicker commented 6 years ago

I see some comments that file descriptors to the same file get reference counted on the kernel side across multiple threads. Then it may not make any difference if we start to reference count them on our side instead. But this would need some testing to be sure.

Once I fix the merge conflict, is this ok to be merged?

dhardy commented 6 years ago

I don't see any advantage to removing ReadRng (a very slight reduction in complexity maybe, but it's marginal). OTOH if further changes get made to this code in the future the extra layer of abstraction may be useful. So it seems like a step backwards removing it, even though it's not necessary.