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 arithmetic vec int divide int sig #14464

Merged
merged 7 commits into from
Jan 20, 2020
Merged

expression: implement arithmetic vec int divide int sig #14464

merged 7 commits into from
Jan 20, 2020

Conversation

ZiheLiu
Copy link
Contributor

@ZiheLiu ZiheLiu commented Jan 13, 2020 •
edited
Loading

PCP #12103

What problem does this PR solve?

implement builtinArithmeticIntDivideIntSig vectorized function
#12103

What is changed and how it works?

  • implement vectorized builtinArithmeticIntDivideIntSig
  • add unit test for vectorized builtinArithmeticIntDivideIntSig with 4 test cases:
    • int64 divides int64
    • uint64 divides uint64
    • int64 divides uint64
    • uint64 divides int64

about 3 times faster

$  go test -v -benchmem -bench=BenchmarkVectorizedBuiltinArithmeticFunc -run=BenchmarkVectorizedBuiltinArithmeticFunc -args builtinArithmeticIntDivideIntSig
goos: darwin
goarch: amd64
pkg: github.com/pingcap/tidb/expression
BenchmarkVectorizedBuiltinArithmeticFunc/builtinArithmeticIntDivideIntSig-VecBuiltinFunc-8                 64203             18457 ns/op               0 B/op          0 allocs/op
BenchmarkVectorizedBuiltinArithmeticFunc/builtinArithmeticIntDivideIntSig-NonVecBuiltinFunc-8              25732             46684 ns/op               0 B/op          0 allocs/op
BenchmarkVectorizedBuiltinArithmeticFunc/builtinArithmeticIntDivideIntSig-VecBuiltinFunc#01-8             106828             11296 ns/op               0 B/op          0 allocs/op
BenchmarkVectorizedBuiltinArithmeticFunc/builtinArithmeticIntDivideIntSig-NonVecBuiltinFunc#01-8           28982             40606 ns/op               0 B/op          0 allocs/op
BenchmarkVectorizedBuiltinArithmeticFunc/builtinArithmeticIntDivideIntSig-VecBuiltinFunc#02-8              78523             15427 ns/op               0 B/op          0 allocs/op
BenchmarkVectorizedBuiltinArithmeticFunc/builtinArithmeticIntDivideIntSig-NonVecBuiltinFunc#02-8           26662             44313 ns/op               0 B/op          0 allocs/op
BenchmarkVectorizedBuiltinArithmeticFunc/builtinArithmeticIntDivideIntSig-VecBuiltinFunc#03-8              78825             15292 ns/op               0 B/op          0 allocs/op
BenchmarkVectorizedBuiltinArithmeticFunc/builtinArithmeticIntDivideIntSig-NonVecBuiltinFunc#03-8           27369             44987 ns/op               0 B/op          0 allocs/op
PASS
ok      github.com/pingcap/tidb/expression      12.083s

Check List

Tests

  • Unit test

@ZiheLiu ZiheLiu requested a review from a team as a code owner January 13, 2020 16:48
@sre-bot
Copy link
Contributor

sre-bot commented Jan 13, 2020

Thanks for your contribution. If your PR get merged, you will be rewarded 50 points.

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Jan 13, 2020
@ZiheLiu ZiheLiu force-pushed the feature_builtinArithmeticIntDivideIntSig branch from bfff894 to b2b6071 Compare January 13, 2020 17:03
@ZiheLiu ZiheLiu force-pushed the feature_builtinArithmeticIntDivideIntSig branch from b2b6071 to f72088c Compare January 13, 2020 17:10
@ZiheLiu ZiheLiu changed the title expression: implement builtinArithmeticIntDivideIntSig expression: implement arithmetic vec int divide int sig Jan 13, 2020
@ghost ghost requested review from qw4990 and XuHuaiyu and removed request for a team January 13, 2020 17:30
@qw4990 qw4990 requested a review from wshwsh12 January 19, 2020 03:40
qw4990
qw4990 reviewed Jan 19, 2020
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.

Thanks for your contribution, LGTM.

wshwsh12
wshwsh12 previously approved these changes Jan 19, 2020
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

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

LGTM

@wshwsh12 wshwsh12 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. labels Jan 19, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 19, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jan 19, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jan 19, 2020

@ZiheLiu merge failed.

@qw4990
Copy link
Contributor

qw4990 commented Jan 19, 2020

It panicked, and here is the stack:

[2020-01-19T06:33:26.465Z] PANIC: builtin_arithmetic_vec_test.go:182: testEvaluatorSuite.TestVectorizedBuiltinArithmeticFunc
[2020-01-19T06:33:26.465Z] 
[2020-01-19T06:33:26.465Z] ... Panic: runtime error: invalid memory address or nil pointer dereference (PC=0xEA6C21)
[2020-01-19T06:33:26.465Z] 
[2020-01-19T06:33:26.465Z] /usr/local/go/src/runtime/panic.go:679
[2020-01-19T06:33:26.465Z]   in gopanic
[2020-01-19T06:33:26.465Z] /usr/local/go/src/runtime/panic.go:199
[2020-01-19T06:33:26.465Z]   in panicmem
[2020-01-19T06:33:26.465Z] /usr/local/go/src/runtime/signal_unix.go:394
[2020-01-19T06:33:26.465Z]   in sigpanic
[2020-01-19T06:33:26.465Z] bench_test.go:533
[2020-01-19T06:33:26.465Z]   in rangeInt64Gener.gen
[2020-01-19T06:33:26.465Z] bench_test.go:958
[2020-01-19T06:33:26.465Z]   in fillColumnWithGener
[2020-01-19T06:33:26.465Z] bench_test.go:946
[2020-01-19T06:33:26.465Z]   in fillColumn
[2020-01-19T06:33:26.465Z] bench_test.go:1174
[2020-01-19T06:33:26.465Z]   in genVecBuiltinFuncBenchCase
[2020-01-19T06:33:26.465Z] bench_test.go:1279
[2020-01-19T06:33:26.465Z]   in testVectorizedBuiltinFunc
[2020-01-19T06:33:26.465Z] builtin_arithmetic_vec_test.go:183
[2020-01-19T06:33:26.465Z]   in testEvaluatorSuite.TestVectorizedBuiltinArithmeticFunc
[2020-01-19T06:33:26.465Z] /usr/local/go/src/reflect/value.go:321
[2020-01-19T06:33:26.465Z]   in Value.Call
[2020-01-19T06:33:26.465Z] /home/jenkins/agent/workspace/tidb_ghpr_check_2/go/pkg/mod/github.com/pingcap/[email protected]/check.go:850
[2020-01-19T06:33:26.465Z]   in suiteRunner.forkTest.func1
[2020-01-19T06:33:26.465Z] /home/jenkins/agent/workspace/tidb_ghpr_check_2/go/pkg/mod/github.com/pingcap/[email protected]/check.go:739
[2020-01-19T06:33:26.465Z]   in suiteRunner.forkCall.func1
[2020-01-19T06:33:26.465Z] /usr/local/go/src/runtime/asm_amd64.s:1357
[2020-01-19T06:33:26.465Z]   in goexit

@ZiheLiu Please fix this problem at your convenience.

@bb7133 bb7133 added this to the v4.0.0-beta.1 milestone Jan 19, 2020
zz-jason
zz-jason approved these changes Jan 20, 2020
View reviewed changes
Copy link
Member

@zz-jason zz-jason 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

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jan 20, 2020

/run-all-tests

@sre-bot sre-bot merged commit 50e7cab into pingcap:master Jan 20, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 20, 2020

@ZiheLiu complete task #12103 and get 50 score, currerent score 50.

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

@zz-jason zz-jason zz-jason approved these changes

@wshwsh12 wshwsh12 wshwsh12 left review comments

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

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
v4.0.0-beta.1
Development

Successfully merging this pull request may close these issues.
6 participants
@ZiheLiu @sre-bot @qw4990 @zz-jason @wshwsh12 @bb7133