executor: parallel read inner table and build hash table. #7544
Conversation
crazycs520
added
type/enhancement
|
@XuHuaiyu PTAL |
|
/run-all-tests |
executor/join.go
Outdated
| chk := e.innerResult.GetChunk(i) | ||
| for j := 0; j < chk.NumRows(); j++ { | ||
|
|
||
| numChks := 0 |
There was a problem hiding this comment.
s/ numChks/ chkIdx := uint32(0)
executor/join.go
Outdated
| numChks := 0 | ||
| for chk := range chkCh { | ||
| numRows := chk.NumRows() | ||
| for j := 0; j < numRows; j++ { |
There was a problem hiding this comment.
func BenchmarkFor1(b *testing.B) {
chk := getChks()
for i := 0; i < b.N; i++ {
for j := 0; j < chk.NumRows(); j++ {
_ = j
}
}
}
func BenchmarkFor2(b *testing.B) {
chk := getChks()
for i := 0; i < b.N; i++ {
numRows := chk.NumRows()
for j := 0; j < numRows; j++ {
_ = j
}
}
}chk.NumRows is 1024.
BenchmarkFor1 5000000 1115 ns/op
BenchmarkFor1 5000000 1119 ns/op
BenchmarkFor1 5000000 1106 ns/op
BenchmarkFor2 10000000 557 ns/op
BenchmarkFor2 10000000 577 ns/op
BenchmarkFor2 10000000 587 ns/op
so, I think the original is better.
executor/join.go
Outdated
| @@ -495,6 +515,8 @@ func (e *HashJoinExec) Next(ctx context.Context, chk *chunk.Chunk) (err error) { | |||
| } | |||
|
|
|||
| func (e *HashJoinExec) fetchInnerAndBuildHashTable(ctx context.Context) { | |||
| chkCh := make(chan *chunk.Chunk, e.concurrency) | |||
There was a problem hiding this comment.
- move this two definition to after the defer func.
- add a comment for chkCh. (s/ chkCh/ innerResultCh ?)
executor/join.go
Outdated
| e.innerFinished <- errors.Trace(err) | ||
| return | ||
| close(doneCh) | ||
| // Func fetchInnerRows may blocked by this channel, so read from the channel to unblock it. |
There was a problem hiding this comment.
- remove
Func - s/ blocked/ be blocked
|
@crazycs520 Could you post some performance result? |
|
| // innerResultCh transfer inner result chunk from inner fetch to build hash table. | ||
| innerResultCh := make(chan *chunk.Chunk, e.concurrency) | ||
| doneCh := make(chan struct{}) | ||
| go e.fetchInnerRows(ctx, innerResultCh, doneCh) |
There was a problem hiding this comment.
how about don't always fork new groutine in here and keep fetchInnerRows logic in "primary" goroutine, and base on fetch result to choose whether or not to fork new groutine to buildHashTableForList.(e.g. Next 2 times but still has data?)
IMHO, The idea is that maybe common small innser case it's too heavy to fork goroutine but some case fork is worth, and fetch with single goutine is required, so make choosen in buildTable?
There was a problem hiding this comment.
@crazycs520 We can bench the scenario that the inner child for the hash join operator is a simple table reader and the number of output records of that child is very small, for example, 1.
There was a problem hiding this comment.
@lysu @zz-jason here is a benchmark:
select t1.* from t1 inner join tid1 where t1.id=tid1.id and tid1.id < 1;
table t1 and tid1 both have 10 rows. and the join result is 1 row;
master:
goos: linux
goarch: amd64
pkg: sql_script
BenchmarkSelect 5000 3474578 ns/op
PASS
ok sql_script 17.852sthis branch
goos: linux
goarch: amd64
pkg: sql_script
BenchmarkSelect 5000 3486767 ns/op
PASS
ok sql_script 17.904s
|
|
||
| if e.finished.Load().(bool) { | ||
| return | ||
| } | ||
|
|
||
| if err := e.buildHashTableForList(); err != nil { | ||
| // TODO: Parallel build hash table. Currently not support because `mvmap` is not thread-safe. |
There was a problem hiding this comment.
maybe this TODO can be removed? @XuHuaiyu how do you think?
There was a problem hiding this comment.
I think it's both ok to keep it or remove it.
|
LGTM |
|
@XuHuaiyu PTAL |
zz-jason
added
sig/execution
|
/run-all-tests |
|
|
||
| if e.finished.Load().(bool) { | ||
| return | ||
| } | ||
|
|
||
| if err := e.buildHashTableForList(); err != nil { | ||
| // TODO: Parallel build hash table. Currently not support because `mvmap` is not thread-safe. |
There was a problem hiding this comment.
I think it's both ok to keep it or remove it.
zz-jason
added
status/LGT2
What problem does this PR solve?
Currently in hash join, TiDB will read inner table first, then build hash table.
This PR will parallel read inner table and build hash table.
What is changed and how it works?
Check List