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

Speed up BSQ tx view load #6600

Merged

Conversation

stejbac
Copy link
Contributor

@stejbac stejbac commented Mar 8, 2023

Apply some fairly simple optimisations to speed up the BSQ tx list display under DAO / BSQ Wallet / Transactions:

  1. Avoid needlessly calling updateList() twice in BsqTxView;
  2. Build a Map<String, BsqSwapTrade> from txIds to swap trades, to fix a quadratic-time performance regression whereby all the tradables were repeatedly scanned to identify the possible matching swap trade for each BSQ tx;
  3. Lazily load tooltips and confidence indicators, as some of the other list views do.

Additionally fix a related bug in the BTC transactions CSV export discovered while working on 3 above, and optimise the BTC funds deposit view slightly with some short circuit logic in DepositListItem, to make it more consistent with change 3 above.

--

With a large number (~1600) of BSQ txs in my Bisq wallet, the following call tree was produced by JProfiler by tabbing to the BSQ tx view 10 times:
Screenshot-2023-3-9 Call Tree

After the optimisations in this PR, the new call tree (from tabbing to the view 10 times) was produced:
Screenshot-2023-3-9 Call Tree(1)

Thus the average load time has gone from around 8.2 seconds to around 170 ms.

stejbac added 5 commits March 8, 2023 13:50
Remove the direct call to 'updateList' in the 'BsqTxView.activate()'
method, as it is later called indirectly via 'onUpdateAnyChainHeight()'.
This nearly doubles the loading speed of the BSQ tx list (in the DAO /
BSQ Wallet tab), since 'updateList' is very slow when there are many txs
in the wallet.
Precompute and pass a map of txIds to BsqSwapTrade instances to the
BsqTxListItem constructor in 'BsqTxView.updateList()', in place of the
tradable repository, so that the tradables don't need to be repeatedly
scanned to find the optional matching BSQ swap trade for each BSQ tx.

This fixes a quadratic time bug and significantly speeds up the BSQ tx
view load for users with many past trades.
Add a 'LazyFields' static class and memoised supplier field to the base
class TxConfidenceListItem, similar to that added to the classes
DepositListItem & TransactionsListItem. This allows lazy loading of
tooltips & tx confidence indicators, just as for those classes.

This removes the current remaining bottleneck in the BSQ tx view load,
revealed by JProfiler. (Note that there is still a quadratic time bug
remaining due to the use of a confidence listener for each list item,
since the BSQ wallet internally uses a CopyOnWriteArrayList, whose
backing array is cloned for each listener addition or removal. However,
it isn't clear that fixing this will give a noticable speedup unless
there are an extremely large number of BSQ txs in the wallet.)

Take care to add the tx confirmation count to LazyFields, to prevent
issues when exporting the list as a CSV. Also add a volatile
'lazyFields' field to detect lazy initialisation of the supplier and
short circuit confidence updates for uninitialised indicators.
Add a 'lazyFields' volatile field to DepositListItem to detect
initialisation of the associated memoised 'LazyFields' supplier,
allowing confidence updates to be short-circuited when the corresponding
indicator has not yet been lazily loaded.

This is for consistency with the 'LazyFields' logic just added to
TxConfidenceListItem and should make the UI more responsive if a new
block arrives when the funds deposit list is in view, by avoiding the
entire list of item tooltips & confidence indicators from being force-
initialised.
Move the 'confirmations' field from TransactionsListItem to the nested
'LazyFields' class, so that it is correctly lazily initialised when
'getNumConfirmations()' is called, instead of just returning 0 for
hidden list items with uninitialised tooltips + confidence indicators.

This makes the logic consistent with that in TxConfidenceListItem and
fixes a bug in the BTC transactions view CSV export, where only the
already rendered list items would have nonzero confirmation counts.
Copy link
Contributor

@alvasw alvasw left a comment

Choose a reason for hiding this comment

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

Excellent work!

ACK

Copy link
Collaborator

@HenrikJannsen HenrikJannsen left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Contributor

@alejandrogarcia83 alejandrogarcia83 left a comment

Choose a reason for hiding this comment

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

utACK

@alejandrogarcia83 alejandrogarcia83 added this to the v1.9.10 milestone Mar 11, 2023
@alejandrogarcia83 alejandrogarcia83 merged commit 4e4321e into bisq-network:master Mar 11, 2023
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