apd: Improve SetFloat64 efficiency

[miretskiy]

Prefer to use append and allocate scratch space to improve allocation efficiency of SetFloat64.

name         old time/op    new time/op    delta
SetFloat-10     261ns ± 0%     243ns ± 1%   -6.66%  (p=0.000 n=9+9)

name         old alloc/op   new alloc/op   delta
SetFloat-10     91.0B ± 0%     67.0B ± 0%  -26.37%  (p=0.000 n=10+10)

name         old allocs/op  new allocs/op  delta
SetFloat-10      4.00 ± 0%      3.00 ± 0%  -25.00%  (p=0.000 n=10+10)

[nvanbenschoten]

The []byte to string allocation is unfortunate. Do you want to throw the following into an unsafe.go file and use it here?

// Copyright 2023 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.

package apd

import "unsafe"

func unsafeConvertBytesToString(b []byte) string {
	return *(*string)(unsafe.Pointer(&b))
}

[miretskiy]

I think you looked at it before I backed out of this approach.

[nvanbenschoten]

What do you mean by that? You had the unsafe cast and then removed it?

[miretskiy]

It's kinda strange... I think if you look at the string converstion that's done inside bytes.HasPrefix(...) the comment there says that string([]byte) conversion does not incur copy when using various compilers. Perhaps that's what's at play here and might be the reason why the unsafeGetString approach is both slower and allocates more.

[miretskiy]

Sorry, I misunderstood. I actually had that, and it appears the the performance is worse when using this unsafe hack: pkg: github.com/cockroachdb/apd/v3 BenchmarkSetFloat-10 4642030 242.8 ns/op 75 B/op 3 allocs/op BenchmarkSetFloat-10 4793172 246.3 ns/op 75 B/op 3 allocs/op BenchmarkSetFloat-10 4975635 243.0 ns/op 75 B/op 3 allocs/op BenchmarkSetFloat-10 4903842 244.0 ns/op 75 B/op 3 allocs/op BenchmarkSetFloat-10 4940126 240.5 ns/op 75 B/op 3 allocs/op BenchmarkSetFloat-10 4915200 243.6 ns/op 75 B/op 3 allocs/op BenchmarkSetFloat-10 4970551 241.4 ns/op 75 B/op 3 allocs/op BenchmarkSetFloat-10 4978546 242.4 ns/op 75 B/op 3 allocs/op BenchmarkSetFloat-10 4885584 241.9 ns/op 75 B/op 3 allocs/op BenchmarkSetFloat-10 4995925 240.9 ns/op 75 B/op 3 allocs/op PASS ok github.com/cockroachdb/apd/v3 14.862s ~/go/src/github.com/cockroachdb/apd | decimal !1 go test -run="nothingmatches" -bench=BenchmarkSetFloat -count=10 2>&1 | tee /tmp/old.txt ok | 16s goos: darwin goarch: arm64 pkg: github.com/cockroachdb/apd/v3 BenchmarkSetFloat-10 4610048 246.9 ns/op 67 B/op 3 allocs/op BenchmarkSetFloat-10 4849850 246.2 ns/op 67 B/op 3 allocs/op BenchmarkSetFloat-10 4850758 245.9 ns/op 67 B/op 3 allocs/op BenchmarkSetFloat-10 4890546 249.4 ns/op 67 B/op 3 allocs/op BenchmarkSetFloat-10 4724228 249.1 ns/op 67 B/op 3 allocs/op BenchmarkSetFloat-10 4828840 246.6 ns/op 67 B/op 3 allocs/op BenchmarkSetFloat-10 4843838 250.1 ns/op 67 B/op 3 allocs/op BenchmarkSetFloat-10 4808347 250.0 ns/op 67 B/op 3 allocs/op BenchmarkSetFloat-10 4612788 251.4 ns/op 67 B/op 3 allocs/op BenchmarkSetFloat-10 4814672 248.9 ns/op 67 B/op 3 allocs/op PASS ok github.com/cockroachdb/apd/v3 14.824s ~/go/src/github.com/cockroachdb/apd | decimal !1 benchstat /tmp/old.txt /tmp/new.txt ok | 15s name old time/op new time/op delta SetFloat-10 248ns ± 1% 243ns ± 1% -2.32% (p=0.000 n=10+10) name old alloc/op new alloc/op delta SetFloat-10 67.0B ± 0% 75.0B ± 0% +11.94% (p=0.000 n=10+10) name old allocs/op new allocs/op delta SetFloat-10 3.00 ± 0% 3.00 ± 0% ~ (all equal)

…

On Fri, Jul 14, 2023 at 1:14 PM Yevgeniy Miretskiy ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In decimal.go <#129 (comment)>: > @@ -241,7 +242,8 @@ func (d *Decimal) setCoefficient(x int64) { // SetFloat64 sets d's Coefficient and Exponent to x and returns d. d will // hold the exact value of f. func (d *Decimal) SetFloat64(f float64) (*Decimal, error) { - _, _, err := d.SetString(strconv.FormatFloat(f, 'E', -1, 64)) + var buf [32]byte // Avoid most of the allocations in strconv. + _, _, err := d.SetString(string(strconv.AppendFloat(buf[:0], f, 'E', -1, 64))) I think you looked at it before I backed out of this approach. — Reply to this email directly, view it on GitHub <#129 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANA4FVB5535XAZZY6SVFHZTXQF47VANCNFSM6AAAAAA2KQC5L4> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[nvanbenschoten]

LGTM

[nvanbenschoten]

nit: BenchmarkDecimalSetFloat because this is a method on Decimal.

[miretskiy]

Done.