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

Cache results of isFiatCurrency and isCryptoCurrency #4955

Merged
merged 3 commits into from
Dec 16, 2020

Conversation

chimp1984
Copy link
Contributor

No description provided.

They are called very often and accumulate cpu time as
shown in profilers.
@stejbac
Copy link
Contributor

stejbac commented Dec 15, 2020

I'm a little surprised there's much speedup, since the methods CurrencyUtil.get[Crypto|Fiat]Currency and Currency.getInstance are already memoised and the other steps in isCryptoCurrency and isFiatCurrency appear to be extremely cheap. Do you know know exactly where the bottleneck in the old version lies? I'm wondering if it could be in the catch (Throwable t) {...} path of isFiatCurrency, since it looks like the first three memoised methods don't cache their result in the case of an exception.

@chimp1984
Copy link
Contributor Author

chimp1984 commented Dec 15, 2020

Thanks @stejbac for the review and comments!

I just did some measurments:
CurrencyUtil2 is the version from that PR, CurrencyUtil the master version.

With wrong Fiat, wrong crypto

CurrencyUtil2.isCryptoCurrency for 100000000 iterations took 1937 ms 
CurrencyUtil2.isFiatCurrency for 100000000 iterations took 1881 ms 
CurrencyUtil.isCryptoCurrency for 100000000 iterations took 2972 ms 
CurrencyUtil.isFiatCurrency for 100000000 iterations took 2134 ms 

With correct Fiat and crypto

CurrencyUtil2.isCryptoCurrency for 100000000 iterations took 1791 ms 
CurrencyUtil2.isFiatCurrency for 100000000 iterations took 1897 ms 
CurrencyUtil.isCryptoCurrency for 100000000 iterations took 2117 ms 
CurrencyUtil.isFiatCurrency for 100000000 iterations took 3572 ms 

With correct Fiat and crypto using concurrentHashMap in CurrencyUtil2

CurrencyUtil2.isCryptoCurrency for 100000000 iterations took 1752 ms 
CurrencyUtil2.isFiatCurrency for 100000000 iterations took 1777 ms 
CurrencyUtil.isCryptoCurrency for 100000000 iterations took 2121 ms 
CurrencyUtil.isFiatCurrency for 100000000 iterations took 3467 ms 

It seems the Currency.getInstance(currencyCode) is the "expensive" one.
The difference is not dramatic, so no strong opinion if we should merge that PR or close it. The methods are called very often so the 100M iteration is not totally crazy (2M times in a short profiling session without lot of UI interaction). To look why they are called that often would be another task, but I assume a big part are Comparators and that sums up quickly at list sorting operations.

@stejbac
Copy link
Contributor

stejbac commented Dec 15, 2020

Yes, I think I recall the methods being a significant hotspot, when profiling the UI a while ago, so even small absolute speedups are probably worthwhile. The updated changes in the PR look fine to me.

utACK

@chimp1984
Copy link
Contributor Author

Yes, I think I recall the methods being a significant hotspot, when profiling the UI a while ago, so even small absolute speedups are probably worthwhile. The updated changes in the PR look fine to me.

utACK

Yes, they are relatively easy and low risk, thats why I started on that beside other more complex ones like in the DAO domain...
A big part are item renderers not caching data but using viewModels as in offerBook. Will work on that as well but thats much more effort...

@sqrrm
Copy link
Member

sqrrm commented Dec 15, 2020

@chimp1984 tests are failing.

sqrrm
sqrrm previously approved these changes Dec 15, 2020
Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

utACK on the changes

core/src/main/java/bisq/core/locale/CurrencyUtil.java Outdated Show resolved Hide resolved
Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

utACK

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.

4 participants