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 the performance of * and / operators #30

Merged
merged 1 commit into from
Feb 25, 2020
Merged

Conversation

Mordil
Copy link
Member

@Mordil Mordil commented Feb 25, 2020

Motivation:

The current implementation of the multiplicative arithmetic operators is naive, as it converts the minorUnits
into a Decimal representation, applies the artihmetic, then does a conversion back to minorUnits.

This can be improved with conversions to both Double, and by doing the arithmetic directly.

Modifications:

  • Change: * and / operators to do conversions to Double instead of Decimal, and inline the arithmetic operations needed

Result:

Performing multiplicative arithmetic against currencies should be much faster now. (XCTest measure reports 50% improvement)

Motivation:

The current implementation of the multiplicative arithmetic operators is naive, as it converts the `minorUnits`
into a Decimal representation, applies the artihmetic, then does a conversion back to `minorUnits`.

This can be improved with conversions to both Double, and by doing the arithmetic directly.

Modifications:

- Change: * and / operators to do conversions to Double instead of Decimal, and inline the arithmetic operations needed

Result:

Performing multiplicative arithmetic against currencies should be much faster now. (XCTest `measure` reports 50% improvement)
@Mordil Mordil added enhancement New feature or request semver-patch Require SemVer Patch bump labels Feb 25, 2020
@codeclimate
Copy link

codeclimate bot commented Feb 25, 2020

Code Climate has analyzed commit cb644de and detected 0 issues on this pull request.

View more on Code Climate.

@codecov
Copy link

codecov bot commented Feb 25, 2020

Codecov Report

Merging #30 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   99.36%   99.36%   +<.01%     
==========================================
  Files           6        6              
  Lines         469      472       +3     
==========================================
+ Hits          466      469       +3     
  Misses          3        3
Impacted Files Coverage Δ
Sources/Currency/AnyCurrency.swift 96.87% <100%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfc56f8...cb644de. Read the comment docs.

@Mordil
Copy link
Member Author

Mordil commented Feb 25, 2020

Methodology:

  • Alternated operators being tested in between each test
  • Code snippet:
    for i in 1...1_000_000 {
        _ = USD(minorUnits: i) <operator under test> USD(minorUnits: i)
    }
  • Executed each test 3 times

Before:

Operator Run 1 Run 2 Run 3 Average
* 2.064 1.968 1.970 2.002
/ 2.399 2.404 2.419 2.407

After:

Operator Run 1 Run 2 Run 3 Average
* 1.073 1.071 1.062 1.069
/ 1.076 1.145 1.078 1.100

@Mordil Mordil merged commit 7264897 into master Feb 25, 2020
@Mordil Mordil deleted the multiply-perf branch February 25, 2020 03:38
@Mordil Mordil mentioned this pull request Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-patch Require SemVer Patch bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant