Introduce mpp query level processListEntry by windtalker · Pull Request #7644 · pingcap/tiflash

windtalker · 2023-06-12T08:14:38Z

What problem does this PR solve?

Issue Number: close #7643

Problem Summary:

Introduce query level processListEntry for mpp query, so all the mpp tasks belong to the same mpp query will share the same processListEntry(and the same memory_tracker)
Remove switchMemTracker for local tunnel since the memory tracker is now the same in source mpp task and target mpp task.
Disable local tunnel in ut if sender task is the root task

What is changed and how it works?

Check List

Tests

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

windtalker · 2023-06-15T02:48:47Z

/run-all-tests

dbms/src/Interpreters/ProcessList.h

dbms/src/Flash/Mpp/MPPTask.cpp

windtalker · 2023-06-15T07:55:43Z

/run-all-tests

SeaRise

Others LGTM

dbms/src/Flash/Mpp/MPPTask.cpp

dbms/src/Flash/Mpp/MPPTask.h

SeaRise

lgtm

windtalker · 2023-06-16T01:57:29Z

/run-all-tests

yibin87

Others LGTM

yibin87 · 2023-06-16T02:30:09Z

dbms/src/Flash/Mpp/MPPTask.h

 private:
+    struct ProcessListEntryHolder
+    {
+        std::shared_ptr<ProcessListEntry> process_list_entry;


Might consider cache a raw pointer here to avoid get() overload? Just a suggestion.

It is only used during compile time, so I think not very necessary to cache a raw pointer.

yibin87 · 2023-06-16T02:42:46Z

dbms/src/Flash/Mpp/MPPTaskManager.cpp

+    if (!abort_reason.empty())
+        return {nullptr, abort_reason};
+    if (query_set == nullptr)
+        query_set = addMPPQueryTaskSet(query_id);


tiflash/dbms/src/Flash/Mpp/MPPTaskManager.cpp

Line 254 in 986d951

if (query_set == nullptr) /// the first one

, should this branch be removed? Since initProcessListEntry is done before registerTask

Yes, you are right, I've updated the code.

ti-chi-bot · 2023-06-16T02:48:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SeaRise, yibin87

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [SeaRise,yibin87]

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 · 2023-06-16T02:48:53Z

[LGTM Timeline notifier]

Timeline:

2023-06-15 09:56:30.346316833 +0000 UTC m=+252386.760920897: ☑️ agreed by SeaRise.
2023-06-16 02:48:52.892622828 +0000 UTC m=+313129.307226907: ☑️ agreed by yibin87.

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker · 2023-06-16T03:05:58Z

/run-all-tests

windtalker · 2023-06-16T03:27:17Z

/run-integration-test

windtalker · 2023-06-16T03:55:19Z

/run-integration-test

This reverts commit b9899e9.

ref #7687

close #7677, close #7678, close #7687

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 12, 2023

windtalker changed the title ~~Introduce query level memory_tracker for mpp query~~ WIP: Introduce query level memory_tracker for mpp query Jun 12, 2023

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

windtalker force-pushed the mpp_query_memory_tracker branch 3 times, most recently from dea695f to 4611394 Compare June 15, 2023 02:39

windtalker changed the title ~~WIP: Introduce query level memory_tracker for mpp query~~ Introduce mpp query level processListEntry Jun 15, 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 Jun 15, 2023

SeaRise reviewed Jun 15, 2023

View reviewed changes

dbms/src/Interpreters/ProcessList.h Show resolved Hide resolved

dbms/src/Flash/Mpp/MPPTask.cpp Outdated Show resolved Hide resolved

SeaRise mentioned this pull request Jun 15, 2023

Some refine about Pipeline #7646

Merged

12 tasks

SeaRise self-requested a review June 15, 2023 07:52

SeaRise reviewed Jun 15, 2023

View reviewed changes

dbms/src/Flash/Mpp/MPPTask.cpp Outdated Show resolved Hide resolved

dbms/src/Flash/Mpp/MPPTask.cpp Outdated Show resolved Hide resolved

dbms/src/Flash/Mpp/MPPTask.h Show resolved Hide resolved

windtalker force-pushed the mpp_query_memory_tracker branch from 916e3a7 to 1e93b23 Compare June 15, 2023 09:42

SeaRise approved these changes Jun 15, 2023

View reviewed changes

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

yibin87 approved these changes Jun 16, 2023

View reviewed changes

ti-chi-bot bot added the lgtm label Jun 16, 2023

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 16, 2023

windtalker added 5 commits June 16, 2023 11:04

save work

70ceab1

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

save work

ca92b97

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

save work

1690279

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

refine

8df9f47

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

save work

10d100c

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker added 8 commits June 16, 2023 11:04

save work

c008c5f

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

save work

0e5b135

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

save work

de919f6

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

save work

23c44ac

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

address comments && fix ut

0087947

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

add more comments

7f38c8e

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

address comments

2d94d75

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

address comments

5846e92

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker force-pushed the mpp_query_memory_tracker branch from 986d951 to 5846e92 Compare June 16, 2023 03:05

ti-chi-bot bot merged commit b9899e9 into pingcap:master Jun 16, 2023

This was referenced Jun 21, 2023

TiFlash crash in failpoint test #7678

Closed

Query with Apply executor may throw query set must not be null when register task error #7687

Closed

windtalker added a commit to windtalker/tiflash that referenced this pull request Jun 21, 2023

Revert "Introduce mpp query level processListEntry (pingcap#7644)"

9743726

This reverts commit b9899e9.

windtalker mentioned this pull request Jun 21, 2023

Revert "Introduce mpp query level processListEntry (#7644)" #7688

Merged

12 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Jun 21, 2023

Revert "Introduce mpp query level processListEntry (#7644)" (#7688)

a57ec6d

ref #7687

windtalker mentioned this pull request Jun 25, 2023

Fix bug introduced by #7644 #7694

Merged

12 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Jun 27, 2023

Fix bug introduced by #7644 (#7694)

c13b489

close #7677, close #7678, close #7687

windtalker deleted the mpp_query_memory_tracker branch September 20, 2023 00:26

Conversation

windtalker commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

windtalker commented Jun 15, 2023

Uh oh!

Uh oh!

Uh oh!

windtalker commented Jun 15, 2023

Uh oh!

SeaRise left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SeaRise left a comment

Choose a reason for hiding this comment

Uh oh!

windtalker commented Jun 16, 2023

Uh oh!

yibin87 left a comment

Choose a reason for hiding this comment

Uh oh!

yibin87 Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

windtalker Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

yibin87 Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

windtalker Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Jun 16, 2023

Uh oh!

ti-chi-bot bot commented Jun 16, 2023

[LGTM Timeline notifier]

Uh oh!

windtalker commented Jun 16, 2023

Uh oh!

windtalker commented Jun 16, 2023

Uh oh!

windtalker commented Jun 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

windtalker commented Jun 12, 2023 •

edited

Loading