executor: refine hash join v2 for spill #55790

xzhangxian1008 · 2024-09-02T06:10:42Z

What problem does this PR solve?

Issue Number: ref #55153

Problem Summary:

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

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

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

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

Signed-off-by: xufei <xufei@pingcap.com>

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

Signed-off-by: xufei <xufei@pingcap.com>

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

ti-chi-bot · 2024-09-02T06:10:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign myonkeminta for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

tiprow · 2024-09-02T06:10:57Z

Hi @xzhangxian1008. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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 kubernetes-sigs/prow repository.

codecov · 2024-09-02T06:33:49Z

Codecov Report

Attention: Patch coverage is 43.01733% with 559 lines in your changes missing coverage. Please review.

Project coverage is 73.4534%. Comparing base (3419bde) to head (75127c7).
Report is 62 commits behind head on master.

❗ Current head 75127c7 differs from pull request most recent head ab22f9b

Please upload reports for the commit ab22f9b to get more accurate results.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #55790        +/-   ##
================================================
+ Coverage   73.0085%   73.4534%   +0.4449%     
================================================
  Files          1584       1600        +16     
  Lines        443036     459190     +16154     
================================================
+ Hits         323454     337291     +13837     
- Misses        99870     101902      +2032     
- Partials      19712      19997       +285

Flag	Coverage Δ
integration	`44.0584% <43.0173%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9567% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`45.0436% <ø> (-0.4146%)`	⬇️

xzhangxian1008 · 2024-09-03T01:47:29Z

/cc @windtalker @XuHuaiyu

pkg/executor/join/hash_join_base.go

windtalker · 2024-09-03T02:29:56Z

pkg/executor/join/hash_join_base.go

 		select {
 		case <-doneCh:
+			syncerDone(fetcherAndWorkerSyncer)


why not put syncerDone as defer function?

windtalker · 2024-09-03T02:52:16Z

pkg/executor/join/hash_join_probe_worker_v2.go

+		return nil
+	} else if joinResult.err != nil || (joinResult.chk != nil && joinResult.chk.NumRows() > 0) {
+		w.HashJoinCtx.joinResultCh <- joinResult
+	} else if joinResult.chk != nil && joinResult.chk.NumRows() == 0 {


when will this happen?

when will this happen?

This logic is from the origin hash join. Maybe this happens when there are no rows be successfully joined.

windtalker · 2024-09-03T03:30:17Z

pkg/executor/join/hash_join_v2.go

 	builder.rowNumberInCurrentRowTableSeg[partitionID] = 0
 	failpoint.Inject("finalizeCurrentSegPanic", nil)
 	seg.finalized = true
-	htc.memoryTracker.Consume(seg.totalUsedBytes())
+	if needConsume {


when will needConsume be false?

when will needConsume be false?

When spill is triggered, needConsume will be false.

windtalker · 2024-09-03T03:34:55Z

pkg/executor/join/hash_join_v2.go

-	htc.rowTables = nil
-	return totalSegmentCnt
+
+	htc.clearAllSegmentsInRowTable()


why keep htc.rowTables

why keep htc.rowTables

Because htc.rowTables may be used in next restore round.

windtalker · 2024-09-03T03:37:09Z

pkg/executor/join/hash_join_v2.go

@@ -301,6 +299,7 @@ func (e *HashJoinV2Exec) Open(ctx context.Context) error {
 		e.prepared = false
 		return err
 	}
+	e.isMemoryClearedForTest = true


why set a test variable to true by default?

why set a test variable to true by default?

Each time finishing a restore round, we will check if all memory has been cleared. Only when there is memory not clear then this variable will be set to false. So we need to set it to true at the beginning.

windtalker · 2024-09-03T09:15:03Z

pkg/executor/join/hash_join_v2.go

-func (e *HashJoinV2Exec) waitJoinWorkersAndCloseResultChan() {
-	e.workerWg.Wait()
+// finaWorker is responsible for scanning the row table after probe done and wake up the build fetcher
+func (e *HashJoinV2Exec) finalWorker(syncer chan struct{}) {


I think if the join don't need to scan hash table after probe, there is no need to start final worker?

I think if the join don't need to scan hash table after probe, there is no need to start final worker?

Yes, but final worker will not create more go routines when there is no need to scan hash table.

windtalker · 2024-09-03T09:32:31Z

pkg/executor/join/hash_join_v2.go

@@ -588,40 +524,31 @@ func (w *ProbeWorkerV2) getNewJoinResult() (bool, *hashjoinWorkerResult) {
 func (e *HashJoinV2Exec) Next(ctx context.Context, req *chunk.Chunk) (err error) {
 	if !e.prepared {
 		e.initHashTableContext()
+		e.initializeForProbe()


why move e.initializeForProbe() out from e.fetchAndProbeHashTable()?

why move e.initializeForProbe() out from e.fetchAndProbeHashTable()?

Because joinResultCh in e.initializeForProbe() function needs to be initialized before starting build fetcher, as joinResultCh is also used for transmitting error.

windtalker · 2024-09-03T09:34:01Z

pkg/executor/join/hash_join_v2.go

 	req.Reset()

 	result, ok := <-e.joinResultCh
+	e.recycleChunk(req, result)


why move this recycle code before error check?

why move this recycle code before error check?

If error happens, we will directly return and fail to recycle chunk, this will let workers hang.

windtalker · 2024-09-03T09:40:34Z

pkg/executor/join/hash_join_v2.go

-			e.buildFinished <- err
-			return false
+	for ifContinue {
+		ifContinue, err = e.dispatchBuildTasksImpl(syncer)


dispatchBuildTasksImpl can be called multiple times here?

dispatchBuildTasksImpl can be called multiple times here?

Yes, after spill is added, this function will be called multiple times/

windtalker · 2024-09-03T09:44:21Z

pkg/executor/sortexec/sort_spill.go

@@ -114,12 +114,13 @@ func (s *parallelSortSpillAction) actionImpl(t *memory.Tracker) bool {
 		s.spillHelper.cond.Wait()
 	}

+	// we may be in `needSpill` status, so it's necessary check if we are in `notSpilled` status


unrelated changes?

unrelated changes?

Yes, it's unrelated with hash join refine. But I find that we can refine the comment in sort spill, so I put it in this pr.

windtalker · 2024-09-04T02:04:47Z

pkg/executor/join/outer_join_probe.go

-			j.appendBuildRowToCachedBuildRowsAndConstructBuildRowsIfNeeded(createMatchRowInfo(0, currentRow), joinResult.chk, 0, false)
-			insertedRows++
-		}
+		j.appendBuildRow(meta, joinResult.chk, j.rowIter.getValue(), &insertedRows)


why change this

why change this

I think it's more readable to put these codes into a function and reader could know the purpose of these codes from function name.

windtalker · 2024-09-04T02:14:15Z

pkg/executor/join/hash_join_v2.go

+		// We must ensure that all prebuild workers have exited before
+		// set `finished` flag and close buildFetcherAndDispatcherSyncChan
+		fetcherAndWorkerSyncer.Wait()
+		e.finished.Store(true)


It means this function need to wait all the probe finishes?

It means this function need to wait all the probe finishes?

No, build fetcher could directly exit without waiting all probe finished.

ti-chi-bot · 2024-09-04T08:52:36Z

@xzhangxian1008: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-integration-ddl-test	`ab22f9b`	link	true	`/test pull-integration-ddl-test`
idc-jenkins-ci-tidb/build	`ab22f9b`	link	true	`/test build`
idc-jenkins-ci-tidb/check_dev	`ab22f9b`	link	true	`/test check-dev`
pull-mysql-client-test	`ab22f9b`	link	true	`/test pull-mysql-client-test`
idc-jenkins-ci-tidb/unit-test	`ab22f9b`	link	true	`/test unit-test`
idc-jenkins-ci-tidb/mysql-test	`ab22f9b`	link	true	`/test mysql-test`
idc-jenkins-ci-tidb/check_dev_2	`ab22f9b`	link	true	`/test check-dev2`

Full PR test history. Your PR dashboard.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

ti-chi-bot · 2024-09-06T08:35:56Z

PR needs rebase.

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 kubernetes-sigs/prow repository.

windtalker and others added 30 commits June 13, 2024 18:01

save work

3f96142

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

save work

100c9cf

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

save work

ae0d5fa

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

save work

dedb368

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

refine

114b433

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

save work

8467359

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

save work

15ba18d

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

save work

4e99741

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

revert useless change

6038218

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

add some bench test for debug

c6e89f9

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

refine && save work

281ee7f

Signed-off-by: xufei <xufei@pingcap.com>

save work

be5ff03

Signed-off-by: xufei <xufei@pingcap.com>

save work

5baf4ec

Signed-off-by: xufei <xufei@pingcap.com>

save work for pre-refine of move partitioned_join to executor

3bb9b7a

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

save work for builder

ccb7bb3

Signed-off-by: xufei <xufei@pingcap.com>

finish builder

d2138ea

Signed-off-by: xufei <xufei@pingcap.com>

save work

fb091ea

Signed-off-by: xufei <xufei@pingcap.com>

save work

02dbe09

Signed-off-by: xufei <xufei@pingcap.com>

save work

69872d2

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

add col_to_row

23d925b

rename scanHT to scanRowTable

25aa633

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

fix scanRowTable in outer join prober

a0ca8fb

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

some refine for scanRowTable

1921591

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

add a session variable to choose hash join impl

5bfc0d1

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

refine of rowIter

c6a2abb

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

save work

df3a14c

save work

6aeaec6

save work

a04ebab

inline bin string for key

9efdca6

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

simplify join hash table

216c5d4

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

xzhangxian1008 added 7 commits August 21, 2024 15:19

udpate bazel

b3e7eb0

tweaking

9402d58

update statics, add tests

e4a0ffe

tweaking

9e8a3b0

Merge branch 'master' into hash-join-spill

0f3e8d7

remove spill codes

e7a414b

remove more

3e225c0

ti-chi-bot bot added 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 Sep 2, 2024

xzhangxian1008 changed the title ~~executor: refine hashv2 join for spill~~ executor: refine hash join v2 for spill Sep 2, 2024

remove some codes

9493cc2

ti-chi-bot bot requested review from windtalker and XuHuaiyu September 3, 2024 01:47

refine

bb2bb77

windtalker reviewed Sep 3, 2024

View reviewed changes

xzhangxian1008 added 2 commits September 3, 2024 11:52

remove buffer in closeCh

1f5e615

remove

5f0f5d9

windtalker reviewed Sep 3, 2024

View reviewed changes

windtalker reviewed Sep 4, 2024

View reviewed changes

xzhangxian1008 added 2 commits September 4, 2024 14:40

tweaking

75127c7

remove useless variable

ab22f9b

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

xzhangxian1008 closed this Sep 9, 2024

xzhangxian1008 deleted the refine-hash-join branch December 4, 2024 08:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: refine hash join v2 for spill #55790

executor: refine hash join v2 for spill #55790

xzhangxian1008 commented Sep 2, 2024

ti-chi-bot bot commented Sep 2, 2024

tiprow bot commented Sep 2, 2024

codecov bot commented Sep 2, 2024 •

edited

Loading

xzhangxian1008 commented Sep 3, 2024

windtalker Sep 3, 2024

windtalker Sep 3, 2024

xzhangxian1008 Sep 4, 2024

windtalker Sep 3, 2024

xzhangxian1008 Sep 4, 2024

windtalker Sep 3, 2024

xzhangxian1008 Sep 4, 2024

windtalker Sep 3, 2024

xzhangxian1008 Sep 4, 2024

windtalker Sep 3, 2024

xzhangxian1008 Sep 4, 2024

windtalker Sep 3, 2024

xzhangxian1008 Sep 4, 2024

windtalker Sep 3, 2024

xzhangxian1008 Sep 4, 2024

windtalker Sep 3, 2024

xzhangxian1008 Sep 4, 2024

windtalker Sep 3, 2024

xzhangxian1008 Sep 4, 2024

windtalker Sep 4, 2024

xzhangxian1008 Sep 4, 2024

windtalker Sep 4, 2024

xzhangxian1008 Sep 4, 2024

ti-chi-bot bot commented Sep 4, 2024

ti-chi-bot bot commented Sep 6, 2024

executor: refine hash join v2 for spill #55790

executor: refine hash join v2 for spill #55790

Conversation

xzhangxian1008 commented Sep 2, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Sep 2, 2024

tiprow bot commented Sep 2, 2024

codecov bot commented Sep 2, 2024 • edited Loading

Codecov Report

xzhangxian1008 commented Sep 3, 2024

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

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

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

ti-chi-bot bot commented Sep 4, 2024

ti-chi-bot bot commented Sep 6, 2024

codecov bot commented Sep 2, 2024 •

edited

Loading