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

Use math/rand top-level functions in grpcrand #6650

Closed
huangchong94 opened this issue Sep 20, 2023 · 3 comments · Fixed by #6925
Closed

Use math/rand top-level functions in grpcrand #6650

huangchong94 opened this issue Sep 20, 2023 · 3 comments · Fixed by #6925
Assignees
Labels
Type: Performance Performance improvements (CPU, network, memory, etc)

Comments

@huangchong94
Copy link
Contributor

huangchong94 commented Sep 20, 2023

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 huangchong94 added the Type: Feature New features or improvements in behavior label Sep 20, 2023
@huangchong94 huangchong94 changed the title Use top-level rand functions in grpcrand Use math/rand top-level functions in grpcrand Sep 21, 2023
@ginayeh ginayeh added the P2 label Sep 22, 2023
@dfawley
Copy link
Member

dfawley commented Oct 2, 2023

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 dfawley added Type: Performance Performance improvements (CPU, network, memory, etc) and removed Type: Feature New features or improvements in behavior labels Oct 2, 2023
@kmirzavaziri
Copy link
Contributor

@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?

@dfawley
Copy link
Member

dfawley commented Jan 17, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Performance Performance improvements (CPU, network, memory, etc)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants