Skip to content

implement vectorized evaluation for builtinSqrtSig#12187

Merged
qw4990 merged 1 commit intopingcap:masterfrom
jacklightChen:sqrt
Sep 16, 2019
Merged

implement vectorized evaluation for builtinSqrtSig#12187
qw4990 merged 1 commit intopingcap:masterfrom
jacklightChen:sqrt

Conversation

@jacklightChen
Copy link
Contributor

What problem does this PR solve?

implement vectorized evaluation for builtinSqrtSig, for #12105

What is changed and how it works?

according to benchmark, about 5 times faster than before:

BenchmarkVectorizedBuiltinMathFunc/builtinSqrtSig-VecBuiltinFunc-8                500000              2444 ns/op            0 B/op          0 allocs/op
BenchmarkVectorizedBuiltinMathFunc/builtinSqrtSig-NonVecBuiltinFunc-8             100000             13055 ns/op            0 B/op          0 allocs/op

ps:if we don't check result.IsNull(i)in the loop,the method is only 3 times faster than before, so it's better to add those check nulls codes:

BenchmarkVectorizedBuiltinMathFunc/builtinSqrtSig-VecBuiltinFunc-8                300000              4281 ns/op            0 B/op          0 allocs/op
BenchmarkVectorizedBuiltinMathFunc/builtinSqrtSig-NonVecBuiltinFunc-8             100000             14611 ns/op            0 B/op          0 allocs/op

Check List

Tests

  • Unit test

@CLAassistant
Copy link

CLAassistant commented Sep 15, 2019

CLA assistant check
All committers have signed the CLA.

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Sep 15, 2019
@codecov
Copy link

codecov bot commented Sep 15, 2019

Codecov Report

Merging #12187 into master will decrease coverage by 0.3958%.
The diff coverage is 71.4285%.

@@               Coverage Diff               @@
##            master     #12187        +/-   ##
===============================================
- Coverage   81.579%   81.1832%   -0.3959%     
===============================================
  Files          454        454                
  Lines        99425      97987      -1438     
===============================================
- Hits         81110      79549      -1561     
- Misses       12607      12712       +105     
- Partials      5708       5726        +18

@francis0407 francis0407 requested review from Reminiscent and qw4990 and removed request for qw4990 September 15, 2019 11:38
@francis0407 francis0407 requested a review from qw4990 September 15, 2019 11:40
Copy link
Contributor

@Reminiscent Reminiscent left a comment

Choose a reason for hiding this comment

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

LGTM

@Reminiscent Reminiscent added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 16, 2019
@Reminiscent
Copy link
Contributor

Thanks! @jacklightChen Please update your branch to the latest version and try it again.

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 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 16, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 16, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Sep 16, 2019

@jacklightChen merge failed.

@qw4990 qw4990 merged commit 5a543a5 into pingcap:master Sep 16, 2019
@jacklightChen jacklightChen deleted the sqrt branch September 16, 2019 04:05
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/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