Skip to content
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

expression: make builtinCastIntAsInt support vectorized evaluation #11826

Merged
merged 11 commits into from
Aug 28, 2019

Conversation

qw4990
Copy link
Contributor

@qw4990 qw4990 commented Aug 22, 2019

What problem does this PR solve?

Implement vectorized evaluation for builtinCastIntAsInt.

What is changed and how it works?

  1. Implement vectorized evaluation.
  2. Add some tests.

Here is the benchmark in my MAC, almost 95x faster than before:

BenchmarkCastIntAsCastRow-12         200000              8542 ns/op               0 B/op          0 allocs/op
BenchmarkCastIntAsCastVec-12       20000000                90.9 ns/op             0 B/op          0 allocs/op

Check List

Tests

  • Unit test

@codecov
Copy link

codecov bot commented Aug 22, 2019

Codecov Report

Merging #11826 into master will decrease coverage by 0.0632%.
The diff coverage is 73.3333%.

@@               Coverage Diff                @@
##             master     #11826        +/-   ##
================================================
- Coverage   81.3093%   81.2461%   -0.0633%     
================================================
  Files           438        438                
  Lines         94566      94455       -111     
================================================
- Hits          76891      76741       -150     
- Misses        12208      12238        +30     
- Partials       5467       5476         +9

Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@qw4990 qw4990 added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 23, 2019
@qw4990
Copy link
Contributor Author

qw4990 commented Aug 23, 2019

/run-all-tests

@qw4990
Copy link
Contributor Author

qw4990 commented Aug 23, 2019

/run-common-test

XuHuaiyu
XuHuaiyu previously approved these changes Aug 23, 2019
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@qw4990
Copy link
Contributor Author

qw4990 commented Aug 23, 2019

/run-all-tests

@XuHuaiyu XuHuaiyu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 23, 2019
@qw4990
Copy link
Contributor Author

qw4990 commented Aug 23, 2019

/run-common-test

@qw4990
Copy link
Contributor Author

qw4990 commented Aug 23, 2019

/run-all-tests

@qw4990
Copy link
Contributor Author

qw4990 commented Aug 23, 2019

/run-all-tests

@qw4990
Copy link
Contributor Author

qw4990 commented Aug 23, 2019

/run-common-test

expression/builtin_cast.go Show resolved Hide resolved
expression/builtin_vectorized.go Outdated Show resolved Hide resolved
expression/builtin_vectorized.go Outdated Show resolved Hide resolved
expression/builtin_cast_bench_test.go Outdated Show resolved Hide resolved
"github.com/pingcap/tidb/util/mock"
)

func BenchmarkCastIntAsInt(b *testing.B) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you post the benchmark code on the old cast implementation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to use https://godoc.org/golang.org/x/perf/cmd/benchstat and post the benchmark result in the pull request description

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qw4990
Copy link
Contributor Author

qw4990 commented Aug 27, 2019

/run-all-tests

@qw4990 qw4990 removed the status/WIP label Aug 28, 2019
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zz-jason zz-jason added the status/can-merge Indicates a PR has been approved by a committer. label Aug 28, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 28, 2019

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants