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

Experimental change to solve performance overshooting issue #1286

Open
wants to merge 1 commit into
base: fb-mysql-8.0.28
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions plugin/clone/src/clone_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,8 @@ uint64_t Client_Stat::task_target(uint64_t target_speed, uint64_t current_speed,
not set yet, start by assuming all active thread. */
auto active_tasks =
(current_target == 0) ? num_tasks : (current_speed / current_target);

active_tasks = num_tasks;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here is the calculation of active tasks is not correct. Let's say, the current_target is 700MB/S, and the current_speed is 900MB/S. Then the active_tasks will be 1.

According to the calculation for task_target below, the task target will be 700MB/s.
auto task_target = target_speed / active_tasks;
The task_target will be used as the current_target of next call of this task_target() function. Hence, it will go over the above calculation again and the task target speed will not be updated at all.

Since the max speed capacity for each thread is around 450MB/s, it will never throttle the thread. (Because it's always smaller than the target task speed 700MB/s).

For the change I made, I assume all threads created are active.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why InnoDB clone does not exhibit the issue?


/* Keep the value within current boundary. */
if (active_tasks == 0) {
Expand Down