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

Remove unnecessary decimal rescaling - memory usage optimization #160

Merged
merged 5 commits into from
Jan 5, 2020

Conversation

mwoss
Copy link
Member

@mwoss mwoss commented Dec 29, 2019

I've tried to fix the performance issue mentioned in #81. Partially succeed as I couldn't simply return an already existing decimal object in rescale method due to some problems with DivMod pointer receiver method that change decimal's value attribute. As a workaround, I've created RescalePair helper function that rescales only one or none decimals from pair to avoid unnecessary memory allocations.

Results of optimization shown below.

I've used benchmarks implemented by @ngalaiko in PR #102. Thanks for the contributions to the library, I really appreciate it.

Before:

Benchmark_decimal_Decimal_Add_different_precision-8      3753075               317 ns/op             605 B/op         12 allocs/op
Benchmark_decimal_Decimal_Sub_different_precision-8      3879166               308 ns/op             557 B/op         12 allocs/op
Benchmark_math_big_Float_Sub_different_precision-8      372282800                3.24 ns/op            2 B/op          0 allocs/op
Benchmark_math_big_Float_Add_different_precision-8      37566445                31.4 ns/op            26 B/op          0 allocs/op
Benchmark_decimal_Decimal_Add_same_precision-8           6109453               201 ns/op             349 B/op          8 allocs/op
Benchmark_decimal_Decimal_Sub_same_precision-8           6106828               197 ns/op             349 B/op          8 allocs/op
Benchmark_math_big_Float_Add_same_precision-8           18792164                65.3 ns/op           105 B/op          1 allocs/op
Benchmark_math_big_Float_Sub_same_precision-8           18800349                65.5 ns/op           103 B/op          1 allocs/op

After:

Benchmark_decimal_Decimal_Add_different_precision-8      4774314               253 ns/op             493 B/op          9 allocs/op
Benchmark_decimal_Decimal_Sub_different_precision-8      4920571               243 ns/op             446 B/op          9 allocs/op
Benchmark_math_big_Float_Sub_different_precision-8      458631969                2.62 ns/op            2 B/op          0 allocs/op
Benchmark_math_big_Float_Add_different_precision-8      37542704                31.3 ns/op            26 B/op          0 allocs/op
Benchmark_decimal_Decimal_Add_same_precision-8          20565834                59.5 ns/op           130 B/op          2 allocs/op
Benchmark_decimal_Decimal_Sub_same_precision-8          20362555                61.1 ns/op           131 B/op          2 allocs/op
Benchmark_math_big_Float_Add_same_precision-8           18800202                65.7 ns/op           103 B/op          1 allocs/op
Benchmark_math_big_Float_Sub_same_precision-8           18510948                65.4 ns/op           103 B/op          1 allocs/op

@mwoss mwoss requested a review from njason December 29, 2019 23:20
@mwoss
Copy link
Member Author

mwoss commented Dec 29, 2019

Tests failing on Go1.2.2, I'll fix it tomorrow.

@mwoss
Copy link
Member Author

mwoss commented Dec 31, 2019

Removed benchmarks using big.NewFromFloat (my bad adding them). I'll leave comparison table in the main commnet.

@njason
Copy link
Member

njason commented Jan 5, 2020

RescalePair looks good! I'm hesitant to approve with the formatting changes added since it is difficult to see what code is new in the Sin Cos and Tan functions. I made a separate PR to just format the code #162 . Once it is merged it should clean up this PR up a lot.

@mwoss
Copy link
Member Author

mwoss commented Jan 5, 2020

Oh yea, my bad. I think Golnand formated code automatically.

@mwoss mwoss merged commit 408a250 into master Jan 5, 2020
@mwoss mwoss deleted the memory-allocation-issue branch January 5, 2020 23:12
fairyhunter13 added a commit to fairyhunter13/decimal that referenced this pull request Jul 12, 2020
…pspring#160)

* Remove unnecessary decimal rescaling

* Move benchmarks to separate file, add new benchmark tests
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.

2 participants