-
Notifications
You must be signed in to change notification settings - Fork 184
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 threadlocal random generator to avoid contention #399
base: main
Are you sure you want to change the base?
Conversation
* Constructor | ||
*/ | ||
public RandomGenerator() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version drops the seed
from the constructor. This could be very useful for implementing more reproducible benchmarking results. Not claiming that benchbase does a great job of carrying that thru all levels of the stack now, but this seems to introduce further issues with that.
import java.util.concurrent.ThreadLocalRandom; | ||
|
||
public class RandomGenerator extends java.util.Random { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it still produce the same results as the previous version? Any tests for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerned about the seed aspects of this.
Would be good if we allowed setting consistent seeds throughout the stack and this seems to exacerbate that issue.
Not claiming that this PR should fix all of those, but it at least shouldn't make it worse.
As far as I know, thread local random doesn't allow to set the seed. Though, I don't see any particular places at Benbench, where you might need it. My original intent in the previous pull request 359 was to introduce a separate random generator without the seed. But we opted for a global new one. So, I suggest either to go with this one (and maybe in future fix it to support seeds, e.g. by using manually created thread local random generators) or to use my previous pull request with custom rand. |
I think I'd prefer the previous method - make it an optional use feature for now, with lots of documentation/caveats about what the implications of not using consistent random generation might be. @apavlo, thoughts? |
My intuition is that I absolutely agree, that we can use option to use this feature and have it non-default. |
This is updated pull request, previous one is here.
Copy-pasted description:
I noticed a very high CPU consumption, when import initial TPC-C data. According the attached flamegraph all threads are in TPCCUtil:::randomStr. Because of util.RandomGenerator there is a contention. This patch uses thread local random generator, which helps to avoid contention and dramatically reduces CPU consumption.