Storages: refine `SegmentReadTaskScheduler` #8557

JinheLin · 2023-12-20T07:36:25Z

What problem does this PR solve?

Issue Number: ref #6834

Problem Summary:

What is changed and how it works?

The main updates of this PR is removing the code of CircularScanList and refining SegmentReadTaskScheduler. Removing CircularScanList is because it's not very necessary.
The design of CircularScanList is to make each iteration of read_pools start from the previous end position. Meanwhile, for the convenience of accessing by pool_id, also added an unordered_map to index all pools.
CircularScanList also will release pools that are never used, causing inconvenience in some monitoring statistics.
This PR deletes the code of CircularScanList, and just uses an unordered_map to store read_pools.
When scheduling, try scheduling each pool in the read_pools before releasing lock.
The biggest difference is that the new code will try to schedule all pools in read_pools before the scheduler releasing its lock.
Also add some unit-tests of SegmentReadTaskScheduler.
Add CPU usage of threads:

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
- Run chbenchmark workload with concurrent queries.
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-12-20T07:36:28Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

JinheLin · 2023-12-20T07:36:47Z

/run-all-tests

JinheLin · 2023-12-20T10:28:03Z

/run-all-tests

JinheLin · 2023-12-20T12:39:21Z

/run-unit-tests

JinheLin · 2023-12-21T07:37:24Z

/run-all-tests

JinheLin · 2023-12-21T09:32:08Z

/run-integration-test

JinheLin · 2023-12-21T10:35:54Z

/run-integration-test

dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.h

Lloyd-Pottiger

LGTM

CalvinNeo · 2024-01-02T07:31:00Z

dbms/src/Storages/DeltaMerge/ReadThread/MergedTask.h

@@ -127,7 +127,11 @@ class MergedTask
        }
    }

+#ifndef DBMS_PUBLIC_GTEST


Why? I found these 3 methods not accessed in gtest_segment_read_tasks.

Use the member variables in gtest_segment_read_tasks. For example,

ASSERT_EQ(merged_task->units.size(), 1);

CalvinNeo · 2024-01-02T07:34:06Z

dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp

    Stopwatch sw_do_add;
-    read_pools.add(pool);
+    read_pools.emplace(pool->pool_id, pool);


The SegmentReadTaskScheduler::add is public, it could be called by someone else. Wo we should assert pool to be not nullptr.

dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp

JaySon-Huang

LGTM

ti-chi-bot · 2024-01-02T13:47:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang, Lloyd-Pottiger

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JaySon-Huang,Lloyd-Pottiger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-01-02T13:47:54Z

[LGTM Timeline notifier]

Timeline:

2023-12-28 08:51:40.903548162 +0000 UTC m=+1728591.940775089: ☑️ agreed by Lloyd-Pottiger.
2024-01-02 13:47:53.886218722 +0000 UTC m=+2178364.923445649: ☑️ agreed by JaySon-Huang.

JaySon-Huang · 2024-01-02T14:56:19Z

/hold

JaySon-Huang · 2024-01-02T16:36:52Z

/run-all-tests

JaySon-Huang · 2024-01-03T00:39:30Z

/unhold

JinheLin · 2024-05-07T14:36:55Z

/cherry-pick to release-7.5

ti-chi-bot · 2024-05-07T14:37:29Z

@JinheLin: cannot checkout to release-7.5: error checking out to release-7.5: exit status 1. output: error: pathspec 'to release-7.5' did not match any file(s) known to git

In response to this:

/cherry-pick to release-7.5

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 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 Dec 20, 2023

JinheLin force-pushed the refine_scheduler branch from f12267e to fe654df Compare December 21, 2023 07:22

JinheLin marked this pull request as ready for review December 21, 2023 07:36

JinheLin changed the title ~~[WIP]Storages: refine SegmentReadTaskScheduler~~ Storages: refine SegmentReadTaskScheduler Dec 21, 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 Dec 21, 2023

JinheLin force-pushed the refine_scheduler branch 2 times, most recently from b965301 to c275ba1 Compare December 27, 2023 10:03

JinheLin requested review from JaySon-Huang, Lloyd-Pottiger and CalvinNeo December 28, 2023 06:17

Lloyd-Pottiger reviewed Dec 28, 2023

View reviewed changes

Lloyd-Pottiger approved these changes Dec 28, 2023

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Dec 28, 2023

CalvinNeo reviewed Jan 2, 2024

View reviewed changes

JinheLin force-pushed the refine_scheduler branch from 8949a86 to 0b717e8 Compare January 2, 2024 09:26

JinheLin added 4 commits January 2, 2024 17:27

Storages: refine SegmentReadTaskScheduler

87827c4

comment

1c76d62

comment

21ec867

Add assert

2586b05

JinheLin force-pushed the refine_scheduler branch from 0b717e8 to 2586b05 Compare January 2, 2024 09:28

JaySon-Huang reviewed Jan 2, 2024

View reviewed changes

dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp Show resolved Hide resolved

JaySon-Huang reviewed Jan 2, 2024

View reviewed changes

dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp Show resolved Hide resolved

JaySon-Huang approved these changes Jan 2, 2024

View reviewed changes

ti-chi-bot bot added the lgtm label Jan 2, 2024

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jan 2, 2024

ti-chi-bot bot added 2 commits January 2, 2024 14:01

Merge branch 'master' into refine_scheduler

ffcc9d8

Merge branch 'master' into refine_scheduler

9d383d9

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 2, 2024

Merge branch 'master' into refine_scheduler

86f138f

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 3, 2024

ti-chi-bot bot merged commit 64c1ae5 into pingcap:master Jan 3, 2024
6 checks passed

JinheLin added a commit to JinheLin/tiflash-1 that referenced this pull request May 7, 2024

cp pingcap#8557

ab3cca4

JinheLin mentioned this pull request May 7, 2024

Storages: Avoid SegmentReadTaskScheduler blocking the adding threads and ignore empty SegmentReadTaskPool #9026

Merged

12 tasks

This was referenced Aug 26, 2024

Set thread name for seg task scheduler #9362

Merged

ddl: Fix rename come after database has been dropped (#9274) #9350

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storages: refine `SegmentReadTaskScheduler` #8557

Storages: refine `SegmentReadTaskScheduler` #8557

JinheLin commented Dec 20, 2023 •

edited

Loading

ti-chi-bot bot commented Dec 20, 2023

JinheLin commented Dec 20, 2023

JinheLin commented Dec 20, 2023

JinheLin commented Dec 20, 2023

JinheLin commented Dec 21, 2023

JinheLin commented Dec 21, 2023

JinheLin commented Dec 21, 2023

Lloyd-Pottiger left a comment

CalvinNeo Jan 2, 2024

JinheLin Jan 2, 2024

CalvinNeo Jan 2, 2024

JinheLin Jan 2, 2024

JaySon-Huang left a comment

ti-chi-bot bot commented Jan 2, 2024

ti-chi-bot bot commented Jan 2, 2024

JaySon-Huang commented Jan 2, 2024

JaySon-Huang commented Jan 2, 2024

JaySon-Huang commented Jan 3, 2024

JinheLin commented May 7, 2024

ti-chi-bot commented May 7, 2024

Storages: refine SegmentReadTaskScheduler #8557

Storages: refine SegmentReadTaskScheduler #8557

Conversation

JinheLin commented Dec 20, 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 Dec 20, 2023

JinheLin commented Dec 20, 2023

JinheLin commented Dec 20, 2023

JinheLin commented Dec 20, 2023

JinheLin commented Dec 21, 2023

JinheLin commented Dec 21, 2023

JinheLin commented Dec 21, 2023

Lloyd-Pottiger left a comment

Choose a reason for hiding this comment

CalvinNeo Jan 2, 2024

Choose a reason for hiding this comment

JinheLin Jan 2, 2024

Choose a reason for hiding this comment

CalvinNeo Jan 2, 2024

Choose a reason for hiding this comment

JinheLin Jan 2, 2024

Choose a reason for hiding this comment

JaySon-Huang left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jan 2, 2024

ti-chi-bot bot commented Jan 2, 2024

[LGTM Timeline notifier]

JaySon-Huang commented Jan 2, 2024

JaySon-Huang commented Jan 2, 2024

JaySon-Huang commented Jan 3, 2024

JinheLin commented May 7, 2024

ti-chi-bot commented May 7, 2024

Storages: refine `SegmentReadTaskScheduler` #8557

Storages: refine `SegmentReadTaskScheduler` #8557

JinheLin commented Dec 20, 2023 •

edited

Loading