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

Allow %Currency{} structs in currency_for_code/3 #4

Merged

Conversation

jeroenvisser101
Copy link
Contributor

@jeroenvisser101 jeroenvisser101 commented Jun 16, 2021

This PR is part of a series of PRs that aim to improve performance by allowing both %Cldr.Number.Format.Options{} structs to be passed as options in place of keyword lists, and to add support for %Cldr.Currency{} structs as alternative for currency codes.

#4 (this PR)
elixir-cldr/cldr_numbers#18
kipcole9/money#127

This PR adds support to pass a %Cldr.Currency struct directly, and will return it if it's passed to currency_for_code/3. Since currency_for_code/3 is relatively complex, and iterates over currencies (which may be something that can be addressed in a future PR, replacing it with a keyed lookup via a map), this change attempts to offer developers a way to pass the currency struct, already resolved. This in turn made a real-world impact when we ran this in a tight loop. In our use-case, we know the currencies beforehand, and we pre-resolve these currencies. We take the trade-off between slighly higher compile time, and much faster runtime performance.

I ran this benchmark:
https://gist.github.com/jeroenvisser101/32deb85f50fe76f8cd5d48049591e355

Operating System: macOS
CPU Information: Intel(R) Xeon(R) W-3245 CPU @ 3.20GHz
Number of Available Cores: 32
Available memory: 160 GB
Elixir 1.12.0
Erlang 24.0

Benchmark suite executing with the following configuration:
warmup: 2 s
time: 10 s
memory time: 2 s
parallel: 1
inputs: none specified
Estimated total run time: 42 s

Benchmarking with precompiled everything...
Benchmarking with precompiled options but without precompiled currency...
Benchmarking without precompiled options or currency...

Name                                                                ips        average  deviation         median         99th %
with precompiled everything                                    104.20 K        9.60 μs   ±254.01%        7.99 μs       31.99 μs
with precompiled options but without precompiled currency        2.45 K      408.37 μs    ±45.39%      393.99 μs      627.66 μs
without precompiled options or currency                          2.41 K      414.23 μs    ±10.81%      405.99 μs      588.99 μs

Comparison: 
with precompiled everything                                    104.20 K
with precompiled options but without precompiled currency        2.45 K - 42.55x slower +398.77 μs
without precompiled options or currency                          2.41 K - 43.16x slower +404.64 μs

Memory usage statistics:

Name                                                         Memory usage
with precompiled everything                                       9.51 KB
with precompiled options but without precompiled currency       193.95 KB - 20.40x memory usage +184.45 KB
without precompiled options or currency                         216.27 KB - 22.75x memory usage +206.77 KB

**All measurements for memory usage were the same**

@kipcole9
Copy link
Collaborator

Thanks for the PR, much appreciated. Great suggestion. Its the same principle that led me to allow precomputed options to be passed to Cldr.Number.to_string/2 since in tight loops, options processing is 50% of the compute time. And clearly from your benchmark its worse than that for currencies.

I will definitely look into whether currency validation can be improved as you suggest but this PR is a great start.

@kipcole9 kipcole9 merged commit e0b3194 into elixir-cldr:master Jun 16, 2021
@jeroenvisser101
Copy link
Contributor Author

Thanks! We use Money and Cldr very heavily, and appreciate the work you put into this, it has never let us down. If I can find the time, I might be able to contribute more performance related enhancements :)

@kipcole9
Copy link
Collaborator

Published ex_cldr_currencies version 2.10.0 with the following changelog:

Enhancements

  • Allow %Currency{} structs in currency_for_code/3. Thanks to @jeroenvisser101 for the PR. Improves the performance by up to 40x when validating currencies when the currency has already been pre-constructed.

  • Add implementation of the Inspect protocol for t:Cldr.Currency structs.

If there are other particular hotspots you find, please keep the issues coming!

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