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 builtinDurationDurationTimeDiffSig #12838

Closed
wants to merge 10 commits into from

Conversation

mmyj
Copy link
Member

@mmyj mmyj commented Oct 20, 2019

What problem does this PR solve?

implement vectorized evaluation for builtinDurationDurationTimeDiffSig, for #12176

What is changed and how it works?

go test -v -benchmem -bench=BenchmarkVectorizedBuiltinTimeFunc -run=BenchmarkVectorizedBuiltinTimeFunc -args 'builtinDurationDurationTimeDiffSig'
goos: linux
goarch: amd64
pkg: github.com/pingcap/tidb/expression
BenchmarkVectorizedBuiltinTimeFuncGenerated-4   	1000000000	         0.00908 ns/op	       0 B/op	       0 allocs/op
BenchmarkVectorizedBuiltinTimeFunc/builtinDurationDurationTimeDiffSig-VecBuiltinFunc-4         	   64982	     17922 ns/op	       0 B/op	       0 allocs/op
BenchmarkVectorizedBuiltinTimeFunc/builtinDurationDurationTimeDiffSig-NonVecBuiltinFunc-4      	   31800	     37973 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/pingcap/tidb/expression	3.029s

Check List

Tests

  • Unit test

@mmyj mmyj requested a review from a team as a code owner October 20, 2019 12:17
@ghost ghost requested a review from XuHuaiyu October 20, 2019 12:17
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Oct 20, 2019
@ghost ghost removed their request for review October 20, 2019 12:17
@codecov
Copy link

codecov bot commented Oct 20, 2019

Codecov Report

Merging #12838 into master will decrease coverage by 0.2078%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #12838        +/-   ##
================================================
- Coverage   80.1069%   79.8991%   -0.2079%     
================================================
  Files           465        465                
  Lines        107520     107632       +112     
================================================
- Hits          86131      85997       -134     
- Misses        14909      15142       +233     
- Partials       6480       6493        +13

@mmyj
Copy link
Member Author

mmyj commented Oct 22, 2019

/run-all-tests

@zz-jason
Copy link
Member

/run-all-tests

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

@XuHuaiyu XuHuaiyu added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 23, 2019
@zz-jason
Copy link
Member

/run-all-tests

@mmyj
Copy link
Member Author

mmyj commented Oct 27, 2019

@Reminiscent PTAL

return err
}
defer b.bufAllocator.put(buf1)
if err := b.args[1].VecEvalDuration(b.ctx, input, buf1); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

we can use result to store one of the parameters, which helps us to reduce one memory allocation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will fix it.

continue
}
lhs := types.Duration{Duration: d64s[i]}
rhs := types.Duration{Duration: d64s1[i]}
Copy link
Member

Choose a reason for hiding this comment

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

we can define lhs and rhs outside the for loop, which saves two allocation for each loop iteration. It would improve the performance IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will fix it.

@mmyj mmyj closed this Oct 31, 2019
@mmyj mmyj deleted the builtinDurationDurationTimeDiffSig branch November 14, 2019 14:34
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/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants