Discussion: Replace rander with factory to allow pooling instances about ksuid

[achille-roussel]

I’m not particularly fond of the factory suggestion. The reason there’s a lock is because the random number generators in math/rand are not safe for concurrent use, and those in crypto/rand are super expansive in terms of CPU time, so the cost of synchronizing on a mutex is very low in comparison.

That being said, I haven’t run any benchmarks to test the performance of the crypto/rand generator when used in parallel from multiple goroutines, so that may be a good starting point.

We use this package a lot at Segment and we’ve never observed contention on this lock in any production service, so I’m not sure changing the API would be worth it.

[dahankzter]

No perhaps you are right that in a service where you anyway hit databases and such it is not hot spot but in general the idiom is to put random sources and such are generally in a sync.Pool. As for scalability on truly multi core systems I think you can measure it quite easily but of course it needs testing.

The idea was to explore if there is a simple way to allow for the caller to decide this. When the lock is hard wired in the library it is impossible to get around.

I am not sure of the actual usage of the library but if it can be done in a non breaking way perhaps you would be open to it?

[achille-roussel]

Sure, if we can make the change with no API break there’s no reason not to.

We could also modify the constraints (like requiring the rander to be concurrent-safe), I’d really like to have a real world example showing that this lock is an contention point before making this kind of change tho.

A program can also use ksuid.FromParts if it needs to generate the radom payload itself.