util/decimal: decrease allowed shift length

pkg/util/decimal/decimal.go

@@ -333,8 +333,15 @@ func Log(z *inf.Dec, x *inf.Dec, s inf.Scale) (*inf.Dec, error) {
 
 	// maxExp10 is the maximum number of digits exp10 is expected to return. Above
 	// this it becomes very slow. However since the implementation for that lies
-	// in the inf package, we instead check the length in this function.
-	const maxExp10 = 500000
+	// in the inf package, we instead check the length in this function. The
+	// tests in TestDecimalLogN using strE (a very high-precision E value)
+	// need just under 350000 to complete. They, however, could easily add
+	// more precision because, although the shift value is high, it converges
+	// quickly. If we need to improve this heuristic in the future (which is
+	// designed to prevent slowness only), perhays maxExp10 should decrease as
+	// loop.i increases. This would allow fast-converging calculations to have high
+	// precision and prevent slow-converging calculations from taking all the CPU.
+	const maxExp10 = 350000
 
 	// Loop over the Maclaurin series given above until convergence.
 	for loop := newLoop("log", x, s, 40); ; {

pkg/util/decimal/decimal_test.go

@@ -17,13 +17,16 @@
 package decimal
 
 import (
+	"fmt"
 	"math"
 	"testing"
+	"time"
 
 	"gopkg.in/inf.v0"
 
 	_ "github.com/cockroachdb/cockroach/pkg/util/log" // for flags
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
+	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 )
 
 var floatDecimalEqualities = map[float64]*inf.Dec{
@@ -80,36 +83,51 @@ func testDecimalSingleArgFunc(
 	s inf.Scale,
 	tests []decimalOneArgTestCase,
 ) {
-	for i, tc := range tests {
-		x, exp := new(inf.Dec), new(inf.Dec)
-		x.SetString(tc.input)
-		exp.SetString(tc.expected)
-
-		// Test allocated return value.
-		z, err := f(nil, x, s)
-		if err != nil {
-			if tc.expected != err.Error() {
-				t.Errorf("%d: expected error %s, got %s", i, tc.expected, err)
+	for _, tc := range tests {
+		t.Run(fmt.Sprintf("%s = %s", tc.input, tc.expected), func(t *testing.T) {
+			x, exp := new(inf.Dec), new(inf.Dec)
+			x.SetString(tc.input)
+			exp.SetString(tc.expected)
+
+			// Test allocated return value.
+			var z *inf.Dec
+			var err error
+			done := make(chan struct{}, 1)
+			start := timeutil.Now()
+			go func() {
+				z, err = f(nil, x, s)
+				done <- struct{}{}
+			}()
+			select {
+			case <-done:
+				t.Logf("execute duration: %s", timeutil.Since(start))
+			case <-time.After(testFuncTimeout):
+				t.Fatalf("timedout after %s", testFuncTimeout)
+			}
+			if err != nil {
+				if tc.expected != err.Error() {
+					t.Errorf("expected error %s, got %s", tc.expected, err)
+				}
+				return
+			}
+			if exp.Cmp(z) != 0 {
+				t.Errorf("expected %s, got %s", exp, z)
 			}
-			continue
-		}
-		if exp.Cmp(z) != 0 {
-			t.Errorf("%d: expected %s, got %s", i, exp, z)
-		}
 
-		// Test provided decimal mutation.
-		z.SetString("0.0")
-		_, _ = f(z, x, s)
-		if exp.Cmp(z) != 0 {
-			t.Errorf("%d: expected %s, got %s", i, exp, z)
-		}
+			// Test provided decimal mutation.
+			z.SetString("0.0")
+			_, _ = f(z, x, s)
+			if exp.Cmp(z) != 0 {
+				t.Errorf("expected %s, got %s", exp, z)
+			}
 
-		// Test same arg mutation.
-		_, _ = f(x, x, s)
-		if exp.Cmp(x) != 0 {
-			t.Errorf("%d: expected %s, got %s", i, exp, x)
-		}
-		x.SetString(tc.input)
+			// Test same arg mutation.
+			_, _ = f(x, x, s)
+			if exp.Cmp(x) != 0 {
+				t.Errorf("expected %s, got %s", exp, x)
+			}
+			x.SetString(tc.input)
+		})
 	}
 }
 
@@ -135,71 +153,84 @@ func testDecimalDoubleArgFunc(
 	s inf.Scale,
 	tests []decimalTwoArgsTestCase,
 ) {
-	for i, tc := range tests {
-		x, y, exp := new(inf.Dec), new(inf.Dec), new(inf.Dec)
-		if _, ok := x.SetString(tc.input1); !ok {
-			t.Errorf("could not set decimal: %s", tc.input1)
-			continue
-		}
-		if _, ok := y.SetString(tc.input2); !ok {
-			t.Errorf("could not set decimal: %s", tc.input2)
-			continue
-		}
-
-		// Test allocated return value.
-		z, err := f(nil, x, y, s)
-		if err != nil {
-			if tc.expected != err.Error() {
-				t.Errorf("%d: expected error %s, got %s", i, tc.expected, err)
+	for _, tc := range tests {
+		t.Run(fmt.Sprintf("%s, %s = %s", tc.input1, tc.input2, tc.expected), func(t *testing.T) {
+			x, y, exp := new(inf.Dec), new(inf.Dec), new(inf.Dec)
+			if _, ok := x.SetString(tc.input1); !ok {
+				t.Fatalf("could not set decimal: %s", tc.input1)
 			}
-			continue
-		}
-		if z == nil {
-			if tc.expected != "nil" {
-				t.Errorf("%d: expected %s, got nil", i, tc.expected)
+			if _, ok := y.SetString(tc.input2); !ok {
+				t.Fatalf("could not set decimal: %s", tc.input2)
 			}
-			continue
-		} else if tc.expected == "nil" {
-			t.Errorf("%d: expected nil, got %s", i, z)
-			continue
-		}
-		if _, ok := exp.SetString(tc.expected); !ok {
-			t.Errorf("%d: could not set decimal: %s", i, tc.expected)
-			continue
-		}
-		if exp.Cmp(z) != 0 {
-			t.Errorf("%d: expected %s, got %s", i, exp, z)
-		}
-
-		// Test provided decimal mutation.
-		z.SetString("0.0")
-		_, _ = f(z, x, y, s)
-		if exp.Cmp(z) != 0 {
-			t.Errorf("%d: expected %s, got %s", i, exp, z)
-		}
 
-		// Test first arg mutation.
-		_, _ = f(x, x, y, s)
-		if exp.Cmp(x) != 0 {
-			t.Errorf("%d: expected %s, got %s", i, exp, x)
-		}
-		x.SetString(tc.input1)
+			// Test allocated return value.
+			var z *inf.Dec
+			var err error
+			done := make(chan struct{}, 1)
+			start := timeutil.Now()
+			go func() {
+				z, err = f(nil, x, y, s)
+				done <- struct{}{}
+			}()
+			select {
+			case <-done:
+				t.Logf("execute duration: %s", timeutil.Since(start))
+			case <-time.After(testFuncTimeout):
+				t.Fatalf("timedout after %s", testFuncTimeout)
+			}
+			if err != nil {
+				if tc.expected != err.Error() {
+					t.Errorf("expected error %s, got %s", tc.expected, err)
+				}
+				return
+			}
+			if z == nil {
+				if tc.expected != "nil" {
+					t.Errorf("expected %s, got nil", tc.expected)
+				}
+				return
+			} else if tc.expected == "nil" {
+				t.Errorf("expected nil, got %s", z)
+				return
+			}
+			if _, ok := exp.SetString(tc.expected); !ok {
+				t.Errorf("could not set decimal: %s", tc.expected)
+				return
+			}
+			if exp.Cmp(z) != 0 {
+				t.Errorf("expected %s, got %s", exp, z)
+			}
 
-		// Test second arg mutation.
-		_, _ = f(y, x, y, s)
-		if exp.Cmp(y) != 0 {
-			t.Errorf("%d: expected %s, got %s", i, exp, y)
-		}
-		y.SetString(tc.input2)
+			// Test provided decimal mutation.
+			z.SetString("0.0")
+			_, _ = f(z, x, y, s)
+			if exp.Cmp(z) != 0 {
+				t.Errorf("expected %s, got %s", exp, z)
+			}
 
-		// Test both arg mutation, if possible.
-		if tc.input1 == tc.input2 {
-			_, _ = f(x, x, x, s)
+			// Test first arg mutation.
+			_, _ = f(x, x, y, s)
 			if exp.Cmp(x) != 0 {
-				t.Errorf("%d: expected %s, got %s", i, exp, x)
+				t.Errorf("expected %s, got %s", exp, x)
 			}
 			x.SetString(tc.input1)
-		}
+
+			// Test second arg mutation.
+			_, _ = f(y, x, y, s)
+			if exp.Cmp(y) != 0 {
+				t.Errorf("expected %s, got %s", exp, y)
+			}
+			y.SetString(tc.input2)
+
+			// Test both arg mutation, if possible.
+			if tc.input1 == tc.input2 {
+				_, _ = f(x, x, x, s)
+				if exp.Cmp(x) != 0 {
+					t.Errorf("expected %s, got %s", exp, x)
+				}
+				x.SetString(tc.input1)
+			}
+		})
 	}
 }
 
@@ -540,6 +571,9 @@ func TestDecimalPow(t *testing.T) {
 		{"2", "-38", "argument too large"},
 		{"10000000000", "500", "argument too large"},
 		{"425644047350.89246", "74.4647211651881", "argument too large"},
+		{"56051.85009165843", "98.23741371063426", "argument too large"},
+		{"2306257620454.719", "49.18687811476825", "argument too large"},
+		{"791018.4038517432", "155.94813858582634", "argument too large"},
 	}
 	testDecimalDoubleArgFunc(t, Pow, 16, tests)
 }
@@ -564,6 +598,9 @@ func TestDecimalPowDoubleScale(t *testing.T) {
 		{"2", "-38", "0.000000000004"},
 		{"10000000000", "500", "argument too large"},
 		{"425644047350.89246", "74.4647211651881", "argument too large"},
+		{"56051.85009165843", "98.23741371063426", "argument too large"},
+		{"2306257620454.719", "49.18687811476825", "argument too large"},
+		{"791018.4038517432", "155.94813858582634", "argument too large"},
 	}
 	testDecimalDoubleArgFunc(t, Pow, 32, tests)
 }

pkg/util/decimal/no_race_test.go

@@ -0,0 +1,21 @@
+// Copyright 2016 The Cockroach Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+// implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+// +build !race
+
+package decimal
+
+import "time"
+
+const testFuncTimeout = time.Second * 5

pkg/util/decimal/race_test.go

@@ -0,0 +1,21 @@
+// Copyright 2016 The Cockroach Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+// implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+// +build race
+
+package decimal
+
+import "time"
+
+const testFuncTimeout = time.Minute