-
Notifications
You must be signed in to change notification settings - Fork 13
TOOLS-2525: Improve performance of async benchmarks #90
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
base: main
Are you sure you want to change the base?
Conversation
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 LGTM, thanks for the PR. @kportertx could you re-run the failing test action? It isn't giving me the option. The TPS limit tests that failed fail intermittently but I'd like to see them pass.
EDIT: nevermind, triggered another run with some usage text changes
Question about performance tuning. So if each event loop creates new transactions then --event-loops is the thread count to tune in async mode, correct? It's essentially the --threads of async mode. I wonder how we can make this as straight forward as possible to users. I'm thinking just add some text to the --threads help that says to use --event-loops when running in async mode. |
Maybe drop the |
I like that idea, I'll run it past @hev. |
@kportertx I think the async throttle tests are failing because |
I'll implement the lock and let you know so you can take a look |
@kportertx Allowing the event loop threads to create transactions causes some issues with the --throughput throttling functionality, and potentially other areas. It was assumed that the thread data struct tdata_t and dynamic throttle struct dyn_throttle_t would only ever be called modified in the same thread. Since dyn_throttle_pause_for is called by multiple threads now I think race conditions can occur. Throttling used to take place in the 1 thread putting transactions on the event loop, now, by adding transactions and throttling in the event loop threads, throughput isn't being throttled correctly. I think one reason is that many transactions are being started by the event loop thread before it gets to a callback where it can do any throttling. Since higher async tps is possbile by running multiple asbench processes we are treating this as low priority. I'll come back to this when I can but don't have the time to look into fixes for this change and --throughput right now. Observations...
higher limits still get low performance (normally this command gets around 3k tps on my setup)
I did try throwing a lock around the throttle changes in the test-throttle-lock branch but it didn't change the throttling or anything, that race may not be a big issue. |
The java client's benchmark doesn't implement throttling for async so I implemented a method a while back - see Basically there is a quota for how many transactions need to be completed within the current interval. Worker threads decrement the quota until it reaches 0 and then stop. A Manager thread periodically increases the quota and restarts stopped threads. This worked well for the testing that we were doing at the time. |
Ooops, did not mean to close |
Thanks, I will take a look |
- Seems to work with `--event-loops`. FIXME - Multiple `--event-loops` eventually breaches the throttle. - Noticed that it occasionally freezes at the end - Workload threads have all exited, could be a pre-exisiting bug.
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.
Thanks for the PR! Just a few minor questions.
- Add parens to clarify `min_usleep` calculation. - Remove defunct `target_per_thread` variable and loop.
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.
The code LGTM but we need the tests to pass. I'll take a closer look at them this week.
@dwelch-spike, looks like the async batch workloads are broken. Unsure when I'll be able to look more closely at this. |
@kportertx I worked on async batches somewhat recently so I'll take a look soon. |
No description provided.