Pipeline: support multi-level feedback queue #7393

SeaRise · 2023-04-26T18:38:13Z

What problem does this PR solve?

Issue Number: ref #6518

Problem Summary:

What is changed and how it works?

Use TaskProfileInfo to record cpu_execute_time, cpu_pending_time, io_execute_time, io_pending_time and await_time of Task.
support multi-level feedback queue base on TaskProfileInfo.

///    +------------+     +------------+       +------------+           +------------+
///    | UnitQueue 1|     | UnitQueue 3|       | UnitQueue 3|    ...    | UnitQueue 8|
///    +------------+     +------------+       +------------+           +------------+
///          ^                   ^                   ^                        ^
///          |                   |                   |                        |
/// +--------+--------+  +-------+--------+  +-------+--------+       +-------+--------+
/// | Task 1          |  | Task 6         |  | Task 11        |       | Task 16        |
/// +-----------------+  +----------------+  +----------------+       +----------------+
///          ^                   ^                   ^                        ^
///          |                   |                   |                        |
/// +--------v--------+  +-------v--------+  +-------v--------+       +-------v--------+
/// | Task 2          |  | Task 7         |  | Task 12        |       | Task 17        |
/// +-----------------+  +----------------+  +----------------+       +----------------+
///          ^                   ^                   ^                        ^
///          |                   |                   |                        |
/// +--------v--------+  +-------v--------+  +-------v--------+       +-------v--------+
/// | Task 3          |  | Task 8         |  | Task 13        |       | Task 18        |
/// +-----------------+  +----------------+  +----------------+       +----------------+
///          ^                   ^                   ^                        ^
///          |                   |                   |                        |
/// +--------v--------+  +-------v--------+  +-------v--------+       +-------v--------+
/// | Task 4          |  | Task 9         |  | Task 14        |       | Task 19        |
/// +-----------------+  +----------------+  +----------------+       +----------------+

Check List

Tests

tsan passed
- gtest_filter=*Event*
- gtest_filter=*TaskScheduler*
- gtest_filter=*Executor*
- gtest_filter=*ComputeServerRunner*
- gtest_filter=*TestMLFQTaskQueue*
Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2023-04-26T18:38:15Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

windtalker
xzhangxian1008

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

SeaRise · 2023-04-26T19:01:32Z

/run-all-tests

xzhangxian1008 · 2023-04-27T09:21:09Z

/assign

dbms/src/Flash/Pipeline/Schedule/TaskQueues/MultiLevelFeedbackQueue.h

…Queue.h

SeaRise · 2023-04-27T10:14:57Z

/rebuild

xzhangxian1008 · 2023-04-29T00:50:51Z

Could you briefly describe what factors determine the priority of a task?

SeaRise · 2023-05-04T06:42:22Z

Could you briefly describe what factors determine the priority of a task?

ok, added in https://github.com/pingcap/tiflash/pull/7393/files#diff-53742cbbf90fcb7d07521ec0386e00c92507570de3e07c2a20422c4a91b1b84cR61-R68

SeaRise · 2023-05-05T08:59:09Z

/run-all-tests

SeaRise · 2023-05-05T09:00:35Z

/run-all-tests

windtalker · 2023-05-09T02:08:10Z

dbms/src/Flash/Pipeline/Schedule/TaskQueues/MultiLevelFeedbackQueue.cpp

+    task_queue.push_back(std::move(task));
+}
+
+double UnitQueue::accuTimeAfterDivisor()


maybe rename it to normalizedTime?

ok, renamed.

dbms/src/Operators/UnorderedSourceOp.h

dbms/src/Flash/Pipeline/Schedule/Tasks/TaskProfileInfo.cpp

windtalker · 2023-05-09T02:42:44Z

dbms/src/Flash/Pipeline/Schedule/TaskThreadPool.cpp

        // The executing task should yield if it takes more than `YIELD_MAX_TIME_SPENT_NS`.
-        if (status != Impl::TargetStatus || execute_time_ns >= YIELD_MAX_TIME_SPENT_NS)
+        if (status != Impl::TargetStatus || total_time_spent >= YIELD_MAX_TIME_SPENT_NS)


YIELD_MAX_TIME_SPENT_NS is the same for different level queue?

I think it can be the same, because the minimum time slice of the queue is 200ms, which is greater than the 100ms here.

windtalker · 2023-05-10T01:53:27Z

dbms/src/Flash/Pipeline/Schedule/TaskThreadPool.cpp

        // The executing task should yield if it takes more than `YIELD_MAX_TIME_SPENT_NS`.
-        if (status != Impl::TargetStatus || execute_time_ns >= YIELD_MAX_TIME_SPENT_NS)
+        if (status != Impl::TargetStatus || total_time_spent >= YIELD_MAX_TIME_SPENT_NS)


windtalker · 2023-05-10T02:00:19Z

dbms/src/Flash/Pipeline/Schedule/TaskThreadPoolImpl.h


    static QueueType newTaskQueue()
    {
-        return std::make_unique<FIFOTaskQueue>();
+        return std::make_unique<CPUMultiLevelFeedbackQueue>();


Why cpu queue use MultiLevelFeedbackQueue and io queue use FIFOTaskQueue? And I think maybe we should add a configure variable to decide which queue to used?

I think IO-related operations should use a different type of queue, such as performing spill before restore.

And I think maybe we should add a configure variable to decide which queue to used?

ok

config added.

windtalker

LGTM

xzhangxian1008 · 2023-05-10T08:08:35Z

dbms/src/Flash/Pipeline/Schedule/Tasks/TaskProfileInfo.h

+    UnitType io_pending_time = 0;  \
+    UnitType await_time = 0;
+
+class LocalTaskProfileInfo


What does Local mean? Do we have RemoteTaskProfileInfo?

I think that in the future, all TaskProfileInfo will be counted together to calculate the amount of resources used by the query, but now I can remove it.

renamed to TaskProfileInfo.

xzhangxian1008 · 2023-05-10T08:09:50Z

dbms/src/Flash/Pipeline/Schedule/Tasks/TaskProfileInfo.h

+class LocalTaskProfileInfo
+{
+public:
+    PROFILE_MEMBER(UInt64)


It seems that the PROFILE_MEMBER is used at only one place, is macro necessary?

ok, removed.

xzhangxian1008 · 2023-05-10T08:11:20Z

dbms/src/Flash/Pipeline/Schedule/Tasks/TaskProfileInfo.h

+
+class LocalTaskProfileInfo
+{
+public:


Maybe put these variable in private sector and get them with related interfaces?

xzhangxian1008

Other LGTM

xzhangxian1008 · 2023-05-10T08:43:14Z

dbms/src/Flash/Pipeline/Schedule/TaskQueues/MultiLevelFeedbackQueue.cpp

+#include <Flash/Pipeline/Schedule/TaskQueues/MultiLevelFeedbackQueue.h>
+#include <assert.h>
+#include <common/likely.h>


Suggested change

#include <Flash/Pipeline/Schedule/TaskQueues/MultiLevelFeedbackQueue.h>

#include <assert.h>

#include <common/likely.h>

#include <Flash/Pipeline/Schedule/TaskQueues/MultiLevelFeedbackQueue.h>

#include <common/likely.h>

#include <assert.h>

But clang-format likes this :)

xzhangxian1008 · 2023-05-10T08:44:29Z

dbms/src/Flash/Pipeline/Schedule/Tasks/Task.h

 #include <Common/Logger.h>
 #include <Common/MemoryTracker.h>
+#include <Flash/Pipeline/Schedule/Tasks/TaskProfileInfo.h>
 #include <memory.h>


Suggested change

#include <Common/Logger.h>

#include <Common/MemoryTracker.h>

#include <Flash/Pipeline/Schedule/Tasks/TaskProfileInfo.h>

#include <memory.h>

#include <Common/Logger.h>

#include <Common/MemoryTracker.h>

#include <Flash/Pipeline/Schedule/Tasks/TaskProfileInfo.h>

#include <memory.h>

But clang-format likes this :)

…Queue.cpp Co-authored-by: xzhangxian1008 <[email protected]>

Co-authored-by: xzhangxian1008 <[email protected]>

SeaRise · 2023-05-10T09:05:49Z

/merge

ti-chi-bot · 2023-05-10T09:05:51Z

@SeaRise: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-05-10T09:05:52Z

This pull request has been accepted and is ready to merge.

Commit hash: f593ab2

SeaRise · 2023-05-10T10:23:51Z

/run-unit-test

ti-chi-bot · 2023-05-10T10:42:19Z

@SeaRise: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

trigger some heavy tests which will not run always when PR updated.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

support_mlfq

7ac0be7

ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 26, 2023

SeaRise added 2 commits April 27, 2023 02:44

u

62d37ac

update

72b9045

SeaRise changed the title ~~WIP: Pipeline: support mlfq~~ Pipeline: support mlfq Apr 26, 2023

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 26, 2023

SeaRise mentioned this pull request Apr 26, 2023

Support pipeline model #6518

Closed

25 tasks

SeaRise changed the title ~~Pipeline: support mlfq~~ Pipeline: support multi level feedback queue Apr 27, 2023

SeaRise changed the title ~~Pipeline: support multi level feedback queue~~ Pipeline: support multi-level feedback queue Apr 27, 2023

update

a6bc958

ti-chi-bot bot assigned xzhangxian1008 Apr 27, 2023

udpate

b0aba7e

SeaRise commented Apr 27, 2023

View reviewed changes

dbms/src/Flash/Pipeline/Schedule/TaskQueues/MultiLevelFeedbackQueue.h Outdated Show resolved Hide resolved

Update dbms/src/Flash/Pipeline/Schedule/TaskQueues/MultiLevelFeedback…

7e66fcb

…Queue.h

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2023

Merge branch 'master' into support_mlfq

ccae5f9

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2023

add more comments

3043b71

nit

1db56eb

Merge branch 'master' into support_mlfq

5cef666

SeaRise requested a review from windtalker May 8, 2023 03:52

windtalker reviewed May 9, 2023

View reviewed changes

rename

f246d56

SeaRise requested a review from windtalker May 9, 2023 05:40

windtalker reviewed May 10, 2023

View reviewed changes

SeaRise added 2 commits May 10, 2023 10:48

Merge branch 'master' into support_mlfq

7b08f64

add settings

c462f9d

SeaRise requested a review from windtalker May 10, 2023 03:35

windtalker approved these changes May 10, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 10, 2023

fix build

71dce63

xzhangxian1008 reviewed May 10, 2023

View reviewed changes

ddress comments

d2f062c

SeaRise requested a review from xzhangxian1008 May 10, 2023 08:40

xzhangxian1008 approved these changes May 10, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 10, 2023

SeaRise and others added 3 commits May 10, 2023 17:03

Update dbms/src/Flash/Pipeline/Schedule/TaskQueues/MultiLevelFeedback…

41f12cb

…Queue.cpp Co-authored-by: xzhangxian1008 <[email protected]>

Update dbms/src/Flash/Pipeline/Schedule/Tasks/Task.h

a82f290

Co-authored-by: xzhangxian1008 <[email protected]>

fmt

f593ab2

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label May 10, 2023

Merge branch 'master' into support_mlfq

59e8faf

Merge branch 'master' into support_mlfq

45c63e0

ti-chi-bot bot merged commit 241a19a into pingcap:master May 10, 2023

SeaRise deleted the support_mlfq branch May 10, 2023 11:43

Pipeline: support multi-level feedback queue #7393

Pipeline: support multi-level feedback queue #7393

Conversation

SeaRise commented Apr 26, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot bot commented Apr 26, 2023 • edited Loading

SeaRise commented Apr 26, 2023

xzhangxian1008 commented Apr 27, 2023

SeaRise commented Apr 27, 2023

xzhangxian1008 commented Apr 29, 2023

SeaRise commented May 4, 2023

SeaRise commented May 5, 2023

SeaRise commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeaRise May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

windtalker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzhangxian1008 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeaRise commented May 10, 2023

ti-chi-bot bot commented May 10, 2023

ti-chi-bot bot commented May 10, 2023

SeaRise commented May 10, 2023

ti-chi-bot bot commented May 10, 2023

SeaRise commented Apr 26, 2023 •

edited

Loading

ti-chi-bot bot commented Apr 26, 2023 •

edited

Loading

SeaRise May 10, 2023 •

edited

Loading