Use a pool of rand.Source to reduce lock contention #357
Conversation
…n ids Signed-off-by: Gary Brown <[email protected]>
ghost
assigned 
|
Running a test creating 5 million spans, using the existing and new approach, shows that generally they take the same time (8 to 9 seconds) - however on occasions the existing approach resulted in runs of 18 seconds. Checking the mutex profiling showed far less time spent - on the pool, rather than the mutex around random number generation. @yurishkuro If the implementation looks ok, is it worth asking the Istio team to try it out before we consider merging? |
Codecov Report
@@ Coverage Diff @@
## master #357 +/- ##
==========================================
+ Coverage 87.19% 87.22% +0.02%
==========================================
Files 54 54
Lines 2999 3005 +6
==========================================
+ Hits 2615 2621 +6
Misses 272 272
Partials 112 112
Continue to review full report at Codecov.
|
yurishkuro
changed the title
| seedGenerator := utils.NewRand(time.Now().UnixNano()) | ||
| pool := sync.Pool{ | ||
| New: func() interface{} { | ||
| return rand.NewSource(seedGenerator.Int63()) |
There was a problem hiding this comment.
could we bump this to use int64? I think it's now available from the stdlib.
|
sorry, already merged - def. worth asking them to try, but overall seems like a worthy improvement. |
| seedGenerator := utils.NewRand(time.Now().UnixNano()) | ||
| pool := sync.Pool{ | ||
| New: func() interface{} { | ||
| return rand.NewSource(seedGenerator.Int63()) |
There was a problem hiding this comment.
Sorry to see this late, but I'm not sure I fully understand the logic here. Perhaps I'm missing something...
If the underlying issue is lock contention, how does moving to a sync.Pool solve the issue? Get() still involves a mutex, correct? And inside of that, we are still calling Int63() on a locked source, correct?
It feels like we want something that is lock-free. Have I missed something obvious?
There was a problem hiding this comment.
True there is a mutex on the pool, but the pool is returning a rand.Source which is not a locked resource. So there is no lock being held while the random numbers are being generated.
There was a problem hiding this comment.
Do we believe that getting a source is quicker than generating the numbers?
There was a problem hiding this comment.
From some basic tests, as described here #357 (comment), the benefit was more related to consistent results.
Not sure if @mandarjog has had a chance to try the latest version in the same test scenario to see whether it performed better?
when creating span ids
Signed-off-by: Gary Brown [email protected]
Which problem is this PR solving?
Resolves #353 - possibly :)
Short description of the changes
Instead of using a lock around the random number generation, a pool of
rand.Sources is maintained.