grpc-go icon indicating copy to clipboard operation
grpc-go copied to clipboard

Use math/rand top-level functions in grpcrand

Open huangchong94 opened this issue 1 year ago • 3 comments

Background

Although the rand object in math/rand is not safe for concurrent access , but the top-level functions defined in math/rand are concurrent-safe and since go1.20.0 these functions are auto-seeded.

Starting from 1.21.0 those functions are not only concurrent-safe, auto-seeded, but also lock-free (most of them are lock-free, except rand.Read), which run significantly faster than rand with mutex, especially for parallel calls.

The grpcrand package is widely used in grpc-go, some of its functionality is used in critical path like Picker.Pick (eg picker in leastrequest.go). The current approach of grpcrand uses a global rand object seeded by system time with mutex protection to generate random number in a concurrent-safe manner.

Proposed Solution

I propose replacing rand object+mutex with math/rand package's top-level functions to boost performance and to simplify the code in grpcrand.

Pros & Cons

Pros

go version >=1.21.0

  • Significantly faster execution when generating random number, especially for parallel calls (up to 30x speedup)
  • Simplified implementation

go version < 1.21.0

  • Simplified implementation

Cons

go version >=1.21.0

  • Currently, there are no apparent drawbacks I can think of

1.20.0 <= go version < 1.21.0

  • In my computer in non parallel case, occasionally top-level function ran much slower(rand.Intn) than rand object+mutex, but the results are not stable. (up to ~20% slower, usually below 10%)

go version <1.20.0

  • Top-level functions will produce a deterministic sequence of values each time a program is run
  • In my computer in non parallel case, occasionally top-level function ran much slower(rand.Intn) than rand object +mutex, but the results are not stable.(up to ~20% slower, usually below 10%)

Using top-level functions will simplify the implementation of grpcrand and significantly improve grpcrand's performance(go version>=1.21.0), the downside is the potential performance degradation when using go version below 1.21.0. As for deterministic sequence of values when using go version below 1.20.0, I think it would not cause a security problem, because if grpcrand is used for secure PRNG, implementation should use crypto/rand instead of math/rand, but it might lead to other issues that I haven't considered.

Conclusion

Since grpc-go only supports the three latest major versions of go, even if we opt to continue using rand object + mutex for now, as go releases number bumping up, the cons will gradually disappear. At that point, using math/rand's top-level functions to generate random numbers will be a highly favorable choice.

huangchong94 avatar Sep 20 '23 05:09 huangchong94

This seems to be worth doing eventually, and we could phase it now by doing a conditional build of the grpcrand package.

I am fairly sure I added this package initially because I was concerned about the fact that the global random source is shared globally, meaning other parts of the application could interfere (e.g. by calling Seed(0) repeatedly) but since we are not using it for cryptographic purposes, it's probably fine, and any library already in your binary can do much worse things than influence your randomness.

dfawley avatar Oct 02 '23 16:10 dfawley

@dfawley, As a new person to gRPC-go contribution, I would like to work on this issue, However I am not sure if conditional building would go well. Do you recommend trying?

kmirzavaziri avatar Jan 14 '24 17:01 kmirzavaziri

@kmirzavaziri sure, that sounds good. The conditional build I had in mind was just based on the Go version, by the way, not a custom build flag.

dfawley avatar Jan 17 '24 18:01 dfawley