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: implement vectorized evaluation for builtinCastDurationAsRealSig #13475

Merged
merged 5 commits into from
Nov 19, 2019

Conversation

pingyu
Copy link
Contributor

@pingyu pingyu commented Nov 14, 2019

PCP #12105

What problem does this PR solve?

Implement vectorized evaluation for builtinCastDurationAsRealSig at #12105

What is changed and how it works?

It gets faster about 1.2%.
Duration.ToNumber() is modified, just as what is done in CheckFsp() from fsp.go. Otherwise builtinCastDurationAsRealSig.evalReal() in builtin_cast.go will raise a runtime error: slice bounds out of range [:-1] in unittest of builtinCastDurationAsRealSig.vecEvalReal().

Check List

Tests

  • Unit test
> GO111MODULE=on go test -check.f TestVectorizedBuiltinCastFunc
PASS: builtin_cast_vec_test.go:130: testEvaluatorSuite.TestVectorizedBuiltinCastFunc	0.134s
OK: 1 passed
PASS
ok  	github.com/pingcap/tidb/expression	0.158s
  • Benchmark
> go test -v -benchmem -bench=BenchmarkVectorizedBuiltinCastFunc -run=BenchmarkVectorizedBuiltinCastFunc -args builtinCastDurationAsRealSig
goos: darwin
goarch: amd64
pkg: github.com/pingcap/tidb/expression
BenchmarkVectorizedBuiltinCastFunc/builtinCastDurationAsRealSig-VecBuiltinFunc-12         	    4402	    255587 ns/op	   50713 B/op	    3669 allocs/op
BenchmarkVectorizedBuiltinCastFunc/builtinCastDurationAsRealSig-NonVecBuiltinFunc-12      	    4347	    277312 ns/op	   50712 B/op	    3669 allocs/op
PASS
ok  	github.com/pingcap/tidb/expression	2.430s

@pingyu pingyu requested a review from a team as a code owner November 14, 2019 19:05
@sre-bot
Copy link
Contributor

sre-bot commented Nov 14, 2019

Thanks for your contribution. If your PR get merged, you will be rewarded 50 points.

@ghost ghost requested review from wshwsh12 and XuHuaiyu November 14, 2019 19:05
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Nov 14, 2019
@ghost ghost removed their request for review November 14, 2019 19:05
@pingyu pingyu force-pushed the expression_vec_cast_duration_as_real branch from 85089de to 1227900 Compare November 14, 2019 19:20
types/time.go Outdated Show resolved Hide resolved
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

@wshwsh12 wshwsh12 added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 17, 2019
@pingyu pingyu force-pushed the expression_vec_cast_duration_as_real branch from 89966f1 to e311a3c Compare November 18, 2019 15:22
@codecov
Copy link

codecov bot commented Nov 18, 2019

Codecov Report

Merging #13475 into master will increase coverage by 0.0462%.
The diff coverage is 61.5384%.

@@               Coverage Diff                @@
##             master     #13475        +/-   ##
================================================
+ Coverage   80.2004%   80.2467%   +0.0463%     
================================================
  Files           472        472                
  Lines        114866     114791        -75     
================================================
- Hits          92123      92116         -7     
+ Misses        15495      15449        -46     
+ Partials       7248       7226        -22

@qw4990
Copy link
Contributor

qw4990 commented Nov 19, 2019

/run-all-tests

@qw4990
Copy link
Contributor

qw4990 commented Nov 19, 2019

/rebuild

Copy link
Contributor

@qw4990 qw4990 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 merged commit 7f02792 into pingcap:master Nov 19, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 19, 2019

@pingyu complete task #12105 and get 50 score, currerent score 250.

@qw4990 qw4990 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 19, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants