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 of rand.Float64() reduces parallelism #80

Closed
eliaslevy opened this issue Feb 12, 2019 · 6 comments
Closed

Use of rand.Float64() reduces parallelism #80

eliaslevy opened this issue Feb 12, 2019 · 6 comments

Comments

@eliaslevy
Copy link

The global default math/rand generator is protected by a mutex. In a highly concurrent application making lots of calls to the DD library and making use of the rate parameter to the API sample metrics so as to not drop packets and overload the statsd server, the use of rand.Float64 in statsd.Client.send can become a bottleneck.

Ideally, there would be an alternative API that allows the caller to pass in goroutine local math.rand.Rand, which is not protected by a mutex.

@eliaslevy eliaslevy changed the title Use use rand.Float64() reduces parallelism Use of rand.Float64() reduces parallelism Feb 12, 2019
@arbll
Copy link
Member

arbll commented Apr 11, 2019

Hi @eliaslevy,

This looks like it would be a nice improvement. Having an alternative API would work but I am wondering if using a separate math.rand.Rand for each client instance and creating one client per goroutine wouldn't be cleaner.
WDYT ?

@eliaslevy
Copy link
Author

eliaslevy commented Apr 11, 2019

That would work. Although it would require folks using the buffered client to reconfigure the timeout or buffer size value so the system to output roughly the same number of packets as when using a single global client.

I went a somewhat different route. I used sync.Pool to get a per-CPU RNG. The only issue is that per my understanding of sync.Pool, the RNG is destroyed during GC and recreated again and again. But it was a quick hack that did not require any API changes.

@arbll
Copy link
Member

arbll commented Nov 28, 2019

Once #108 is merged I should be able to simply add a rand per "worker".

@matthewdale
Copy link
Contributor

@arbll I opened a PR that uses a *rand.Rand per worker to avoid the math/rand package-global RNG lock. Please check it out here: #178

@arbll
Copy link
Member

arbll commented Jan 11, 2021

Hey - I no longer work on this repo
cc @hush-hush

@hush-hush
Copy link
Member

#178 was merged and release (see v4.3.1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants