Speed up deposit and transactions view loads #5120
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Make the WalletService.walletEventListener field private and add it via a protected method defined in the base class, addListenersToWallet(), so that the setup code in the two subclasses (Bsq|Btc)WalletService can be deduplicated and more easily kept in sync with the listener removal code in WalletService.shutDown(). Also remove some unnecessary deprecation warning suppressions.
Use a guava Multiset to cache the total number of tx outputs (out of the live txs in the user's wallet) with a given address. Since this requires a scan of the entire tx set, compute all the counts in one go and store in an ImmutableMultiset<Address>. Invalidate the entire cache any time a tx set change occurs, by attaching a WalletChangeEventListener to the wallet (using a direct executor for immediate effect). This is to fix a quadratic time bug in DepositView, which uses the count to determine if a given address in the BTC wallet is used/unused.
Use a guava SetMultimap (a many-to-many mapping without duplicates) to cache the set of live txs in the user's wallet with a given address as an input or output. As with the cache of output counts from the previous commit, compute all the tx sets in one go (by a tx stream followed by a map inverse) and store in an ImmutableSetMultimap<Address, Transaction>, invalidating the entire cache immediately upon each wallet change event. This is to fix another (larger) quadratic time bug in DepositView, when getting the confidence (i.e. confirmation count) of each wallet address. Also simplify getTransactionConfidence & onTransactionConfidenceChanged methods slightly, which generated (possibly unintentionally) repeating & singleton lists of TransactionConfidence objects to pass to WalletService.getMostRecentConfidence(..) respectively.
Attempt to remove a bottleneck during the transactions view load, as revealed by JProfiler, by optimising the code to determine if any given transaction and trade are related. Since both these sets will tend to grow linearly with time, causing quadratic slowdown of TransactionsView, try to alleviate (without completely fixing) the problem. To do this, add a cached set of disputed trade IDs to DisputeListService so that TransactionAwareTradable.is(Dispute|RefundPayout)Tx can be made O(1) in the common case that the given trade is not involved in any dispute. Also avoid calling Sha256Hash::toString by passing tx IDs as Sha256Hash objects directly to is(Deposit|Payout)Tx, and short circuit an expensive call BtcWalletService.getTransaction in isDelayedPayoutTx, in the common case, by pre-checking the transaction locktime. This also fixes a bug in isRefundPayoutTx whereby it incorrectly returns false if there are >1 disputes in the list returned by RefundManager but the given trade is not involved in the last one.
Fix a serious memory leak in DepositListItem due to missing removal of the BalanceListener (one per item) from BtcWalletService. This prevented GC of the entire list of items, which was observed to leak ~500 MB in JProfiler after repeated switching (several dozen times) between tabs.
Add a nested class of lazy fields to (Deposit|Transactions)ListItem, together with an associated memoised Supplier, lazyFieldsSupplier, which initialises them all the first time any one of them is requested. Move the txConfidenceIndicator & tooltip fields to this class, so that they are only loaded when the given address/tx row is displayed in the deposit/transactions views, respectively. This prevents a minor bottleneck, as profiling indicates that creating a tooltip for each tx confidence indicator in the list is quite expensive and takes up around half the rendering time. (There may be 1000's of txs or addresses in an old wallet.)
chimp1984
suggested changes
Jan 28, 2021
Undo the earlier simplification changes to getTransactionConfidence, which preserved its original but broken behaviour. Fix the original stream pipeline so that each matching tx input maps to the confidence of the connected parent tx (if any), not the child tx. In this way, it correctly considers parent tx confidences when determining the most recent confidence of all the matching inputs & outputs. Before it was simply feeding a repeating list of identical objects into getMostRecentConfidence, via the erroneous line: .map(o -> tx.getConfidence()) (Also add a missing @nullable annotation & make getMostRecentConfidence private instead of protected.) Finally, simplify BisqWalletListener.onTransactionConfidenceChanged, by no longer feeding a singleton list into getMostRecentConfidence whose element is already a return value of that method, as that is a no-op.
sqrrm
approved these changes
Feb 10, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR speeds up the loading of the deposit ("Receive funds") and transactions views (under the "Funds" tab), by adding some caches to
WalletServiceandDisputeListService, lazily loading tx confidence tooltips for the two respective lists of items (DepositListItemandTransactionsListItem) and applying some low level optimisations toTransactionsAwareTrade.isRelatedToTransaction.This also fixes a fairly minor bug in the transactions view due to a broken
TranscationsAwareTrade.isRefundPayoutTxmethod, as well as a somewhat serious memory leak in the deposit view due to missingBalanceListenerremovals from theBtcWalletServiceinstance.The performance issues in these two views had started to become severe with the number of transactions and addresses now in my Bisq wallet. The deposit view was the worst, taking around 25 seconds to load, with bottlenecks in
WalletService.getTransactionConfidence&WalletService.getNumTxOutputsForAddressas seen with JProfiler:The transactions view (with 1210 rows) was taking around 5 seconds to load, with bottlenecks in
TransactionsAwareTrade.isRelatedToTransaction&Tooltip.<init>:They both seem significantly faster (in my Bisq client) with the changes in this PR, with the biggest improvement to the deposit view via the two caches added to
WalletService. The quadratic time issue (# transactions * # past trades) causing the slowness of the transactions view is alleviated but not completely fixed by these changes. It may be possible to do more extensive lazy loading of the relevantTransactionsListItemfields to get further speedups.