-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
executor: vectorize hash calculation in hashJoin (#12048) #12076
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12076 +/- ##
================================================
- Coverage 81.4863% 81.4532% -0.0332%
================================================
Files 449 449
Lines 97058 96421 -637
================================================
- Hits 79089 78538 -551
+ Misses 12356 12266 -90
- Partials 5613 5617 +4 |
@sduzh Thanks! Could you show some benchmark results? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @sduzh, your pull request looks awesome, I've let some comments in there, PTAL.
In addition, you may provide a reportable benchmark differences.
We have two benchmarks about your pull request, BenchmarkHashJoinExec
and BenchmarkBuildHashTableForList
.
We could run the benchmarks by using
go test -run=^$ -bench="BenchmarkHashJoinExec|BenchmarkBuildHashTableForList" -test.benchmem -count 10
Then copying and pasting that output to two different files: old.txt and new.txt
and then runing:
benchstat old.txt new.txt
and if you need to, you can find benchstat at https://godoc.org/golang.org/x/perf/cmd/benchstat, then putting your benchstat result into your pull request description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Reminiscent @qw4990 @XuHuaiyu PTAL, thanks. |
Could you please investigate why there is performance drawback in the second case |
I ran the benchmark again and could not reproduce the result. |
LGTM |
rows := chk.NumRows() | ||
switch tp.Tp { | ||
case mysql.TypeTiny, mysql.TypeShort, mysql.TypeInt24, mysql.TypeLong, mysql.TypeLonglong, mysql.TypeYear: | ||
i64s := column.Int64s() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about:
for i := 0; i < rows; i++ {
if column.IsNull(i) {
h[i].Write(NilFlag)
continue
}
h[i].Write(column.GetRaw(i))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean no need to write the flag byte?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore the flag byte will generate a different hash value from that generated by HashChunkRow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Maybe we can optimize this in another PR:
- optimize the functions we used to calculate the hash value.
- vectorize the way to calculate hash values for the outer table when performs a hash join.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
What problem does this PR solve?
Fix issue #12048
What is changed and how it works?
benchstat result
Check List
Tests
Code changes
Side effects
Related changes
Release note