Further speed up transactions view load #6579
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
Replace the "Optional.ofNullable(...)..." constructs with more direct code using short-circuit operators, as this is shorter and a little faster. Also use "trade.get[Deposit|Payout]TxId()" instead of the code "trade.get[Deposit|Payout]TxId().getTxId()", as (upon inspection of the code) there should never be a case where the deposit/payout transaction field of a Trade object is set but the respective txID field is null (or set to an inconsistent value). Also remove a redundant 'RefundManager.getDisputesAsObservableList' method call, which was also slowing things down slightly. The minor speedups afforded by the above are important because the method 'TransactionAwareTrade.isRelatedToTransaction' is called a quadratic number of times and consequently a major bottleneck when loading the Transactions view.
Use a mutable static tuple field to cache the last result of 'Sha256Hash.toString', which is used to get the ID string of the input tx, when calling 'TransactionAwareTrade.isRelatedToTransaction'. In this way, consecutive calls to 'isRelatedToTransaction' on the same input tx (over all the past trades, as done by 'TransactionsView.updateList') are sped up significantly, since hex encoding the txId is a bottleneck.
1. Expression lambda -> method reference; 2. Statement lambda -> expression lambda; 3. Field -> local variable.
Move the line, Set<Tradable> tradables = tradableRepository.getAll(); to the top level of 'TransactionsView.updateList', instead of needlessly calling 'TradableRepository.getAll' (which builds a new set every invocation) for each wallet transaction being iterated over. This was causing a significant slowdown of the view load.
Inline a local variable, to eliminate another minor Sha256Hash.toString hotspot in the Transactions view load, this time coming from 'TransactionsAwareOpenOffer.isRelatedToTransaction'. This is helpful in the case that the user has a large number of (possibly disabled) BSQ swap offers.
Use a crude Bloom filter (of sorts) to cut down the quadratic number of calls to 'TransactionAwareTradable.isRelatedToTransaction' (that is, one for each tx-tradable pair) during the Transactions view load. In this way, we may reduce the number of calls roughly 40-fold, for a Bisq instance with similar numbers of BSQ swap trades and escrow trades. (Sadly, profiling does not show a 40-fold reduction in the size of the 'isRelatedToTransaction' hotspot, likely due to the remaining calls being expensive ones involving disputed trades or unusual txs with nonzero locktime, e.g. dust attacks or funds from Electrum wallets.) To this end, partition the wallet transactions into 64 pseudo-randomly chosen buckets (with a dedicated bucket for txs which might be delayed payouts, namely those with nonzero locktime). Add an interface method, 'TransactionAwareTradable.getRelatedTransactionFilter', which returns an IntStream of all the indices of buckets where a related tx may plausibly be found. Where this is unclear, e.g. for trades involved in a dispute, just return everything (that is, the range 0..63 inclusive). Add a class, 'RelatedTransactionFilterSlices', that holds a provided list of TransactionAwareTradable instances and 64 bitsets of all the slices through their respective filters (each realised as 64-bit word instead of a streams of integers). In this way, a list of tradables plausibly related to any given tx may be quickly found by simply selecting the appropriate bitset of the 64 (by the tx bucket index).
(This also avoids a redundant call to 'Tradable.getShortId', shaving off another percent or two from the Transactions view load.)
Eliminate a minor quadratic time bug, caused by the unnecessary addition of a (BtcWalletService) TxConfidenceListener for each list item in the Transactions view. (Since the confidence listeners are internally held in a CopyOnWriteArraySet, this sadly runs in quadratic time, slowing down the Transactions view load a little.) The confidence listener is apparently redundant because of a set of calls to 'TransactionsListItem.cleanup' immediately upon construction of the item list, which removes all the listeners just added. (This code appears to date from at least February 2016, in commit c70df86.) (The confidence indicators are kept up to date by simply reloading the entire list upon each wallet change event.)
Make 'FormattingUtils.formatDateTime(Date, boolean)' two or three times faster by caching the 'java.text.DateFormat' objects used to format the given date & time in either the local or UTC time zone. Since these formatters are not thread safe (though they may be reused), use a ThreadLocal to store them (as a tuple, along with the current locale to make sure it hasn't changed since the last invocation). This is a minor hotspot for the Transactions view load, since the date strings in TransactionsListItem are formatted eagerly (via DisplayUtils) for the purpose of filtering the items by substring match. (Other list views seem to format dates lazily, though it's possible that there will be speedups elsewhere in the UI.)
In the (hopefully rare) case that the user has multiple past trades that end in arbitration, the entire wallet tx output set was scanned once for every such trade (via 'TransactionAwareTrade.isRefundPayoutTx' calls), to look for any outputs matching the payout address. This potentially causes a slowdown of the Transaction view load for each new arbitration case added. To avoid this problem, cache the last set of recipient address strings of the provided tx, as the next call to 'isRefundPayoutTx' is likely to be for the same tx. Also check that there is exactly one input (the multisig input) for any candidate delayed payout tx, to speed up 'isDelayedPayoutTx' in case the wallet contains many unusual txs with nonzero locktime.
alejandrogarcia83
approved these changes
Feb 12, 2023
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 is a collection of optimisations to the Funds/Transactions display, via the
TransactionsView.updateListmethod. The much earlier PR #5120 also attempted to optimise the transactions view load, but did not completely fix the main quadratic time issue (cost ~ transations count * tradable count, due to this many calls toTransactionAwareTradable.isRelatedToTransaction) and since then it has slowed down again a bit.The main optimisation of this PR is to partition the wallet transactions pseudo-randomly into 64 buckets and use a bit array structure (which could be considered a degenerate Bloom filter with k, the number of hash functions, equal to 1), to filter the list of all tradable-transaction pairs prior to calling
isRelatedToTransactionon each of them. While technically still quadratic time, it cuts down the number of calls (though not the runtime) roughly 40 times (at least when there are similar numbers of BSQ swap trades and escrow trades).The other main speedup is fixing an issue where the set of all tradables was needlessly rebuild for every transaction listed. In addition to that, some further modest optimisations to
isRelatedToTransactionand theTransactionsListItemconstructor were made by introducing some appropriate simple caching, cleaning up the code slightly and removing an unused listener.The following is a call tree of the transactions view load from JProfiler, prior to the optimisations (in which the load was taking 5-15 seconds on my old Bisq instance with ~1300 trades and ~3000 transactions):
The following is the same call tree after the optimisations, repeatedly tabbing to the view 10 times (instead of just once):
