expression: separated arithmeticPlusIntSig #22026

Tjianke · 2020-12-25T05:47:11Z

What problem does this PR solve?

Issue Number: closes #13973

Problem Summary:

Separating ArithmeticPlusIntSig at planning stage will be more efficient then current status

What is changed and how it works?

What's Changed:

Impl vectorized, separated func sig from original PlusIntSig, separated PlusIntSig into four signatures by sign/unsign of arguments

How it Works:

Benchmark Result:

 benchstat ~/plusOld.txt plusSSNew.txt 
name                                                                             old time/op    new time/op    delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-VecBuiltinFunc-8       5.02µs ± 4%    4.67µs ± 2%   -6.94%  (p=0.000 n=19+19)
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-NonVecBuiltinFunc-8    45.0µs ±83%    30.3µs ± 1%  -32.71%  (p=0.000 n=20+20)

name                                                                             old alloc/op   new alloc/op   delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-VecBuiltinFunc-8        0.00B          0.00B          ~     (all equal)
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-NonVecBuiltinFunc-8     0.00B          0.00B          ~     (all equal)

name                                                                             old allocs/op  new allocs/op  delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-VecBuiltinFunc-8         0.00           0.00          ~     (all equal)
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-NonVecBuiltinFunc-8      0.00           0.00          ~     (all equal)

benchstat ~/plusOld.txt plusSUNew.txt 
name                                                                             old time/op    new time/op    delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-VecBuiltinFunc-8       5.02µs ± 4%    2.81µs ± 1%  -43.94%  (p=0.000 n=19+20)
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-NonVecBuiltinFunc-8    45.0µs ±83%    23.3µs ± 2%  -48.15%  (p=0.000 n=20+20)

name                                                                             old alloc/op   new alloc/op   delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-VecBuiltinFunc-8        0.00B          0.00B          ~     (all equal)
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-NonVecBuiltinFunc-8     0.00B          0.00B          ~     (all equal)

name                                                                             old allocs/op  new allocs/op  delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-VecBuiltinFunc-8         0.00           0.00          ~     (all equal)
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-NonVecBuiltinFunc-8      0.00           0.00          ~     (all equal)

benchstat ~/plusOld.txt plusUSNew.txt 
name                                                                             old time/op    new time/op    delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-VecBuiltinFunc-8       5.02µs ± 4%    2.84µs ± 1%  -43.42%  (p=0.000 n=19+20)
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-NonVecBuiltinFunc-8    45.0µs ±83%    23.3µs ± 2%  -48.30%  (p=0.000 n=20+19)

name                                                                             old alloc/op   new alloc/op   delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-VecBuiltinFunc-8        0.00B          0.00B          ~     (all equal)
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-NonVecBuiltinFunc-8     0.00B          0.00B          ~     (all equal)

name                                                                             old allocs/op  new allocs/op  delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-VecBuiltinFunc-8         0.00           0.00          ~     (all equal)
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-NonVecBuiltinFunc-8      0.00           0.00          ~     (all equal)

benchstat ~/plusOld.txt plusUUNew.txt 
name                                                                             old time/op    new time/op    delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-VecBuiltinFunc-8       5.02µs ± 4%    2.34µs ± 1%  -53.42%  (p=0.000 n=19+19)
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-NonVecBuiltinFunc-8    45.0µs ±83%    23.0µs ± 2%  -48.90%  (p=0.000 n=20+19)

name                                                                             old alloc/op   new alloc/op   delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-VecBuiltinFunc-8        0.00B          0.00B          ~     (all equal)
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-NonVecBuiltinFunc-8     0.00B          0.00B          ~     (all equal)

name                                                                             old allocs/op  new allocs/op  delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-VecBuiltinFunc-8         0.00           0.00          ~     (all equal)
VectorizedBuiltinArithmeticFunc/builtinArithmeticPlusIntSig-NonVecBuiltinFunc-8      0.00           0.00          ~     (all equal)

plusOld.txt
plusSSNew.txt
plusUSNew.txt
plusSUNew.txt
plusUUNew.txt

Check List

Tests

Unit test

Release note

separated signatures for builtin PlusIngSig

sre-bot · 2020-12-25T05:47:22Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

breezewish · 2020-12-27T07:38:23Z

/run-all-tests

Tjianke · 2020-12-30T02:17:00Z

@breeswish PTAL

expression/builtin_arithmetic.go

Tjianke · 2020-12-30T04:40:51Z

/run-all-tests

breezewish

LGTM. Thanks! @qw4990 @XuHuaiyu PTAL, thanks!

Tjianke · 2021-02-01T01:33:11Z

@qw4990 @XuHuaiyu PTAL, thanks!

pingyu

@Tjianke
Seems that it should be math.MaxUint64-uint64(rh) here.

pingyu · 2021-02-06T14:07:56Z

expression/builtin_arithmetic_vec.go

 			return types.ErrOverflow.GenWithStackByArgs("BIGINT UNSIGNED", fmt.Sprintf("(%s + %s)", b.args[0].String(), b.args[1].String()))
 		}
-		if lh > 0 && uint64(rh) > math.MaxUint64-uint64(lh) {
+		if rh > 0 && uint64(lh) > math.MaxUint64-uint64(lh) {


Suggested change

if rh > 0 && uint64(lh) > math.MaxUint64-uint64(lh) {

if rh > 0 && uint64(lh) > math.MaxUint64-uint64(rh) {

Thanks for pointing out.

pingyu · 2021-02-06T14:23:06Z

I think it would be better to add unit tests for OVERFLOW cases. Rest LGTM.

pingyu · 2021-02-07T18:03:38Z

LGTM

pingyu · 2021-02-07T18:04:13Z

Great !
@breeswish PTAL~

Tjianke · 2021-02-22T15:47:03Z

/run-all-tests

…o separatePlusSig

zz-jason · 2021-02-23T10:56:27Z

@Tjianke could you post the benchmark result to show the performance improvements?

Tjianke · 2021-02-23T15:44:19Z

@Tjianke could you post the benchmark result to show the performance improvements?

Benchmark added, no significant regression or improvement.

Note that the main purpose of #13973 and other builtin function separation is to improve efficiency on TiKV side.

zz-jason · 2021-03-27T13:53:04Z

@Tjianke could you post the benchmark result to show the performance improvements?

Benchmark added, no significant regression or improvement.

Note that the main purpose of #13973 and other builtin function separation is to improve efficiency on TiKV side.

If the TiKV coprocessor writes the code in the similar way to TiDB, how could it improve the performance in the TiKV side?

Do you have any benchmark result (for the demo code in TiKV) or any analysis on that?

Tjianke · 2021-04-01T11:02:13Z

@Tjianke could you post the benchmark result to show the performance improvements?

Benchmark added, no significant regression or improvement.
Note that the main purpose of #13973 and other builtin function separation is to improve efficiency on TiKV side.

If the TiKV coprocessor writes the code in the similar way to TiDB, how could it improve the performance in the TiKV side?

Do you have any benchmark result (for the demo code in TiKV) or any analysis on that?

Sorry, I cannot provide you TiKV benchmark since I am not familiar with TiKV. But the logic here is that judging args and separating functions at planning stage(in TiDB) would save TiKV the time of judging args, therefore improve efficiency. Origin idea from @breeswish.

zz-jason · 2021-04-12T02:01:31Z

From the performance benchmark, the performance is not improved, but the code complexity is improved, so I prefer not to bring in this change if there is no benefit.

Tjianke · 2021-04-12T09:33:33Z

From the performance benchmark, the performance is not improved, but the code complexity is improved, so I prefer not to bring in this change if there is no benefit.

Original benchmark was inaccurate, after func sig separation, benchstat cannot compare directly between old and new function signatures, I had to rename benchmark result in newer version to compare, seems like there is improvement in newer versions(benchmark updated in main post).

I wrote a script for quick reproduction.

ti-chi-bot · 2021-06-19T20:18:27Z

@Tjianke: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

separated arithmeticPlusIntSig

d4a4d96

ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Dec 25, 2020

github-actions bot added the component/expression label Dec 25, 2020

Tjianke marked this pull request as ready for review December 25, 2020 06:01

Tjianke requested a review from a team as a code owner December 25, 2020 06:01

Tjianke requested review from SunRunAway and removed request for a team December 25, 2020 06:01

Tjianke mentioned this pull request Dec 25, 2020

Separate signatures for PlusInt #13973

Open

breezewish reviewed Dec 30, 2020

View reviewed changes

expression/builtin_arithmetic.go Show resolved Hide resolved

expression/builtin_arithmetic.go Show resolved Hide resolved

breezewish previously approved these changes Dec 30, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 30, 2020

ti-srebot added the status/all-tests-passed label Feb 1, 2021

pingyu suggested changes Feb 6, 2021

View reviewed changes

Tjianke dismissed breezewish’s stale review via 00fa682 February 6, 2021 14:12

address comments

76adc56

Tjianke force-pushed the separatePlusSig branch from 00fa682 to 76adc56 Compare February 6, 2021 14:50

ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Feb 7, 2021

ti-srebot approved these changes Feb 7, 2021

View reviewed changes

ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 7, 2021

Merge branch 'master' into separatePlusSig

a9a0016

ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 22, 2021

Tjianke added 2 commits February 23, 2021 00:01

fix ut

1e3c214

Merge branch 'separatePlusSig' of https://github.com/Tjianke/tidb int…

ac06c9c

…o separatePlusSig

Merge branch 'master' into separatePlusSig

e1dfe42

SunRunAway removed their request for review March 28, 2021 12:50

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2021

tisonkun removed status/all-tests-passed labels Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: separated arithmeticPlusIntSig #22026

expression: separated arithmeticPlusIntSig #22026

Tjianke commented Dec 25, 2020 •

edited

Loading

sre-bot commented Dec 25, 2020

breezewish commented Dec 27, 2020

Tjianke commented Dec 30, 2020

Tjianke commented Dec 30, 2020

breezewish left a comment

Tjianke commented Feb 1, 2021

pingyu left a comment

pingyu Feb 6, 2021

Tjianke Feb 6, 2021

pingyu commented Feb 6, 2021

pingyu commented Feb 7, 2021

pingyu commented Feb 7, 2021

Tjianke commented Feb 22, 2021

zz-jason commented Feb 23, 2021

Tjianke commented Feb 23, 2021

zz-jason commented Mar 27, 2021

Tjianke commented Apr 1, 2021

zz-jason commented Apr 12, 2021

Tjianke commented Apr 12, 2021 •

edited

Loading

ti-chi-bot commented Jun 19, 2021

	if rh > 0 && uint64(lh) > math.MaxUint64-uint64(lh) {
	if rh > 0 && uint64(lh) > math.MaxUint64-uint64(rh) {

expression: separated arithmeticPlusIntSig #22026

Are you sure you want to change the base?

expression: separated arithmeticPlusIntSig #22026

Conversation

Tjianke commented Dec 25, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

sre-bot commented Dec 25, 2020

breezewish commented Dec 27, 2020

Tjianke commented Dec 30, 2020

Tjianke commented Dec 30, 2020

breezewish left a comment

Choose a reason for hiding this comment

Tjianke commented Feb 1, 2021

pingyu left a comment

Choose a reason for hiding this comment

pingyu Feb 6, 2021

Choose a reason for hiding this comment

Tjianke Feb 6, 2021

Choose a reason for hiding this comment

pingyu commented Feb 6, 2021

pingyu commented Feb 7, 2021

pingyu commented Feb 7, 2021

Tjianke commented Feb 22, 2021

zz-jason commented Feb 23, 2021

Tjianke commented Feb 23, 2021

zz-jason commented Mar 27, 2021

Tjianke commented Apr 1, 2021

zz-jason commented Apr 12, 2021

Tjianke commented Apr 12, 2021 • edited Loading

ti-chi-bot commented Jun 19, 2021

Tjianke commented Dec 25, 2020 •

edited

Loading

Tjianke commented Apr 12, 2021 •

edited

Loading