Skip to content

Commit 2c14a75

Browse files
authored
perf(overflow): Speed up overflow checks for small integers (#303)
### Rationale for this change We can check the bounds of the arguments to avoid an int64 division. The `Mul` and `Mul64` functions perform better on the systems I tested. This is an older machine: ``` goos: linux goarch: amd64 pkg: github.com/apache/arrow-go/v18/internal/utils cpu: Intel(R) Core(TM) i5-7Y57 CPU @ 1.20GHz │ bench_amd64_old.txt │ bench_amd64_new.txt │ │ sec/op │ sec/op vs base │ Add/int/8192_+_8192-4 1.990n ± 14% 2.001n ± 18% ~ (p=1.000 n=6) Add/int/MaxInt16_+_1-4 2.248n ± 1% 2.271n ± 4% +1.07% (p=0.035 n=6) Add/int/MaxInt16_+_5-4 2.215n ± 2% 2.270n ± 3% +2.48% (p=0.013 n=6) Add/int/MaxInt16_+_MaxInt16-4 2.293n ± 4% 2.331n ± 9% ~ (p=0.195 n=6) Add/int/MaxInt32_+_1-4 2.339n ± 16% 2.345n ± 67% ~ (p=0.937 n=6) Add/int/MaxInt32_+_5-4 2.363n ± 4% 2.330n ± 2% ~ (p=0.331 n=6) Add/int/MaxInt32_+_MaxInt32-4 2.402n ± 23% 2.343n ± 3% ~ (p=0.310 n=6) Add/int/MaxInt_+_MaxInt-4 2.334n ± 3% 2.364n ± 2% ~ (p=0.180 n=6) Mul/int/8192_×_8192-4 14.335n ± 3% 3.578n ± 5% -75.04% (p=0.002 n=6) Mul/int/MaxInt16_×_1-4 14.015n ± 90% 3.718n ± 20% -73.47% (p=0.002 n=6) Mul/int/MaxInt16_×_5-4 14.480n ± 25% 3.647n ± 3% -74.82% (p=0.002 n=6) Mul/int/MaxInt16_×_MaxInt16-4 14.450n ± 30% 3.627n ± 35% -74.90% (p=0.002 n=6) Mul/int/MaxInt32_×_1-4 14.725n ± 2% 3.597n ± 12% -75.57% (p=0.002 n=6) Mul/int/MaxInt32_×_5-4 17.485n ± 31% 3.667n ± 5% -79.03% (p=0.002 n=6) Mul/int/MaxInt32_×_MaxInt32-4 14.385n ± 31% 5.704n ± 48% -60.35% (p=0.002 n=6) Mul/int/MaxInt_×_MaxInt-4 14.34n ± 3% 15.68n ± 4% +9.34% (p=0.002 n=6) Mul64/int64/8192_×_8192-4 14.495n ± 6% 3.730n ± 4% -74.27% (p=0.002 n=6) Mul64/int64/MaxInt16_×_1-4 14.300n ± 3% 3.612n ± 1% -74.74% (p=0.002 n=6) Mul64/int64/MaxInt16_×_5-4 14.020n ± 20% 3.657n ± 3% -73.91% (p=0.002 n=6) Mul64/int64/MaxInt16_×_MaxInt16-4 14.110n ± 2% 3.611n ± 12% -74.41% (p=0.002 n=6) Mul64/int64/MaxInt32_×_1-4 14.330n ± 2% 3.609n ± 4% -74.82% (p=0.002 n=6) Mul64/int64/MaxInt32_×_5-4 14.250n ± 14% 3.670n ± 2% -74.25% (p=0.002 n=6) Mul64/int64/MaxInt32_×_MaxInt32-4 14.070n ± 2% 3.571n ± 3% -74.62% (p=0.002 n=6) Mul64/int64/MaxInt_×_MaxInt-4 14.17n ± 2% 15.77n ± 3% +11.25% (p=0.002 n=6) geomean 7.807n 3.583n -54.10% ``` The following is inside a `docker.io/i386/ubuntu` container on that same machine. I don't have any 32-bit hardware. ``` goos: linux goarch: 386 pkg: github.com/apache/arrow-go/v18/internal/utils cpu: Intel(R) Core(TM) i5-7Y57 CPU @ 1.20GHz │ bench_386_old.txt │ bench_386_new.txt │ │ sec/op │ sec/op vs base │ Add/int/8192_+_8192-4 3.113n ± 22% 2.848n ± 17% ~ (p=0.180 n=6) Add/int/MaxInt16_+_1-4 3.344n ± 4% 3.294n ± 18% ~ (p=0.818 n=6) Add/int/MaxInt16_+_5-4 3.420n ± 7% 3.411n ± 5% ~ (p=0.937 n=6) Add/int/MaxInt16_+_MaxInt16-4 3.360n ± 3% 3.402n ± 30% ~ (p=0.240 n=6) Add/int/MaxInt_+_1-4 3.329n ± 2% 3.433n ± 3% +3.09% (p=0.039 n=6) Add/int/MaxInt_+_5-4 3.452n ± 5% 3.436n ± 2% ~ (p=0.937 n=6) Add/int/MaxInt_+_MaxInt-4 3.353n ± 6% 3.454n ± 3% ~ (p=0.132 n=6) Add/int/MaxInt_+_MaxInt#01-4 3.346n ± 13% 3.488n ± 3% ~ (p=0.132 n=6) Mul/int/8192_×_8192-4 11.250n ± 2% 6.238n ± 6% -44.55% (p=0.002 n=6) Mul/int/MaxInt16_×_1-4 11.565n ± 4% 6.261n ± 1% -45.86% (p=0.002 n=6) Mul/int/MaxInt16_×_5-4 11.300n ± 3% 6.505n ± 3% -42.44% (p=0.002 n=6) Mul/int/MaxInt16_×_MaxInt16-4 11.370n ± 4% 6.394n ± 40% -43.76% (p=0.002 n=6) Mul/int/MaxInt_×_1-4 11.245n ± 3% 5.798n ± 2% -48.44% (p=0.002 n=6) Mul/int/MaxInt_×_5-4 11.210n ± 18% 9.941n ± 2% -11.32% (p=0.002 n=6) Mul/int/MaxInt_×_MaxInt-4 10.975n ± 4% 9.895n ± 2% -9.84% (p=0.002 n=6) Mul/int/MaxInt_×_MaxInt#01-4 11.40n ± 18% 10.17n ± 2% -10.83% (p=0.002 n=6) Mul64/int64/8192_×_8192-4 21.74n ± 70% 23.75n ± 2% ~ (p=0.065 n=6) Mul64/int64/MaxInt16_×_1-4 22.37n ± 28% 23.87n ± 12% ~ (p=0.065 n=6) Mul64/int64/MaxInt16_×_5-4 22.09n ± 2% 23.96n ± 23% +8.44% (p=0.002 n=6) Mul64/int64/MaxInt16_×_MaxInt16-4 21.06n ± 3% 24.27n ± 3% +15.24% (p=0.002 n=6) Mul64/int64/MaxInt_×_1-4 21.42n ± 8% 23.91n ± 64% +11.62% (p=0.002 n=6) Mul64/int64/MaxInt_×_5-4 29.87n ± 4% 24.16n ± 22% -19.09% (p=0.009 n=6) Mul64/int64/MaxInt_×_MaxInt-4 28.10n ± 2% 24.40n ± 2% -13.17% (p=0.002 n=6) Mul64/int64/9223372036854775807_×_9223372036854775807-4 23.22n ± 6% 31.90n ± 35% +37.38% (p=0.002 n=6) geomean 9.609n 8.523n -11.30% ``` ### What changes are included in this PR? A new generic `Add` function in `internal/utils` returns the sum of any two integers and whether or not it overflowed. New `Mul` and `Mul64` functions in `internal/utils` return the product of two `int` or `int64`, respectively, and whether or not it overflowed. These functions perform better on the systems I tested. ### Are these changes tested? Yes. ### Are there any user-facing changes? No effect on the API. This drops one dependency. --------- Signed-off-by: Chris Bandy <bandy.chris@gmail.com>
1 parent 0f0d667 commit 2c14a75

11 files changed

Lines changed: 441 additions & 16 deletions

File tree

arrow/compute/internal/kernels/base_arithmetic.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,12 @@ import (
2323
"math"
2424
"math/bits"
2525

26-
"github.com/JohnCGriffin/overflow"
2726
"github.com/apache/arrow-go/v18/arrow"
2827
"github.com/apache/arrow-go/v18/arrow/compute/exec"
2928
"github.com/apache/arrow-go/v18/arrow/decimal128"
3029
"github.com/apache/arrow-go/v18/arrow/decimal256"
3130
"github.com/apache/arrow-go/v18/arrow/internal/debug"
31+
"github.com/apache/arrow-go/v18/internal/utils"
3232
"golang.org/x/exp/constraints"
3333
)
3434

@@ -709,7 +709,7 @@ func SubtractDate32(op ArithmeticOp) exec.ArrayKernelExec {
709709
case OpSubChecked:
710710
return ScalarBinary(getGoArithmeticBinary(func(a, b arrow.Time32, e *error) (result arrow.Duration) {
711711
result = arrow.Duration(a) - arrow.Duration(b)
712-
val, ok := overflow.Mul64(int64(result), secondsPerDay)
712+
val, ok := utils.Mul64(int64(result), secondsPerDay)
713713
if !ok {
714714
*e = errOverflow
715715
}

go.mod

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ go 1.23.0
2121
toolchain go1.23.2
2222

2323
require (
24-
github.com/JohnCGriffin/overflow v0.0.0-20211019200055-46fa312c352c
2524
github.com/andybalholm/brotli v1.1.1
2625
github.com/apache/thrift v0.21.0
2726
github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815

go.sum

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ atomicgo.dev/schedule v0.1.0 h1:nTthAbhZS5YZmgYbb2+DH8uQIZcTlIrd4eYr3UQxEjs=
88
atomicgo.dev/schedule v0.1.0/go.mod h1:xeUa3oAkiuHYh8bKiQBRojqAMq3PXXbJujjb0hw8pEU=
99
cloud.google.com/go v0.118.0 h1:tvZe1mgqRxpiVa3XlIGMiPcEUbP1gNXELgD4y/IXmeQ=
1010
cloud.google.com/go v0.118.0/go.mod h1:zIt2pkedt/mo+DQjcT4/L3NDxzHPR29j5HcclNH+9PM=
11-
github.com/JohnCGriffin/overflow v0.0.0-20211019200055-46fa312c352c h1:RGWPOewvKIROun94nF7v2cua9qP+thov/7M50KEoeSU=
12-
github.com/JohnCGriffin/overflow v0.0.0-20211019200055-46fa312c352c/go.mod h1:X0CRv0ky0k6m906ixxpzmDRLvX58TFUKS2eePweuyxk=
1311
github.com/MarvinJWendt/testza v0.1.0/go.mod h1:7AxNvlfeHP7Z/hDQ5JtE3OKYT3XFUeLCDE2DQninSqs=
1412
github.com/MarvinJWendt/testza v0.2.1/go.mod h1:God7bhG8n6uQxwdScay+gjm9/LnO4D3kkcZX4hv9Rp8=
1513
github.com/MarvinJWendt/testza v0.2.8/go.mod h1:nwIcjmr0Zz+Rcwfh3/4UhBp7ePKVhuBExvZqnKYWlII=

internal/utils/math.go

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,12 @@
1616

1717
package utils
1818

19-
import "golang.org/x/exp/constraints"
19+
import (
20+
"math"
21+
"math/bits"
22+
23+
"golang.org/x/exp/constraints"
24+
)
2025

2126
func Min[T constraints.Ordered](a, b T) T {
2227
if a < b {
@@ -31,3 +36,91 @@ func Max[T constraints.Ordered](a, b T) T {
3136
}
3237
return b
3338
}
39+
40+
// Add returns the sum of two integers while checking for [overflow].
41+
// It returns false (not ok) when the operation overflows.
42+
//
43+
// [overflow]: https://go.dev/ref/spec#Integer_overflow
44+
func Add[T constraints.Signed](a, b T) (T, bool) {
45+
// Overflow occurs when a and b are too positive or too negative.
46+
// That is, when: (a > 0) && (b > 0) && (a > math.Max[T] - b)
47+
// or when: (a < 0) && (b < 0) && (a < math.Min[T] - b)
48+
result := a + b
49+
50+
// No overflow occurred if the result is larger exactly when b is positive.
51+
return result, (result > a) == (b > 0)
52+
}
53+
54+
const (
55+
sqrtMaxInt = 1<<((bits.UintSize>>1)-1) - 1
56+
sqrtMinInt = -1 << ((bits.UintSize >> 1) - 1)
57+
)
58+
59+
// Mul returns the product of two integers while checking for [overflow].
60+
// It returns false (not ok) when the operation overflows.
61+
//
62+
// [overflow]: https://go.dev/ref/spec#Integer_overflow
63+
func Mul(a, b int) (int, bool) {
64+
// Avoid division by zero and calculate nothing when a or b is zero.
65+
if a == 0 || b == 0 {
66+
return 0, true
67+
}
68+
69+
result := a * b
70+
71+
// Overflow occurred if the result is positive when exactly one input
72+
// is negative.
73+
if result > 0 == ((a < 0) != (b < 0)) {
74+
return result, false
75+
}
76+
77+
// Overflow cannot occur when a or b is zero or one.
78+
// Overflow cannot occur when a and b are less positive than sqrt(MaxInt).
79+
// Overflow cannot occur when a and b are less negative than sqrt(MinInt).
80+
if (sqrtMinInt <= a && a <= sqrtMaxInt &&
81+
sqrtMinInt <= b && b <= sqrtMaxInt) || a == 1 || b == 1 {
82+
return result, true
83+
}
84+
85+
// Finally, no overflow occurred if division produces the input. This is
86+
// last because division can be expensive. Dividing by -1 can overflow,
87+
// but we returned early in that case above.
88+
return result, (result/a == b)
89+
}
90+
91+
const (
92+
sqrtMaxInt64 = math.MaxInt32
93+
sqrtMinInt64 = math.MinInt32
94+
)
95+
96+
// Mul64 returns the product of two integers while checking for [overflow].
97+
// It returns false (not ok) when the operation overflows.
98+
//
99+
// [overflow]: https://go.dev/ref/spec#Integer_overflow
100+
func Mul64(a, b int64) (int64, bool) {
101+
// Avoid division by zero and calculate nothing when a or b is zero.
102+
if a == 0 || b == 0 {
103+
return 0, true
104+
}
105+
106+
result := a * b
107+
108+
// Overflow occurred if the result is positive when exactly one input
109+
// is negative.
110+
if result > 0 == ((a < 0) != (b < 0)) {
111+
return result, false
112+
}
113+
114+
// Overflow cannot occur when a or b is zero or one.
115+
// Overflow cannot occur when a and b are less positive than sqrt(MaxInt64).
116+
// Overflow cannot occur when a and b are less negative than sqrt(MinInt64).
117+
if (sqrtMinInt64 <= a && a <= sqrtMaxInt64 &&
118+
sqrtMinInt64 <= b && b <= sqrtMaxInt64) || a == 1 || b == 1 {
119+
return result, true
120+
}
121+
122+
// Finally, no overflow occurred if division produces the input. This is
123+
// last because division can be expensive. Dividing by -1 can overflow,
124+
// but we returned early in that case above.
125+
return result, (result/a == b)
126+
}

internal/utils/math_32bit_test.go

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing, software
12+
// distributed under the License is distributed on an "AS IS" BASIS,
13+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
// See the License for the specific language governing permissions and
15+
// limitations under the License.
16+
17+
//go:build 386 || arm || mips || ppc
18+
19+
package utils
20+
21+
import (
22+
"math"
23+
"testing"
24+
25+
"github.com/stretchr/testify/assert"
26+
"github.com/stretchr/testify/require"
27+
)
28+
29+
func TestMul_32bit(t *testing.T) {
30+
// These constants are smaller in magnitude than the exact square root.
31+
assert.Greater(t, sqrtMinInt, int(math.Sqrt(math.MinInt32)))
32+
assert.Less(t, sqrtMaxInt, int(math.Sqrt(math.MaxInt32)))
33+
34+
for _, tt := range []struct {
35+
a, b, expected int
36+
ok bool
37+
}{
38+
{ok: true, a: math.MinInt16, b: 0, expected: 0},
39+
{ok: true, a: math.MinInt16, b: 1, expected: math.MinInt16},
40+
{ok: true, a: math.MinInt16, b: -1, expected: -math.MinInt16},
41+
42+
{ok: true, a: math.MaxInt16, b: 0, expected: 0},
43+
{ok: true, a: math.MaxInt16, b: 1, expected: math.MaxInt16},
44+
{ok: true, a: math.MaxInt16, b: -1, expected: -math.MaxInt16},
45+
46+
{ok: true, a: math.MinInt16, b: math.MinInt16, expected: 1073741824},
47+
{ok: true, a: math.MaxInt16, b: math.MaxInt16, expected: 1073676289},
48+
{ok: true, a: math.MinInt16, b: math.MaxInt16, expected: -1073709056},
49+
{ok: true, a: math.MaxInt16, b: math.MinInt16, expected: -1073709056},
50+
51+
{ok: true, a: math.MinInt32, b: 0, expected: 0},
52+
{ok: true, a: math.MinInt32, b: 1, expected: math.MinInt32},
53+
{ok: false, a: math.MinInt32, b: -1, expected: math.MinInt32},
54+
{ok: false, a: math.MinInt32, b: -2, expected: 0},
55+
{ok: false, a: math.MinInt32, b: 2, expected: 0},
56+
57+
{ok: true, a: math.MaxInt32, b: 0, expected: 0},
58+
{ok: true, a: math.MaxInt32, b: 1, expected: math.MaxInt32},
59+
{ok: true, a: math.MaxInt32, b: -1, expected: -math.MaxInt32},
60+
{ok: false, a: math.MaxInt32, b: -2, expected: 2},
61+
{ok: false, a: math.MaxInt32, b: 2, expected: -2},
62+
} {
63+
actual, ok := Mul(tt.a, tt.b)
64+
assert.Equal(t, tt.ok, ok, "(%v + %v)", tt.a, tt.b)
65+
require.Equal(t, tt.expected, actual, "(%v + %v)", tt.a, tt.b)
66+
}
67+
}

internal/utils/math_64bit_test.go

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing, software
12+
// distributed under the License is distributed on an "AS IS" BASIS,
13+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
// See the License for the specific language governing permissions and
15+
// limitations under the License.
16+
17+
//go:build amd64 || arm64 || mips64 || ppc64 || s390x
18+
19+
package utils
20+
21+
import (
22+
"math"
23+
"testing"
24+
25+
"github.com/stretchr/testify/assert"
26+
"github.com/stretchr/testify/require"
27+
)
28+
29+
func TestMul_64bit(t *testing.T) {
30+
// These constants are smaller in magnitude than the exact square root.
31+
assert.Greater(t, sqrtMinInt, int(math.Sqrt(math.MinInt64)))
32+
assert.Less(t, sqrtMaxInt, int(math.Sqrt(math.MaxInt64)))
33+
34+
for _, tt := range []struct {
35+
a, b, expected int
36+
ok bool
37+
}{
38+
{ok: true, a: math.MinInt32, b: 0, expected: 0},
39+
{ok: true, a: math.MinInt32, b: 1, expected: math.MinInt32},
40+
{ok: true, a: math.MinInt32, b: -1, expected: -math.MinInt32},
41+
42+
{ok: true, a: math.MaxInt32, b: 0, expected: 0},
43+
{ok: true, a: math.MaxInt32, b: 1, expected: math.MaxInt32},
44+
{ok: true, a: math.MaxInt32, b: -1, expected: -math.MaxInt32},
45+
46+
{ok: true, a: math.MinInt32, b: math.MinInt32, expected: 4611686018427387904},
47+
{ok: true, a: math.MaxInt32, b: math.MaxInt32, expected: 4611686014132420609},
48+
{ok: true, a: math.MinInt32, b: math.MaxInt32, expected: -4611686016279904256},
49+
{ok: true, a: math.MaxInt32, b: math.MinInt32, expected: -4611686016279904256},
50+
51+
{ok: true, a: math.MinInt64, b: 0, expected: 0},
52+
{ok: true, a: math.MinInt64, b: 1, expected: math.MinInt64},
53+
{ok: false, a: math.MinInt64, b: -1, expected: math.MinInt64},
54+
{ok: false, a: math.MinInt64, b: -2, expected: 0},
55+
{ok: false, a: math.MinInt64, b: 2, expected: 0},
56+
57+
{ok: true, a: math.MaxInt64, b: 0, expected: 0},
58+
{ok: true, a: math.MaxInt64, b: 1, expected: math.MaxInt64},
59+
{ok: true, a: math.MaxInt64, b: -1, expected: -math.MaxInt64},
60+
{ok: false, a: math.MaxInt64, b: -2, expected: 2},
61+
{ok: false, a: math.MaxInt64, b: 2, expected: -2},
62+
} {
63+
actual, ok := Mul(tt.a, tt.b)
64+
assert.Equal(t, tt.ok, ok, "(%v * %v)", tt.a, tt.b)
65+
require.Equal(t, tt.expected, actual, "(%v * %v)", tt.a, tt.b)
66+
}
67+
}

0 commit comments

Comments
 (0)