*: allow UnionBlockInputStream ignore the blocks by fuzhe1989 · Pull Request #3638 · pingcap/tiflash

fuzhe1989 · 2021-12-13T06:43:46Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

HashJoinBuildInputStream and ExchangeSender will return blocks through UnionBlockInputStream and all blocks will first contend for being pushed into a ConcurrentBoundedQueue. This can be a performance bottleneck:

Sometimes the receiver doesn't need the returned blocks: HashJoinBuildInputStream, MPPTask.
All the blocks generated from a MPP task will be transmitted to MPPTask::runImpl and then destructed, which is very unfriendly to the memory allocator (many many threads allocate and one thread deallocates).

What is changed and how it works?

Introduce a new template parameter ignore_block to UnionBlockInputStream that ignores blocks at onBlock.

To not break the test, also move some fail points from MPPTask::runImpl to StreamingDAGResponseWriter.

Candidate solution:

Introduce IgnoreBlockInputStream which simply ignores all returned blocks in a while-loop and only return an empty block when finished.

Append IgnoreBlockInputStream after HashJoinBuildInputStream and ExchangeSender since we don't need the returned blocks.

Problem: may cause UnionBlockInputStream unbalanced if the thread_num < stream_num.

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

ti-chi-bot · 2021-12-13T06:43:47Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

SeaRise

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.

Details

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

fuzhe1989 · 2021-12-13T08:44:30Z

/run-all-tests

sre-bot · 2021-12-13T10:10:57Z

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/350/cobertura/
(Coverage detail url is limited office network access)

lines: 41.9% (46383 out of 110695)
branches: 5.8% (69692 out of 1202058)

dbms/src/DataStreams/IgnoreBlockInputStream.h

dbms/src/Flash/Mpp/MPPTask.cpp

fuzhe1989 · 2021-12-13T14:26:47Z

/run-all-tests

sre-bot · 2021-12-13T14:40:33Z

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/356/cobertura/
(Coverage detail url is limited office network access)

lines: 41.9% (46383 out of 110699)
branches: 5.8% (69672 out of 1201841)

windtalker · 2021-12-16T02:03:22Z

dbms/src/Flash/Mpp/MPPTask.cpp

-                FAIL_POINT_TRIGGER_EXCEPTION(FailPoints::exception_during_mpp_non_root_task_run);
-            }
-        }
+        FAIL_POINT_PAUSE(FailPoints::hang_in_execution);


The failpoint here seems useless because once reach here, the execution is finished.

I fail to find a better place. If the failpoint is triggered in ExchangeSender, multiple exceptions will be thrown and the error message will be mismatch.

fuzhe1989 · 2021-12-16T05:41:40Z

/run-all-tests

sre-bot · 2021-12-16T05:56:12Z

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/396/cobertura/
(Coverage detail url is limited office network access)

lines: 42.3% (46961 out of 111016)
branches: 6.0% (72509 out of 1206362)

fuzhe1989 · 2021-12-16T07:51:23Z

/run-all-tests

sre-bot · 2021-12-16T08:54:02Z

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/401/cobertura/
(Coverage detail url is limited office network access)

lines: 42.4% (47048 out of 111015)
branches: 6.0% (72615 out of 1206368)

fuzhe1989 · 2021-12-16T09:32:45Z

/run-all-tests

sre-bot · 2021-12-16T10:33:40Z

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/408/cobertura/
(Coverage detail url is limited office network access)

lines: 42.4% (47051 out of 111014)
branches: 6.0% (72619 out of 1206364)

fuzhe1989 · 2021-12-16T12:01:01Z

/run-integration-test

dbms/src/DataStreams/ExchangeSender.cpp

windtalker · 2021-12-17T01:31:13Z

dbms/src/Flash/Coprocessor/StreamingDAGResponseWriter.cpp

 template <class StreamWriterPtr>
 void StreamingDAGResponseWriter<StreamWriterPtr>::write(const Block & block)
 {
+    FAIL_POINT_PAUSE(FailPoints::hang_in_execution);


I think its better to put these failpoint to ExchangeSender

windtalker · 2021-12-17T01:33:55Z

dbms/src/Flash/Mpp/MPPTask.cpp

Are you sure tracing will be GA in the next release, because if not then we will have to release a version without this information.

fuzhe1989 · 2021-12-17T02:55:29Z

/run-integration-test

windtalker

LGTM

fuzhe1989 · 2021-12-17T03:34:07Z

/merge

ti-chi-bot · 2021-12-17T03:34:08Z

@fuzhe1989: 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.

Details

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 · 2021-12-17T03:34:13Z

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

Details

Commit hash: d05bcbe

sre-bot · 2021-12-17T03:48:53Z

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/429/cobertura/
(Coverage detail url is limited office network access)

lines: 42.5% (47528 out of 111811)
branches: 6.0% (73010 out of 1208747)

sre-bot · 2021-12-17T04:06:51Z

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/430/cobertura/
(Coverage detail url is limited office network access)

lines: 42.5% (47528 out of 111811)
branches: 6.0% (73010 out of 1208747)

fuzhe1989 and others added 6 commits December 8, 2021 18:45

update

8abf7bf

update

86c59d4

fix

6731dc7

fix

a8b9cef

Merge branch 'master' into fuzhe/refine_dag_query_source

b1d8262

update

d56d1af

ti-chi-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 Dec 13, 2021

fuzhe1989 added 7 commits December 13, 2021 14:45

Merge branch 'master' into fuzhe/refine_dag_query_source

ee21d75

update

eb82956

update

2469db4

Merge branch 'master' into fuzhe/new_union_wait

57f0c1a

format

80b949c

Merge branch 'fuzhe/refine_dag_query_source' into fuzhe/new_union_wait

4d8731c

update

e8e6f5b

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 13, 2021

Merge branch 'master' into fuzhe/new_union_wait

a923640

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 13, 2021

fuzhe1989 requested review from SeaRise and windtalker December 13, 2021 08:44

fuzhe1989 changed the title *: allow UnionBlockInputStream ignore the blocks *: introduce IgnoreBlockInputStream Dec 13, 2021

SeaRise reviewed Dec 13, 2021

View reviewed changes

dbms/src/DataStreams/IgnoreBlockInputStream.h Outdated Show resolved Hide resolved

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

SeaRise approved these changes Dec 14, 2021

View reviewed changes

windtalker reviewed Dec 16, 2021

View reviewed changes

update

ed6da17

update

9c1c13c

update

4e5bf8d

fuzhe1989 requested a review from windtalker December 16, 2021 14:15

windtalker reviewed Dec 17, 2021

View reviewed changes

fuzhe1989 added 2 commits December 17, 2021 10:52

Merge branch 'master' into fuzhe/new_union_wait

0e5e0e2

update

d05bcbe

fuzhe1989 requested a review from windtalker December 17, 2021 02:57

windtalker approved these changes Dec 17, 2021

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 17, 2021

Merge branch 'master' into fuzhe/new_union_wait

0938d87

ti-chi-bot merged commit 6092626 into pingcap:master Dec 17, 2021

fuzhe1989 deleted the fuzhe/new_union_wait branch December 17, 2021 04:24

ti-chi-bot pushed a commit that referenced this pull request Dec 17, 2021

Fix ci build break introduced by #3638 (#3672)

c723d35

SeaRise mentioned this pull request Dec 19, 2021

Tracing: collect executor runtime details #3682

Merged

12 tasks

windtalker mentioned this pull request Jul 19, 2022

Log in CreatingSetsBlockInputStream always shows join build has empty result #5403

Closed

Conversation

fuzhe1989 commented Dec 13, 2021 • 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!

ti-chi-bot commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fuzhe1989 commented Dec 13, 2021

Uh oh!

sre-bot commented Dec 13, 2021

Uh oh!

Uh oh!

Uh oh!

fuzhe1989 commented Dec 13, 2021

Uh oh!

sre-bot commented Dec 13, 2021

Uh oh!

windtalker Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

fuzhe1989 Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

fuzhe1989 commented Dec 16, 2021

Uh oh!

sre-bot commented Dec 16, 2021

Uh oh!

fuzhe1989 commented Dec 16, 2021

Uh oh!

sre-bot commented Dec 16, 2021

Uh oh!

fuzhe1989 commented Dec 16, 2021

Uh oh!

sre-bot commented Dec 16, 2021

Uh oh!

fuzhe1989 commented Dec 16, 2021

Uh oh!

Uh oh!

windtalker Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

windtalker Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

fuzhe1989 commented Dec 17, 2021

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

fuzhe1989 commented Dec 17, 2021

Uh oh!

ti-chi-bot commented Dec 17, 2021

Uh oh!

ti-chi-bot commented Dec 17, 2021

Uh oh!

sre-bot commented Dec 17, 2021

Uh oh!

sre-bot commented Dec 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fuzhe1989 commented Dec 13, 2021 •

edited

Loading

ti-chi-bot commented Dec 13, 2021 •

edited

Loading