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 threadlocal random generator to avoid contention #399

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eivanov89
Copy link

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.

* Constructor
*/
public RandomGenerator() {
Copy link
Collaborator

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 {
Copy link
Collaborator

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?

Copy link
Collaborator

@bpkroth bpkroth left a 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.

@eivanov89
Copy link
Author

Would be good if we allowed setting consistent seeds throughout the stack and this seems to exacerbate that issue.

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.

@bpkroth
Copy link
Collaborator

bpkroth commented Dec 5, 2023

Would be good if we allowed setting consistent seeds throughout the stack and this seems to exacerbate that issue.

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?

@eivanov89
Copy link
Author

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.

My intuition is that ThreadLocalRandom should produce consistent random numbers as Random does. But only if it creates per thread random generators with different seeds (I bet it does).

I absolutely agree, that we can use option to use this feature and have it non-default.

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

Successfully merging this pull request may close these issues.

2 participants