Skip to content

expression: implement vectorized evaluation for builtinRoundWithFracDecSig#12252

Merged
sre-bot merged 8 commits intopingcap:masterfrom
FateTHarlaown:builtin
Sep 26, 2019
Merged

expression: implement vectorized evaluation for builtinRoundWithFracDecSig#12252
sre-bot merged 8 commits intopingcap:masterfrom
FateTHarlaown:builtin

Conversation

@FateTHarlaown
Copy link
Contributor

@FateTHarlaown FateTHarlaown commented Sep 18, 2019

What problem does this PR solve?

implement vectorized evaluation for builtinRoundWithFracDecSig, for #12102

What is changed and how it works?

about 3 times faster than before

goos: darwin
goarch: amd64
pkg: github.com/pingcap/tidb/expression
BenchmarkVectorizedBuiltinMathFunc/builtinRoundWithFracDecSig-VecBuiltinFunc-8         	  100000	     15380 ns/op	       0 B/op	       0 allocs/op
BenchmarkVectorizedBuiltinMathFunc/builtinRoundWithFracDecSig-NonVecBuiltinFunc-8      	   30000	     48793 ns/op	   32112 B/op	     669 allocs/op
PASS
ok  	github.com/pingcap/tidb/expression	3.709s

Check List

Tests

  • Unit test

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

codecov bot commented Sep 18, 2019 •
edited
Loading

Codecov Report

Merging #12252 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12252   +/-   ##
===========================================
  Coverage   79.7594%   79.7594%           
===========================================
  Files           462        462           
  Lines        101865     101865           
===========================================
  Hits          81247      81247           
  Misses        14757      14757           
  Partials       5861       5861

Reminiscent
Reminiscent reviewed Sep 18, 2019
View reviewed changes
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 18, 2019
SunRunAway
SunRunAway previously approved these changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@SunRunAway SunRunAway left a comment •
edited
Loading

Choose a reason for hiding this comment

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

~

@SunRunAway SunRunAway 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 18, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 18, 2019

/run-all-tests

SunRunAway
SunRunAway reviewed Sep 18, 2019
View reviewed changes
@SunRunAway SunRunAway added status/LGT1 Indicates that a PR has LGTM 1. and removed status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. labels Sep 18, 2019
qw4990
qw4990 reviewed Sep 18, 2019
View reviewed changes
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

@zz-jason
Copy link
Member

zz-jason commented Sep 23, 2019

@FateTHarlaown Please update master branch and resolve conflicts.

SunRunAway
SunRunAway approved these changes Sep 26, 2019
View reviewed changes
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

@SunRunAway SunRunAway 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 26, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 26, 2019

Your auto merge job has been accepted, waiting for 11951

@sre-bot
Copy link
Contributor

sre-bot commented Sep 26, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Sep 26, 2019

@FateTHarlaown merge failed.

@SunRunAway
Copy link
Contributor

SunRunAway commented Sep 26, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Sep 26, 2019

/run-all-tests

@sre-bot sre-bot merged commit 97f679e into pingcap:master Sep 26, 2019
@FateTHarlaown FateTHarlaown deleted the builtin branch November 6, 2019 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qw4990 qw4990 qw4990 left review comments

+2 more reviewers

@Reminiscent Reminiscent Reminiscent left review comments

@SunRunAway SunRunAway SunRunAway approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@FateTHarlaown @sre-bot @zz-jason @SunRunAway @qw4990 @Reminiscent