From 6dafafd7b1f2742b7448001f895eea0bef9d0e09 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Sun, 1 Sep 2019 22:47:10 +0200 Subject: [PATCH] Fix performance issue in BsqWalletService The updateBsqWalletTransactions method got called at each block for all transactions. During block download that wasted a lot of cpu and led to stuck UI thread and lost connections. The updateBsqBalance is not cheap (a few ms) and called for 100s of txs at each block was very problematic. Furthermore the listeners on the walletTransactions observableList got triggered which made the situation worse. We changed the observableList to a ArrayList and use a listener which gets called after the list is updated. We also make sure the onTransactionConfidenceChanged listener is not calling updateBsqWalletTransactions if bsq parsing is not complete and if the depth of the tx is > 1. In the updateBsqWalletTransactions method we use a flag and a delay to ensure that the updateBsqBalance is not called more then once in 100 ms. We changed also the getter to return a cloned version of the list to avoid potential concurrent modification exceptions at clients. Closes #3175 --- .../core/btc/wallet/BsqWalletService.java | 64 +++++++++++++++---- .../dao/governance/bond/BondRepository.java | 17 ++++- .../MyBondedReputationRepository.java | 18 ++++-- .../bond/role/BondedRolesRepository.java | 2 +- .../desktop/main/dao/wallet/tx/BsqTxView.java | 23 ++++--- 5 files changed, 94 insertions(+), 30 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/wallet/BsqWalletService.java b/core/src/main/java/bisq/core/btc/wallet/BsqWalletService.java index 49018f8bca9..b8ccad5aba0 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BsqWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BsqWalletService.java @@ -35,6 +35,8 @@ import bisq.core.provider.fee.FeeService; import bisq.core.user.Preferences; +import bisq.common.UserThread; + import org.bitcoinj.core.Address; import org.bitcoinj.core.AddressFormatException; import org.bitcoinj.core.BlockChain; @@ -57,15 +59,14 @@ import javax.inject.Inject; -import javafx.collections.FXCollections; -import javafx.collections.ObservableList; - +import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.concurrent.CopyOnWriteArraySet; +import java.util.concurrent.TimeUnit; import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -80,12 +81,20 @@ @Slf4j public class BsqWalletService extends WalletService implements DaoStateListener { + + public interface WalletTransactionsChangeListener { + + void onWalletTransactionsChange(); + } + private final BsqCoinSelector bsqCoinSelector; private final NonBsqCoinSelector nonBsqCoinSelector; private final DaoStateService daoStateService; private final UnconfirmedBsqChangeOutputListService unconfirmedBsqChangeOutputListService; - private final ObservableList walletTransactions = FXCollections.observableArrayList(); + private final List walletTransactions = new ArrayList<>(); private final CopyOnWriteArraySet bsqBalanceListeners = new CopyOnWriteArraySet<>(); + private final List walletTransactionsChangeListeners = new ArrayList<>(); + private boolean updateBsqWalletTransactionsPending; // balance of non BSQ satoshis @Getter @@ -152,7 +161,13 @@ public void onReorganize(Wallet wallet) { @Override public void onTransactionConfidenceChanged(Wallet wallet, Transaction tx) { - updateBsqWalletTransactions(); + // We are only interested in updates from unconfirmed txs and confirmed txs at the + // time when it gets into a block. Otherwise we would get called + // updateBsqWalletTransactions for each tx as the block depth changes for all. + if (tx.getConfidence().getDepthInBlocks() <= 1 && + daoStateService.isParseBlockChainComplete()) { + updateBsqWalletTransactions(); + } unconfirmedBsqChangeOutputListService.onTransactionConfidenceChanged(tx); } @@ -215,6 +230,7 @@ String getWalletAsString(boolean includePrivKeys) { /////////////////////////////////////////////////////////////////////////////////////////// private void updateBsqBalance() { + long ts = System.currentTimeMillis(); unverifiedBalance = Coin.valueOf( getTransactions(false).stream() .filter(tx -> tx.getConfidence().getConfidenceType() == PENDING) @@ -246,7 +262,7 @@ private void updateBsqBalance() { } return false; }) - .mapToLong(in -> in != null ? in.getValue().value : 0) + .mapToLong(in -> in.getValue() != null ? in.getValue().value : 0) .sum(); return outputs - lockedInputs; }) @@ -289,6 +305,7 @@ private void updateBsqBalance() { bsqBalanceListeners.forEach(e -> e.onUpdateBalances(availableConfirmedBalance, availableNonBsqBalance, unverifiedBalance, unconfirmedChangeBalance, lockedForVotingBalance, lockupBondsBalance, unlockingBondsBalance)); + log.info("updateBsqBalance took {} ms", System.currentTimeMillis() - ts); } public void addBsqBalanceListener(BsqBalanceListener listener) { @@ -299,13 +316,21 @@ public void removeBsqBalanceListener(BsqBalanceListener listener) { bsqBalanceListeners.remove(listener); } + public void addWalletTransactionsChangeListener(WalletTransactionsChangeListener listener) { + walletTransactionsChangeListeners.add(listener); + } + + public void removeWalletTransactionsChangeListener(WalletTransactionsChangeListener listener) { + walletTransactionsChangeListeners.remove(listener); + } + /////////////////////////////////////////////////////////////////////////////////////////// // BSQ TransactionOutputs and Transactions /////////////////////////////////////////////////////////////////////////////////////////// - public ObservableList getWalletTransactions() { - return walletTransactions; + public List getClonedWalletTransactions() { + return new ArrayList<>(walletTransactions); } public Stream getPendingWalletTransactionsStream() { @@ -314,9 +339,21 @@ public Stream getPendingWalletTransactionsStream() { } private void updateBsqWalletTransactions() { - walletTransactions.setAll(getTransactions(false)); if (daoStateService.isParseBlockChainComplete()) { - updateBsqBalance(); + // We get called updateBsqWalletTransactions multiple times from onWalletChanged, onTransactionConfidenceChanged + // and from onParseBlockCompleteAfterBatchProcessing. But as updateBsqBalance is an expensive operation we do + // not want to call it in a short interval series so we use a flag and a delay to not call it multiple times + // in a 100 ms period. + if (!updateBsqWalletTransactionsPending) { + updateBsqWalletTransactionsPending = true; + UserThread.runAfter(() -> { + walletTransactions.clear(); + walletTransactions.addAll(getTransactions(false)); + walletTransactionsChangeListeners.forEach(WalletTransactionsChangeListener::onWalletTransactionsChange); + updateBsqBalance(); + updateBsqWalletTransactionsPending = false; + }, 100, TimeUnit.MILLISECONDS); + } } } @@ -434,7 +471,7 @@ public Coin getValueSentToMeForTransaction(Transaction transaction) throws Scrip } public Optional isWalletTransaction(String txId) { - return getWalletTransactions().stream().filter(e -> e.getHashAsString().equals(txId)).findAny(); + return walletTransactions.stream().filter(e -> e.getHashAsString().equals(txId)).findAny(); } @@ -553,7 +590,10 @@ private Transaction getPreparedBurnFeeTx(Coin fee, boolean requireChangeOutput) return tx; } - private void addInputsAndChangeOutputForTx(Transaction tx, Coin fee, BsqCoinSelector bsqCoinSelector, boolean requireChangeOutput) + private void addInputsAndChangeOutputForTx(Transaction tx, + Coin fee, + BsqCoinSelector bsqCoinSelector, + boolean requireChangeOutput) throws InsufficientBsqException { Coin requiredInput; // If our fee is less then dust limit we increase it so we are sure to not get any dust output. diff --git a/core/src/main/java/bisq/core/dao/governance/bond/BondRepository.java b/core/src/main/java/bisq/core/dao/governance/bond/BondRepository.java index 19b4336076f..3b227b02e54 100644 --- a/core/src/main/java/bisq/core/dao/governance/bond/BondRepository.java +++ b/core/src/main/java/bisq/core/dao/governance/bond/BondRepository.java @@ -35,7 +35,6 @@ import javax.inject.Inject; import javafx.collections.FXCollections; -import javafx.collections.ListChangeListener; import javafx.collections.ObservableList; import java.util.Arrays; @@ -55,7 +54,8 @@ * unconfirmed txs. */ @Slf4j -public abstract class BondRepository implements DaoSetupService { +public abstract class BondRepository implements DaoSetupService, + BsqWalletService.WalletTransactionsChangeListener { /////////////////////////////////////////////////////////////////////////////////////////// // Static @@ -161,7 +161,7 @@ public void onParseBlockCompleteAfterBatchProcessing(Block block) { update(); } }); - bsqWalletService.getWalletTransactions().addListener((ListChangeListener) c -> update()); + bsqWalletService.addWalletTransactionsChangeListener(this); } @Override @@ -170,6 +170,16 @@ public void start() { } + /////////////////////////////////////////////////////////////////////////////////////////// + // BsqWalletService.WalletTransactionsChangeListener + /////////////////////////////////////////////////////////////////////////////////////////// + + @Override + public void onWalletTransactionsChange() { + update(); + } + + /////////////////////////////////////////////////////////////////////////////////////////// // API /////////////////////////////////////////////////////////////////////////////////////////// @@ -195,6 +205,7 @@ public List getActiveBonds() { abstract protected Stream getBondedAssetStream(); protected void update() { + log.debug("update"); getBondedAssetStream().forEach(bondedAsset -> { String uid = bondedAsset.getUid(); bondByUidMap.putIfAbsent(uid, createBond(bondedAsset)); diff --git a/core/src/main/java/bisq/core/dao/governance/bond/reputation/MyBondedReputationRepository.java b/core/src/main/java/bisq/core/dao/governance/bond/reputation/MyBondedReputationRepository.java index 4fc97d47540..0de193ba97d 100644 --- a/core/src/main/java/bisq/core/dao/governance/bond/reputation/MyBondedReputationRepository.java +++ b/core/src/main/java/bisq/core/dao/governance/bond/reputation/MyBondedReputationRepository.java @@ -26,12 +26,9 @@ import bisq.core.dao.state.DaoStateService; import bisq.core.dao.state.model.blockchain.Block; -import org.bitcoinj.core.Transaction; - import javax.inject.Inject; import javafx.collections.FXCollections; -import javafx.collections.ListChangeListener; import javafx.collections.ObservableList; import java.util.Arrays; @@ -50,7 +47,7 @@ * unconfirmed txs. */ @Slf4j -public class MyBondedReputationRepository implements DaoSetupService { +public class MyBondedReputationRepository implements DaoSetupService, BsqWalletService.WalletTransactionsChangeListener { private final DaoStateService daoStateService; private final BsqWalletService bsqWalletService; private final MyReputationListService myReputationListService; @@ -84,7 +81,7 @@ public void onParseBlockCompleteAfterBatchProcessing(Block block) { update(); } }); - bsqWalletService.getWalletTransactions().addListener((ListChangeListener) c -> update()); + bsqWalletService.addWalletTransactionsChangeListener(this); } @Override @@ -92,11 +89,22 @@ public void start() { } + /////////////////////////////////////////////////////////////////////////////////////////// + // BsqWalletService.WalletTransactionsChangeListener + /////////////////////////////////////////////////////////////////////////////////////////// + + @Override + public void onWalletTransactionsChange() { + update(); + } + + /////////////////////////////////////////////////////////////////////////////////////////// // Private /////////////////////////////////////////////////////////////////////////////////////////// private void update() { + log.debug("update"); // It can be that the same salt/hash is in several lockupTxs, so we use the bondByLockupTxIdMap to eliminate // duplicates by the collection algorithm. Map bondByLockupTxIdMap = new HashMap<>(); diff --git a/core/src/main/java/bisq/core/dao/governance/bond/role/BondedRolesRepository.java b/core/src/main/java/bisq/core/dao/governance/bond/role/BondedRolesRepository.java index 2f25d972eca..8176565d0ed 100644 --- a/core/src/main/java/bisq/core/dao/governance/bond/role/BondedRolesRepository.java +++ b/core/src/main/java/bisq/core/dao/governance/bond/role/BondedRolesRepository.java @@ -60,7 +60,7 @@ public BondedRolesRepository(DaoStateService daoStateService, BsqWalletService b /////////////////////////////////////////////////////////////////////////////////////////// public boolean isMyRole(Role role) { - Set myWalletTransactionIds = bsqWalletService.getWalletTransactions().stream() + Set myWalletTransactionIds = bsqWalletService.getClonedWalletTransactions().stream() .map(Transaction::getHashAsString) .collect(Collectors.toSet()); return getAcceptedBondedRoleProposalStream() diff --git a/desktop/src/main/java/bisq/desktop/main/dao/wallet/tx/BsqTxView.java b/desktop/src/main/java/bisq/desktop/main/dao/wallet/tx/BsqTxView.java index 803c54900b3..ed34c304ac4 100644 --- a/desktop/src/main/java/bisq/desktop/main/dao/wallet/tx/BsqTxView.java +++ b/desktop/src/main/java/bisq/desktop/main/dao/wallet/tx/BsqTxView.java @@ -71,13 +71,11 @@ import javafx.beans.value.ChangeListener; import javafx.collections.FXCollections; -import javafx.collections.ListChangeListener; import javafx.collections.ObservableList; import javafx.collections.transformation.SortedList; import javafx.util.Callback; -import java.util.ArrayList; import java.util.Comparator; import java.util.Date; import java.util.List; @@ -85,7 +83,8 @@ import java.util.stream.Collectors; @FxmlView -public class BsqTxView extends ActivatableView implements BsqBalanceListener, DaoStateListener { +public class BsqTxView extends ActivatableView implements BsqBalanceListener, DaoStateListener, + BsqWalletService.WalletTransactionsChangeListener { private TableView tableView; @@ -100,7 +99,6 @@ public class BsqTxView extends ActivatableView implements BsqBal private final ObservableList observableList = FXCollections.observableArrayList(); // Need to be DoubleProperty as we pass it as reference private final SortedList sortedList = new SortedList<>(observableList); - private ListChangeListener walletBsqTransactionsListener; private int gridRow = 0; private Label chainHeightLabel; private ProgressBar chainSyncIndicator; @@ -173,7 +171,6 @@ public void initialize() { VBox.setVgrow(tableView, Priority.ALWAYS); root.getChildren().add(vBox); - walletBsqTransactionsListener = change -> updateList(); walletChainHeightListener = (observable, oldValue, newValue) -> { walletChainHeight = bsqWalletService.getBestChainHeight(); onUpdateAnyChainHeight(); @@ -183,7 +180,7 @@ public void initialize() { @Override protected void activate() { bsqBalanceUtil.activate(); - bsqWalletService.getWalletTransactions().addListener(walletBsqTransactionsListener); + bsqWalletService.addWalletTransactionsChangeListener(this); bsqWalletService.addBsqBalanceListener(this); btcWalletService.getChainHeightProperty().addListener(walletChainHeightListener); @@ -207,7 +204,7 @@ protected void activate() { protected void deactivate() { bsqBalanceUtil.deactivate(); sortedList.comparatorProperty().unbind(); - bsqWalletService.getWalletTransactions().removeListener(walletBsqTransactionsListener); + bsqWalletService.removeWalletTransactionsChangeListener(this); bsqWalletService.removeBsqBalanceListener(this); btcWalletService.getChainHeightProperty().removeListener(walletChainHeightListener); daoFacade.removeBsqStateListener(this); @@ -254,6 +251,15 @@ public void onParseBlockChainComplete() { } } + /////////////////////////////////////////////////////////////////////////////////////////// + // BsqWalletService.WalletTransactionsChangeListener + /////////////////////////////////////////////////////////////////////////////////////////// + + @Override + public void onWalletTransactionsChange() { + updateList(); + } + /////////////////////////////////////////////////////////////////////////////////////////// // Private @@ -299,8 +305,7 @@ private void onUpdateAnyChainHeight() { private void updateList() { observableList.forEach(BsqTxListItem::cleanup); - // copy list to avoid ConcurrentModificationException - final List walletTransactions = new ArrayList<>(bsqWalletService.getWalletTransactions()); + List walletTransactions = bsqWalletService.getClonedWalletTransactions(); List items = walletTransactions.stream() .map(transaction -> { return new BsqTxListItem(transaction,