perf: skip redundant currency resolution in Money.new#519
perf: skip redundant currency resolution in Money.new#519
Conversation
Money.new already resolves the currency string via Helpers.value_to_currency before calling super, but Money#initialize called value_to_currency a second time on the already-resolved Currency object. This meant every Money.new(value, "USD") paid for two full case/when dispatches and two Currency.find! lookups instead of one. Remove the redundant resolution from initialize (fix init_with to resolve before calling initialize), and cache string->Currency mappings in Helpers.value_to_currency so repeat calls with the same currency code skip the to_s.downcase + hash lookup in Currency.find!. Benchmark (500k iterations): Before: string ~455ms, currency ~325ms After: string ~360ms, currency ~285ms ~21% faster for the string path, ~12% for pre-resolved Currency objects. The gap between passing a string vs a Currency object shrinks by ~42%. Co-Authored-By: Vera Olsson <842976+surkova@users.noreply.github.com>
6568d58 to
b993147
Compare
| Money::NULL_CURRENCY | ||
| when String | ||
| Currency.find!(currency) | ||
| resolved_currencies[currency] ||= Currency.find!(currency) |
There was a problem hiding this comment.
this is already happening inside the find! method https://github.com/Shopify/money/blob/main/lib/money/currency.rb#L16 🤔 wonder how you're still getting a speed improvement. Can you remove the resolved_currencies and redo the benchmark 🙏
There was a problem hiding this comment.
In those cases we skip over an invocation of currency_iso.to_s.downcase, which helps at the margins.
There was a problem hiding this comment.
if we want to skip the downcase, we should do it in the find!. This could effectively double the size of the hash, but could be worthwhile. Can you split the PRs (one for removing the downcase, and one for the initialize changes). We need to show that each is a performance improvement on their own
def new(currency_iso)
iso = currency_iso.to_s
return @@loaded_currencies[iso] if @@loaded_currencies.key?(iso)
raise UnknownCurrency, "Currency can't be blank" if iso.empty?
@@mutex.synchronize { @@loaded_currencies[iso] = super(iso.downcase) }
end| raise ArgumentError if value.infinite? | ||
|
|
||
| @currency = Helpers.value_to_currency(currency) | ||
| @currency = currency |
|
|
||
| @currency = Helpers.value_to_currency(currency) | ||
| @currency = currency | ||
| @value = BigDecimal(value.round(@currency.minor_units)) |
There was a problem hiding this comment.
we can probably also do the same here
| @value = BigDecimal(value.round(@currency.minor_units)) | |
| @value = value.round(@currency.minor_units) |
There was a problem hiding this comment.
Tried this out... nice idea; makes the tests fail. The problem is BigDecimal#round returns an Integer when the currency has no subunits (i.e. minor_units == 0 -- JPY for example), so we really need this wrapper for those cases. (Methods downstream assume @value will be a BigDecimal instance)
|
Benchmark I used for this: require "money"
require "benchmark"
n = 500_000
c = Money::Currency.find!("USD")
Benchmark.bm do |x|
x.report("string") { n.times { Money.new(100, "USD") } }
x.report("currency") { n.times { Money.new(100, c) } }
endResults when I ran them just now: Main branch user system total real
string 0.426026 0.000652 0.426678 ( 0.426774)
currency 0.303333 0.000438 0.303771 ( 0.303835)This branch user system total real
string 0.349412 0.001080 0.350492 ( 0.350653)
currency 0.276891 0.000863 0.277754 ( 0.277763) |
Summary
Money.new(value, "USD")resolves the currency string twice: once inMoney.newviaHelpers.value_to_currency, and again inMoney#initializewhich callsvalue_to_currencyon the already-resolvedCurrencyobject. This means every construction pays for two fullcase/whendispatches throughvalue_to_currencyand twoCurrency.find!lookups instead of one.This PR makes two changes:
Remove redundant
value_to_currencyfromMoney#initialize: sinceMoney.newalways resolves the currency before callingsuper,initializecan trust that itscurrencyargument is already aCurrencyobject.init_with(YAML deserialization) is updated to resolve before callinginitialize.Cache string-to-Currency mappings in
Helpers.value_to_currency: repeat calls with the same currency code string (e.g."USD") now skip theCurrency.find!lookup (which doesto_s.downcase+ hash lookup) after the first resolution. The cache is reset alongsideCurrency.reset_loaded_currencies.Benchmark (500k iterations of
Money.new)