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 #359

Closed
wants to merge 0 commits into from

Conversation

eivanov89
Copy link

Hi,

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.

@apavlo
Copy link
Member

apavlo commented Sep 17, 2023

@eivanov89 Nice! I will take a look

@eivanov89
Copy link
Author

@apavlo
Attached missing flamegraph.

flamegraph-1107381

Copy link
Member

@apavlo apavlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to decide if we should just drop com.oltpbenchmark.util.RandomGenerator and replace it with ThreadLocalRandomGenerator. We can just replace that implementation with the new one (using the same name).

I don't think there will be an non-determinism issue...

@eivanov89
Copy link
Author

@apavlo it seems that we need your approval to run the workflow.

@eivanov89
Copy link
Author

@apavlo ping

@apavlo
Copy link
Member

apavlo commented Oct 19, 2023

@eivanov89 Thanks for poking me. I will look at this in more detail later this week.

@apavlo
Copy link
Member

apavlo commented Oct 19, 2023

@eivanov89 Let's have your RNG class replace the existing one. Can you do this?

@eivanov89
Copy link
Author

@eivanov89 Let's have your RNG class replace the existing one. Can you do this?

@apavlo Sure, I can try to do it next week.

@eivanov89
Copy link
Author

@eivanov89 Let's have your RNG class replace the existing one. Can you do this?

@apavlo I'm sorry for a long delay. Here is the updated pull request.

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