Round should avoid clearing s, save a string

Instead of saving bytes, save a string, which makes String() faster
and does not unduly penalize marshal. During parse, save the string
if it is in canonical form.

pkg/api/resource/deep_copy.go

@@ -1,5 +1,3 @@
-// +build !ignore_autogenerated
-
 /*
 Copyright 2016 The Kubernetes Authors All rights reserved.
 
@@ -16,32 +14,19 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-// This file was autogenerated by deepcopy-gen. Do not edit it manually!
-
 package resource
 
 import (
+	inf "gopkg.in/inf.v0"
+
 	conversion "k8s.io/kubernetes/pkg/conversion"
 )
 
 func DeepCopy_resource_Quantity(in Quantity, out *Quantity, c *conversion.Cloner) error {
-	if newVal, err := c.DeepCopy(in.i); err != nil {
-		return err
-	} else {
-		out.i = newVal.(int64Amount)
+	*out = in
+	if in.d.Dec != nil {
+		tmp := &inf.Dec{}
+		out.d.Dec = tmp.Set(in.d.Dec)
 	}
-	if newVal, err := c.DeepCopy(in.d); err != nil {
-		return err
-	} else {
-		out.d = newVal.(infDecAmount)
-	}
-	if in.s != nil {
-		in, out := in.s, &out.s
-		*out = make([]byte, len(in))
-		copy(*out, in)
-	} else {
-		out.s = nil
-	}
-	out.Format = in.Format
 	return nil
 }

pkg/api/resource/quantity.go

@@ -97,7 +97,7 @@ type Quantity struct {
 	// d is the quantity in inf.Dec form if d.Dec != nil
 	d infDecAmount
 	// s is the generated value of this quantity to avoid recalculation
-	s []byte
+	s string
 
 	// Change Format at will. See the comment for Canonicalize for
 	// more details.
@@ -275,7 +275,7 @@ func ParseQuantity(str string) (Quantity, error) {
 		return Quantity{}, ErrFormatWrong
 	}
 	if str == "0" {
-		return Quantity{Format: DecimalSI}, nil
+		return Quantity{Format: DecimalSI, s: str}, nil
 	}
 
 	positive, value, num, denom, suf, err := parseQuantityString(str)
@@ -324,6 +324,17 @@ func ParseQuantity(str string) (Quantity, error) {
 				if !positive {
 					result = -result
 				}
+				// if the number is in canonical form, reuse the string
+				switch format {
+				case BinarySI:
+					if exponent%10 == 0 && (value&0x07 != 0) {
+						return Quantity{i: int64Amount{value: result, scale: Scale(scale)}, Format: format, s: str}, nil
+					}
+				default:
+					if scale%3 == 0 && !strings.HasSuffix(shifted, "000") && shifted[0] != '0' {
+						return Quantity{i: int64Amount{value: result, scale: Scale(scale)}, Format: format, s: str}, nil
+					}
+				}
 				return Quantity{i: int64Amount{value: result, scale: Scale(scale)}, Format: format}, nil
 			}
 		}
@@ -490,10 +501,16 @@ func (q *Quantity) AsScale(scale Scale) (CanonicalValue, bool) {
 // Negative numbers are rounded away from zero (-9 scale 1 rounds to -10).
 func (q *Quantity) RoundUp(scale Scale) bool {
 	if q.d.Dec != nil {
+		q.s = ""
 		d, exact := q.d.AsScale(scale)
 		q.d = d
 		return exact
 	}
+	// avoid clearing the string value if we have already calculated it
+	if q.i.scale >= scale {
+		return true
+	}
+	q.s = ""
 	i, exact := q.i.AsScale(scale)
 	q.i = i
 	return exact
@@ -502,7 +519,7 @@ func (q *Quantity) RoundUp(scale Scale) bool {
 // Add adds the provide y quantity to the current value. If the current value is zero,
 // the format of the quantity will be updated to the format of y.
 func (q *Quantity) Add(y Quantity) {
-	q.s = nil
+	q.s = ""
 	if q.d.Dec == nil && y.d.Dec == nil {
 		if q.i.value == 0 {
 			q.Format = y.Format
@@ -519,7 +536,7 @@ func (q *Quantity) Add(y Quantity) {
 // Sub subtracts the provided quantity from the current value in place. If the current
 // value is zero, the format of the quantity will be updated to the format of y.
 func (q *Quantity) Sub(y Quantity) {
-	q.s = nil
+	q.s = ""
 	if q.IsZero() {
 		q.Format = y.Format
 	}
@@ -549,7 +566,7 @@ func (q *Quantity) CmpInt64(y int64) int {
 
 // Neg sets quantity to be the negative value of itself.
 func (q *Quantity) Neg() {
-	q.s = nil
+	q.s = ""
 	if q.d.Dec == nil {
 		q.i.value = -q.i.value
 		return
@@ -557,31 +574,26 @@ func (q *Quantity) Neg() {
 	q.d.Dec.Neg(q.d.Dec)
 }
 
-// toBytes ensures q.s is set to a byte slice representing the canonical string form of this
-// quantity and then returns the value. CanonicalizeBytes is an expensive operation, and caching
-// this result significantly reduces the cost of normal parse / marshal operations on Quantity.
-func (q *Quantity) toBytes() []byte {
-	if q.s == nil {
-		result := make([]byte, 0, int64QuantityExpectedBytes)
-		number, suffix := q.CanonicalizeBytes(result)
-		number = append(number, suffix...)
-		q.s = number
-	}
-	return q.s
-}
-
 // int64QuantityExpectedBytes is the expected width in bytes of the canonical string representation
 // of most Quantity values.
 const int64QuantityExpectedBytes = 18
 
-// String formats the Quantity as a string.
+// String formats the Quantity as a string, caching the result if not calculated.
+// String is an expensive operation and caching this result significantly reduces the cost of
+// normal parse / marshal operations on Quantity.
 func (q *Quantity) String() string {
-	return string(q.toBytes())
+	if len(q.s) == 0 {
+		result := make([]byte, 0, int64QuantityExpectedBytes)
+		number, suffix := q.CanonicalizeBytes(result)
+		number = append(number, suffix...)
+		q.s = string(number)
+	}
+	return q.s
 }
 
 // MarshalJSON implements the json.Marshaller interface.
 func (q Quantity) MarshalJSON() ([]byte, error) {
-	if q.s != nil {
+	if len(q.s) > 0 {
 		out := make([]byte, len(q.s)+2)
 		out[0], out[len(out)-1] = '"', '"'
 		copy(out[1:], q.s)
@@ -620,11 +632,12 @@ func (q *Quantity) UnmarshalJSON(value []byte) error {
 	if value[0] == '"' && value[l-1] == '"' {
 		value = value[1 : l-1]
 	}
+
 	parsed, err := ParseQuantity(string(value))
 	if err != nil {
 		return err
 	}
-	parsed.s = value
+
 	// This copy is safe because parsed will not be referred to again.
 	*q = parsed
 	return nil
@@ -693,7 +706,7 @@ func (q *Quantity) SetMilli(value int64) {
 
 // SetScaled sets q's value to be value * 10^scale
 func (q *Quantity) SetScaled(value int64, scale Scale) {
-	q.s = nil
+	q.s = ""
 	q.d.Dec = nil
 	q.i = int64Amount{value: value, scale: scale}
 }

pkg/api/resource/quantity_proto.go

@@ -46,7 +46,7 @@ func (m *Quantity) MarshalTo(data []byte) (int, error) {
 	data[i] = 0xa
 	i++
 	// BEGIN CUSTOM MARSHAL
-	out := m.toBytes()
+	out := m.String()
 	i = encodeVarintGenerated(data, i, uint64(len(out)))
 	i += copy(data[i:], out)
 	// END CUSTOM MARSHAL
@@ -69,7 +69,7 @@ func (m *Quantity) Size() (n int) {
 	_ = l
 
 	// BEGIN CUSTOM SIZE
-	l = len(m.toBytes())
+	l = len(m.String())
 	// END CUSTOM SIZE
 
 	n += 1 + l + sovGenerated(uint64(l))

pkg/api/resource/quantity_test.go

@@ -625,55 +625,80 @@ func TestQuantityString(t *testing.T) {
 	table := []struct {
 		in        Quantity
 		expect    string
+		alternate string
 	}{
-		{decQuantity(1024*1024*1024, 0, BinarySI), "1Gi"},
-		{decQuantity(300*1024*1024, 0, BinarySI), "300Mi"},
-		{decQuantity(6*1024, 0, BinarySI), "6Ki"},
-		{decQuantity(1001*1024*1024*1024, 0, BinarySI), "1001Gi"},
-		{decQuantity(1024*1024*1024*1024, 0, BinarySI), "1Ti"},
-		{decQuantity(5, 0, BinarySI), "5"},
-		{decQuantity(500, -3, BinarySI), "500m"},
-		{decQuantity(1, 9, DecimalSI), "1G"},
-		{decQuantity(1000, 6, DecimalSI), "1G"},
-		{decQuantity(1000000, 3, DecimalSI), "1G"},
-		{decQuantity(1000000000, 0, DecimalSI), "1G"},
-		{decQuantity(1, -3, DecimalSI), "1m"},
-		{decQuantity(80, -3, DecimalSI), "80m"},
-		{decQuantity(1080, -3, DecimalSI), "1080m"},
-		{decQuantity(108, -2, DecimalSI), "1080m"},
-		{decQuantity(10800, -4, DecimalSI), "1080m"},
-		{decQuantity(300, 6, DecimalSI), "300M"},
-		{decQuantity(1, 12, DecimalSI), "1T"},
-		{decQuantity(1234567, 6, DecimalSI), "1234567M"},
-		{decQuantity(1234567, -3, BinarySI), "1234567m"},
-		{decQuantity(3, 3, DecimalSI), "3k"},
-		{decQuantity(1025, 0, BinarySI), "1025"},
-		{decQuantity(0, 0, DecimalSI), "0"},
-		{decQuantity(0, 0, BinarySI), "0"},
-		{decQuantity(1, 9, DecimalExponent), "1e9"},
-		{decQuantity(1, -3, DecimalExponent), "1e-3"},
-		{decQuantity(1, -9, DecimalExponent), "1e-9"},
-		{decQuantity(80, -3, DecimalExponent), "80e-3"},
-		{decQuantity(300, 6, DecimalExponent), "300e6"},
-		{decQuantity(1, 12, DecimalExponent), "1e12"},
-		{decQuantity(1, 3, DecimalExponent), "1e3"},
-		{decQuantity(3, 3, DecimalExponent), "3e3"},
-		{decQuantity(3, 3, DecimalSI), "3k"},
-		{decQuantity(0, 0, DecimalExponent), "0"},
-		{decQuantity(1, -9, DecimalSI), "1n"},
-		{decQuantity(80, -9, DecimalSI), "80n"},
-		{decQuantity(1080, -9, DecimalSI), "1080n"},
-		{decQuantity(108, -8, DecimalSI), "1080n"},
-		{decQuantity(10800, -10, DecimalSI), "1080n"},
-		{decQuantity(1, -6, DecimalSI), "1u"},
-		{decQuantity(80, -6, DecimalSI), "80u"},
-		{decQuantity(1080, -6, DecimalSI), "1080u"},
+		{decQuantity(1024*1024*1024, 0, BinarySI), "1Gi", "1024Mi"},
+		{decQuantity(300*1024*1024, 0, BinarySI), "300Mi", "307200Ki"},
+		{decQuantity(6*1024, 0, BinarySI), "6Ki", ""},
+		{decQuantity(1001*1024*1024*1024, 0, BinarySI), "1001Gi", "1025024Mi"},
+		{decQuantity(1024*1024*1024*1024, 0, BinarySI), "1Ti", "1024Gi"},
+		{decQuantity(5, 0, BinarySI), "5", "5000m"},
+		{decQuantity(500, -3, BinarySI), "500m", "0.5"},
+		{decQuantity(1, 9, DecimalSI), "1G", "1000M"},
+		{decQuantity(1000, 6, DecimalSI), "1G", "0.001T"},
+		{decQuantity(1000000, 3, DecimalSI), "1G", ""},
+		{decQuantity(1000000000, 0, DecimalSI), "1G", ""},
+		{decQuantity(1, -3, DecimalSI), "1m", "1000u"},
+		{decQuantity(80, -3, DecimalSI), "80m", ""},
+		{decQuantity(1080, -3, DecimalSI), "1080m", "1.08"},
+		{decQuantity(108, -2, DecimalSI), "1080m", "1080000000n"},
+		{decQuantity(10800, -4, DecimalSI), "1080m", ""},
+		{decQuantity(300, 6, DecimalSI), "300M", ""},
+		{decQuantity(1, 12, DecimalSI), "1T", ""},
+		{decQuantity(1234567, 6, DecimalSI), "1234567M", ""},
+		{decQuantity(1234567, -3, BinarySI), "1234567m", ""},
+		{decQuantity(3, 3, DecimalSI), "3k", ""},
+		{decQuantity(1025, 0, BinarySI), "1025", ""},
+		{decQuantity(0, 0, DecimalSI), "0", ""},
+		{decQuantity(0, 0, BinarySI), "0", ""},
+		{decQuantity(1, 9, DecimalExponent), "1e9", ".001e12"},
+		{decQuantity(1, -3, DecimalExponent), "1e-3", "0.001e0"},
+		{decQuantity(1, -9, DecimalExponent), "1e-9", "1000e-12"},
+		{decQuantity(80, -3, DecimalExponent), "80e-3", ""},
+		{decQuantity(300, 6, DecimalExponent), "300e6", ""},
+		{decQuantity(1, 12, DecimalExponent), "1e12", ""},
+		{decQuantity(1, 3, DecimalExponent), "1e3", ""},
+		{decQuantity(3, 3, DecimalExponent), "3e3", ""},
+		{decQuantity(3, 3, DecimalSI), "3k", ""},
+		{decQuantity(0, 0, DecimalExponent), "0", "00"},
+		{decQuantity(1, -9, DecimalSI), "1n", ""},
+		{decQuantity(80, -9, DecimalSI), "80n", ""},
+		{decQuantity(1080, -9, DecimalSI), "1080n", ""},
+		{decQuantity(108, -8, DecimalSI), "1080n", ""},
+		{decQuantity(10800, -10, DecimalSI), "1080n", ""},
+		{decQuantity(1, -6, DecimalSI), "1u", ""},
+		{decQuantity(80, -6, DecimalSI), "80u", ""},
+		{decQuantity(1080, -6, DecimalSI), "1080u", ""},
 	}
 	for _, item := range table {
 		got := item.in.String()
 		if e, a := item.expect, got; e != a {
 			t.Errorf("%#v: expected %v, got %v", item.in, e, a)
 		}
+		q, err := ParseQuantity(item.expect)
+		if err != nil {
+			t.Errorf("%#v: unexpected error: %v", item.expect, err)
+		}
+		if len(q.s) == 0 || q.s != item.expect {
+			t.Errorf("%#v: did not copy canonical string on parse: %s", item.expect, q.s)
+		}
+		if len(item.alternate) == 0 {
+			continue
+		}
+		q, err = ParseQuantity(item.alternate)
+		if err != nil {
+			t.Errorf("%#v: unexpected error: %v", item.expect, err)
+			continue
+		}
+		if len(q.s) != 0 {
+			t.Errorf("%#v: unexpected nested string: %v", item.expect, q.s)
+		}
+		if q.String() != item.expect {
+			t.Errorf("%#v: unexpected alternate canonical: %v", item.expect, q.String())
+		}
+		if len(q.s) == 0 || q.s != item.expect {
+			t.Errorf("%#v: did not set canonical string on ToString: %s", item.expect, q.s)
+		}
 	}
 	desired := &inf.Dec{} // Avoid modifying the values in the table.
 	for _, item := range table {
@@ -1126,7 +1151,24 @@ func BenchmarkQuantityString(b *testing.B) {
 	var s string
 	for i := 0; i < b.N; i++ {
 		q := values[i%len(values)]
-		q.s = nil
+		q.s = ""
+		s = q.String()
+	}
+	b.StopTimer()
+	if len(s) == 0 {
+		b.Fatal(s)
+	}
+}
+
+func BenchmarkQuantityStringPrecalc(b *testing.B) {
+	values := benchmarkQuantities()
+	for i := range values {
+		_ = values[i].String()
+	}
+	b.ResetTimer()
+	var s string
+	for i := 0; i < b.N; i++ {
+		q := values[i%len(values)]
 		s = q.String()
 	}
 	b.StopTimer()
@@ -1144,7 +1186,7 @@ func BenchmarkQuantityStringBinarySI(b *testing.B) {
 	var s string
 	for i := 0; i < b.N; i++ {
 		q := values[i%len(values)]
-		q.s = nil
+		q.s = ""
 		s = q.String()
 	}
 	b.StopTimer()
@@ -1158,7 +1200,7 @@ func BenchmarkQuantityMarshalJSON(b *testing.B) {
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
 		q := values[i%len(values)]
-		q.s = nil
+		q.s = ""
 		if _, err := q.MarshalJSON(); err != nil {
 			b.Fatal(err)
 		}