Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve performance of Abs() and Round() #240

Merged
merged 1 commit into from Sep 9, 2021
Merged

Conversation

DoubleDi
Copy link
Contributor

@DoubleDi DoubleDi commented Aug 8, 2021

This PR improves the performance of Abs() and Round() and is related to #208 .
The goal is to return the original value instead of creating a new one when:

  • value is not negative in Abs()
  • value already has exact precision in Round()

@DoubleDi
Copy link
Contributor Author

DoubleDi commented Sep 6, 2021

Hi @mwoss!
Sorry for the ping
Can you please check this and consider merging?

@mwoss
Copy link
Member

mwoss commented Sep 6, 2021

Hi @DoubleDi! Could you provide some benchmarks and approximate speed up?

@DoubleDi
Copy link
Contributor Author

DoubleDi commented Sep 7, 2021

@mwoss can send you benchmarks after some time but the improvements are using simple math.
Current Abs and Round always create a new value and makes allocations event if the value didn't change at all.
That is why i have added some math checks:

  1. in Abs if we already have a positive value, then we don't need to negate it and we can just return the original one.
  2. in Round if we already have a value with N paces that, equals the rounding parameter, that we do not need to do anything and we should return the original value.

This PR was made, when i saw, that over 20% of CPU in our production service, that was doing heavy calculations, was on the Round function.

@mwoss
Copy link
Member

mwoss commented Sep 8, 2021

I got it, but I was wondering what impact it has on other (typical) use cases. Usually Abs are called on negative values only, Round also is not used to round decimal to the exact same places after decimal point.
Those two if-statements should not have much impact on performance, but it's always better to analyze it.

@DoubleDi
Copy link
Contributor Author

DoubleDi commented Sep 8, 2021

For example a typica usecase:
You have unmarshalled a decimal.Decimal for a database or JSON.
You actually not sure is it positive or not, so you do:

v = v.Abs()  // this line creates a new big.Int

so if you want to optimize you do

if v.IsNegative() {
   v = v.Abs()
}

Same logic applies to Round (IsNegative() <=> Exp() == -n)

Benchmark for passing positive/zero/negative values to Abs()

package main

import (
	"testing"

	"github.com/shopspring/decimal"
)

var (
	p   = decimal.New(1, 0)
	z   = decimal.Zero
	n   = decimal.New(-1, 0)
	all = []decimal.Decimal{p, z, n}
)

func BenchmarkAbs(b *testing.B) {
	b.ReportAllocs()

	for i := 0; i < b.N; i++ {
		_ = all[i%3].Abs()
	}
}

func BenchmarkNeg(b *testing.B) {
	b.ReportAllocs()

	for i := 0; i < b.N; i++ {
		x := all[i%3]
		if x.IsNegative() {
			_ = x.Abs()
		}
	}
}

Results:

goos: darwin
goarch: amd64
pkg: tmp
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkAbs-16    	24514900	        45.35 ns/op	      37 B/op	       1 allocs/op
BenchmarkNeg-16    	64578939	        18.39 ns/op	      13 B/op	       0 allocs/op
PASS

@mwoss
Copy link
Member

mwoss commented Sep 9, 2021

Thanks for the detailed explanation and benchmarks @DoubleDi!
I also checked it for myself, and there is almost no impact on performance by adding those two - if statements if the values are positive (abs) or if the exp is different than places argument (round).

Copy link
Member

@mwoss mwoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good :3

@mwoss
Copy link
Member

mwoss commented Sep 9, 2021

Thanks for your contribution @DoubleDi! I'm merging this PR to main branch.

@mwoss mwoss merged commit 483a047 into shopspring:master Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants