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

executor: add vectorized access methods for `time.Duration` stored in `Column` #11677

Merged
merged 1 commit into from Aug 8, 2019

Conversation

@qw4990
Copy link
Contributor

commented Aug 8, 2019

What problem does this PR solve?

Add some vectorized access methods for Duration which allows users to manipulate Duration data on a slice []go.Duration.

What is changed and how it works?

The new method is 3x faster than before:

BenchmarkDurationRow-12           100000             17567 ns/op               0 B/op          0 allocs/op
BenchmarkDurationVec-12           300000              5437 ns/op               0 B/op          0 allocs/op

Check List

Tests

  • Unit test
for i := 0; i < 1024; i++ {
d1 := types.Duration{Duration: ds1[i]}
d2 := types.Duration{Duration: ds2[i]}
r, err := d1.Add(d2)

This comment has been minimized.

Copy link
@qw4990

qw4990 Aug 8, 2019

Author Contributor

We can optimize the way to calculate duration variables later.

@coocood

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

LGTM

@codecov

This comment has been minimized.

Copy link

commented Aug 8, 2019

Codecov Report

Merging #11677 into master will decrease coverage by 0.0216%.
The diff coverage is 66.6666%.

@@               Coverage Diff                @@
##             master     #11677        +/-   ##
================================================
- Coverage   81.3389%   81.3173%   -0.0217%     
================================================
  Files           426        426                
  Lines         92460      92353       -107     
================================================
- Hits          75206      75099       -107     
+ Misses        11877      11874         -3     
- Partials       5377       5380         +3
@codecov

This comment has been minimized.

Copy link

commented Aug 8, 2019

Codecov Report

Merging #11677 into master will increase coverage by 0.0011%.
The diff coverage is 66.6666%.

@@               Coverage Diff                @@
##             master     #11677        +/-   ##
================================================
+ Coverage   81.4483%   81.4495%   +0.0011%     
================================================
  Files           428        428                
  Lines         92380      92386         +6     
================================================
+ Hits          75242      75248         +6     
- Misses        11760      11762         +2     
+ Partials       5378       5376         -2
@@ -371,6 +377,13 @@ func (c *Column) Float64s() []float64 {
return res
}

// Durations returns a Golang time.Duration slice stored in this Column.
func (c *Column) Durations() []time.Duration {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Aug 8, 2019

Member

how about s/Duration/GoDuration/?

This comment has been minimized.

Copy link
@qw4990

qw4990 Aug 8, 2019

Author Contributor

updated.

@qw4990 qw4990 force-pushed the qw4990:vecexpr-vecdur branch from e127e18 to d5ff716 Aug 8, 2019

@qw4990

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

/run-all-tests

@qw4990 qw4990 force-pushed the qw4990:vecexpr-vecdur branch from d5ff716 to 14605cd Aug 8, 2019

@qw4990

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

/run-all-tests

@@ -371,6 +377,13 @@ func (c *Column) Float64s() []float64 {
return res
}

// GoDurations returns a Golang time.Duration slice stored in this Column.

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Aug 8, 2019

Contributor

Do we need to add some comment to remind the caller about fsp?

This comment has been minimized.

Copy link
@qw4990

qw4990 Aug 8, 2019

Author Contributor

More comments are added~

@qw4990 qw4990 force-pushed the qw4990:vecexpr-vecdur branch from 14605cd to cf1e48f Aug 8, 2019

@zz-jason
Copy link
Member

left a comment

LGTM

@zz-jason

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

it's better to update the PR title by s/Duration/time.Duration/

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Aug 8, 2019

@qw4990 qw4990 changed the title executor: add vectorized access methods for `Duration` executor: add vectorized access methods for `time.Duration` stored in `Column` Aug 8, 2019

@qw4990 qw4990 merged commit 6098ed6 into pingcap:master Aug 8, 2019

8 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 66.6666% of diff hit (target 0%)
Details
codecov/project 81.4495% (+0.0011%) compared to 554594b
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.