HDDS-14345. Use ThreadLocalRandom in RandomPipelineChoosePolicy#9583
Conversation
There was a problem hiding this comment.
Thanks @siddhantsangwan for working on this.
I think a shared instance of Random is essential here. With a global random generator the sequence of pipelines is the same regardless of server threads (total number of threads, and which actual thread serves a specific request). With the thread-local generator the same pipeline may be chosen for several concurrent requests in the worst case. If that happens, container selection may be quicker by a small margin, but write will be slower due to hitting the same datanodes.
On the other hand, using nextInt(int) may help a bit without functional change. So I suggest:
- create a
private static final Random RANDOMinstance inRandomPipelineChoosePolicy - use
RANDOM.nextInt(pipelineList.size())
What's the reason for this? I think that can only happen if Java uses the same seed for each ThreadLocalRandom generator? I'm looking into how Java implements ThreadLocalRandom. Otherwise, even the shared instance of Random can output the same pipeline multiple times in the worst case. |
You are right, I assumed that was the case, but it looks like each thread has a different seed. |
|
Thanks @adoroszlai for the quick review. Please approve if it looks good to you. I've started the CI. |
What changes were proposed in this pull request?
Using ThreadLocalRandom instead of Math.random() gives significant performance improvement in my micro benchmark. Small improvement for the overall system, but still worth it since it's a simple change and this policy is used for each container allocation.
Without ThreadLocalRandom (current master branch):
So for selecting one out of 500 pipelines and 8 threads doing this, the latency is 295.03 nanoseconds per operation.
With ThreadLocalRandom:
Latency = 6.25 nanoseconds per operation.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14345
How was this patch tested?
Ran existing unit tests
TestWritableRatisContainerProviderandTestWritableECContainerProvider.CI is green in my fork - https://github.com/siddhantsangwan/ozone/actions/runs/20707115218