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.

Sign up for GitHub

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

Jump to bottom

expression: implement vectorized evaluation for builtinLogicOrSig #12365

Merged
merged 7 commits into from
Sep 26, 2019
Merged

expression: implement vectorized evaluation for builtinLogicOrSig #12365

merged 7 commits into from
Sep 26, 2019

Conversation

k-ye
Copy link
Contributor

@k-ye k-ye commented Sep 24, 2019

What problem does this PR solve?

Implement vectorized evaluation for builtinLogicOrSig for #12103.

What is changed and how it works?

According to benchmark, about 4 times faster than before:

BenchmarkVectorizedBuiltinOpFunc/builtinLogicOrSig-VecBuiltinFunc-4         	  300000	      5876 ns/op	       0 B/op	       0 allocs/op
BenchmarkVectorizedBuiltinOpFunc/builtinLogicOrSig-NonVecBuiltinFunc-4      	   50000	     26135 ns/op	       0 B/op	       0 allocs/op

Check List

Tests

  • Unit test

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Sep 24, 2019
Reminiscent
Reminiscent reviewed Sep 25, 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
Copy link
Contributor

Reminiscent commented Sep 25, 2019 •
edited
Loading

Thanks! @k-ye Would you like to join our channel #wg-vec-expr in the tidbcommunity slacks workspace for more discussion?

@k-ye
Copy link
Contributor Author

k-ye commented Sep 25, 2019

Thanks! @k-ye Would you like to join our channel #wg-vec-expr in the tidbcommunity slacks workspace for more discussion?

@Reminiscent Sounds great! I've joined now 😃

@qw4990
Copy link
Contributor

qw4990 commented Sep 25, 2019

@k-ye Almost LGTM, but please solve the conflict.

@k-ye
Copy link
Contributor Author

k-ye commented Sep 25, 2019

@k-ye Almost LGTM, but please solve the conflict.

@qw4990 Thanks! It's resolved now.

@k-ye
Copy link
Contributor Author

k-ye commented Sep 25, 2019

Since the logical AND operator has been committed, I think the customized data generator could be applied to it as well, and thus added f6fb834. Please let me know if you think this is necessary, or we should do it in a separate PR, thx!

@codecov
Copy link

codecov bot commented Sep 25, 2019 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12365   +/-   ##
===========================================
  Coverage   79.7499%   79.7499%           
===========================================
  Files           462        462           
  Lines        101960     101960           
===========================================
  Hits          81313      81313           
  Misses        14789      14789           
  Partials       5858       5858

qw4990
qw4990 approved these changes Sep 26, 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

@qw4990 qw4990 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Sep 26, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 26, 2019

Your auto merge job has been accepted, waiting for 12399

@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

@k-ye merge failed.

@qw4990
Copy link
Contributor

qw4990 commented Sep 26, 2019

/run-all-tests

@qw4990
Copy link
Contributor

qw4990 commented Sep 26, 2019

/run-unit-test

@ngaut ngaut merged commit de2df75 into pingcap:master Sep 26, 2019
@k-ye k-ye deleted the vec branch September 26, 2019 10:50
SunRunAway
SunRunAway reviewed Oct 16, 2019
View reviewed changes
expression/builtin_op_vec.go
} else {
i64s[i] = 0
}
result.SetNull(i, isNull)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about do an extra optimization here,

if isNull0 != isNull {
    result.SetNull(i, isNull)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, please see #12773

@k-ye k-ye mentioned this pull request Oct 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SunRunAway SunRunAway SunRunAway left review comments

@Reminiscent Reminiscent Reminiscent left review comments

@qw4990 qw4990 qw4990 approved these changes

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
@k-ye @Reminiscent @qw4990 @sre-bot @SunRunAway @ngaut