From 8c23f6f957d1c0bedd314806d1ac65bea59b084c Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Thu, 4 Aug 2022 01:27:54 -0700 Subject: [PATCH] perf: fix: tx/textual/valuerender: use io.WriteString to skip str->byteslice + fix negative sign dropping (#12815) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Noticed in an audit that the differeent value renderers perform an expensive and unnecessary string->byteslice in cases where the output write implements io.StringWriter. This change instead invokes io.WriteString(w, formatted) instead of: w.Write([]byte(formatted)) and added benchmarks that show an improvement from just the 1 line change: ```shell $ benchstat before.txt after.txt name old time/op new time/op delta IntValueRendererFormat-8 4.13µs ± 3% 3.95µs ± 6% -4.55% (p=0.000 n=15+14) BytesValueRendererFormat-8 5.22ms ± 3% 4.77ms ± 5% -8.60% (p=0.000 n=15+14) name old alloc/op new alloc/op delta IntValueRendererFormat-8 3.64kB ± 0% 3.31kB ± 0% -9.01% (p=0.000 n=15+15) BytesValueRendererFormat-8 12.6MB ± 0% 8.4MB ± 0% -33.22% (p=0.000 n=15+15) name old allocs/op new allocs/op delta IntValueRendererFormat-8 76.0 ± 0% 67.0 ± 0% -11.84% (p=0.000 n=15+15) BytesValueRendererFormat-8 27.0 ± 0% 18.0 ± 0% -33.33% (p=0.000 n=15+15) ``` While here, implemented negative sign preservation because previously the code wasn't tested for negative values so passing in negative values such as: "-10000000.11" would produce: "10'000'000.11" instead of the proper value with the negative sign preserved: "-10'000'000.11" Fixes #12810 Fixes #12812 --- tx/textual/internal/testdata/decimals.json | 6 +- tx/textual/valuerenderer/bench_test.go | 67 +++++++++++++++++++ tx/textual/valuerenderer/bytes.go | 4 +- tx/textual/valuerenderer/dec.go | 2 +- tx/textual/valuerenderer/int.go | 6 +- .../valuerenderer/valuerenderer_test.go | 19 +++--- 6 files changed, 90 insertions(+), 14 deletions(-) create mode 100644 tx/textual/valuerenderer/bench_test.go diff --git a/tx/textual/internal/testdata/decimals.json b/tx/textual/internal/testdata/decimals.json index 8893fef9e2..3564b597d9 100644 --- a/tx/textual/internal/testdata/decimals.json +++ b/tx/textual/internal/testdata/decimals.json @@ -39,5 +39,9 @@ ["0.000000000000001000", "0.000000000000001"], ["0.000000000000000100", "0.0000000000000001"], ["0.000000000000000010", "0.00000000000000001"], - ["0.000000000000000001", "0.000000000000000001"] + ["0.000000000000000001", "0.000000000000000001"], + ["-10.0", "-10"], + ["-10000", "-10'000"], + ["-9999", "-9'999"], + ["-999999999999", "-999'999'999'999"] ] diff --git a/tx/textual/valuerenderer/bench_test.go b/tx/textual/valuerenderer/bench_test.go new file mode 100644 index 0000000000..fd485fbd76 --- /dev/null +++ b/tx/textual/valuerenderer/bench_test.go @@ -0,0 +1,67 @@ +package valuerenderer + +import ( + "bytes" + "context" + "testing" + + "google.golang.org/protobuf/reflect/protoreflect" +) + +var intValues = []protoreflect.Value{ + protoreflect.ValueOfString("10.00"), + protoreflect.ValueOfString("999.00"), + protoreflect.ValueOfString("999.9999"), + protoreflect.ValueOfString("99999999.9999"), + protoreflect.ValueOfString("9999999999999999999"), + protoreflect.ValueOfString("1000000000000000000000000000000000000000000000000000000.00"), + protoreflect.ValueOfString("77777777777.777777777777777777777700"), + protoreflect.ValueOfString("-77777777777.777777777777777777777700"), + protoreflect.ValueOfString("777777777777777777777777.77777777700"), +} + +func BenchmarkIntValueRendererFormat(b *testing.B) { + ctx := context.Background() + ivr := new(intValueRenderer) + buf := new(bytes.Buffer) + b.ResetTimer() + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + for _, value := range intValues { + if err := ivr.Format(ctx, value, buf); err != nil { + b.Fatal(err) + } + } + buf.Reset() + } +} + +var byteValues = []protoreflect.Value{ + protoreflect.ValueOfBytes(bytes.Repeat([]byte("abc"), 1<<20)), + protoreflect.ValueOfBytes([]byte("999.00")), + protoreflect.ValueOfBytes([]byte("999.9999")), + protoreflect.ValueOfBytes([]byte("99999999.9999")), + protoreflect.ValueOfBytes([]byte("9999999999999999999")), + protoreflect.ValueOfBytes([]byte("1000000000000000000000000000000000000000000000000000000.00")), + protoreflect.ValueOfBytes([]byte("77777777777.777777777777777777777700")), + protoreflect.ValueOfBytes([]byte("-77777777777.777777777777777777777700")), + protoreflect.ValueOfBytes([]byte("777777777777777777777777.77777777700")), +} + +func BenchmarkBytesValueRendererFormat(b *testing.B) { + ctx := context.Background() + bvr := new(bytesValueRenderer) + buf := new(bytes.Buffer) + b.ResetTimer() + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + for _, value := range byteValues { + if err := bvr.Format(ctx, value, buf); err != nil { + b.Fatal(err) + } + } + buf.Reset() + } +} diff --git a/tx/textual/valuerenderer/bytes.go b/tx/textual/valuerenderer/bytes.go index 23ece74c94..2600c87be2 100644 --- a/tx/textual/valuerenderer/bytes.go +++ b/tx/textual/valuerenderer/bytes.go @@ -8,14 +8,14 @@ import ( "google.golang.org/protobuf/reflect/protoreflect" ) -//bytesValueRenderer implements ValueRenderer for bytes +// bytesValueRenderer implements ValueRenderer for bytes type bytesValueRenderer struct { } var _ ValueRenderer = bytesValueRenderer{} func (vr bytesValueRenderer) Format(ctx context.Context, v protoreflect.Value, w io.Writer) error { - _, err := w.Write([]byte(base64.StdEncoding.EncodeToString(v.Bytes()))) + _, err := io.WriteString(w, base64.StdEncoding.EncodeToString(v.Bytes())) return err } diff --git a/tx/textual/valuerenderer/dec.go b/tx/textual/valuerenderer/dec.go index 688ef73c23..5ea72e110d 100644 --- a/tx/textual/valuerenderer/dec.go +++ b/tx/textual/valuerenderer/dec.go @@ -21,7 +21,7 @@ func (vr decValueRenderer) Format(_ context.Context, v protoreflect.Value, w io. return err } - _, err = w.Write([]byte(formatted)) + _, err = io.WriteString(w, formatted) return err } diff --git a/tx/textual/valuerenderer/int.go b/tx/textual/valuerenderer/int.go index 2770cce13f..476dbe2b4b 100644 --- a/tx/textual/valuerenderer/int.go +++ b/tx/textual/valuerenderer/int.go @@ -18,7 +18,7 @@ func (vr intValueRenderer) Format(_ context.Context, v protoreflect.Value, w io. return err } - _, err = w.Write([]byte(formatted)) + _, err = io.WriteString(w, formatted) return err } @@ -30,7 +30,9 @@ func (vr intValueRenderer) Parse(_ context.Context, r io.Reader) (protoreflect.V // operates with string manipulation (instead of manipulating the int or sdk.Int // object). func formatInteger(v string) (string, error) { + sign := "" if v[0] == '-' { + sign = "-" v = v[1:] } if len(v) > 1 { @@ -44,5 +46,5 @@ func formatInteger(v string) (string, error) { v = v[:outputIndex] + thousandSeparator + v[outputIndex:] } - return v, nil + return sign + v, nil } diff --git a/tx/textual/valuerenderer/valuerenderer_test.go b/tx/textual/valuerenderer/valuerenderer_test.go index 36b399f2b0..9d53616745 100644 --- a/tx/textual/valuerenderer/valuerenderer_test.go +++ b/tx/textual/valuerenderer/valuerenderer_test.go @@ -73,15 +73,18 @@ func TestFormatDecimal(t *testing.T) { require.NoError(t, err) for _, tc := range testcases { - d, err := math.LegacyNewDecFromStr(tc[0]) - require.NoError(t, err) - r, err := valueRendererOf(d) - require.NoError(t, err) - b := new(strings.Builder) - err = r.Format(context.Background(), protoreflect.ValueOf(tc[0]), b) - require.NoError(t, err) + tc := tc + t.Run(tc[0], func(t *testing.T) { + d, err := math.LegacyNewDecFromStr(tc[0]) + require.NoError(t, err) + r, err := valueRendererOf(d) + require.NoError(t, err) + b := new(strings.Builder) + err = r.Format(context.Background(), protoreflect.ValueOf(tc[0]), b) + require.NoError(t, err) - require.Equal(t, tc[1], b.String()) + require.Equal(t, tc[1], b.String()) + }) } }