Fix largest part of GUI lockups with large wallets #3155
Conversation
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
I like optimizations for address and date strings but I think changes made in updateStatus, statusUpdateNeeded etc. look better in #3152 tbh (for example, the overhead of IsChainLocked() is minimal imo, it basically does the same as your new similar code here, but it's much more readable in 3152).
Also, it looks like the idea in 04e3ce3 is very similar to bitcoin@6d7104c... which we missed earlier 🙈 probably because it's a part of a PR with fixes for feebump. I think we should backport that instead (and add Q_EMIT part on top).
And finally, updateConfirmations() is still the major source of freezes for me, applying #3154 fixes them.
So to sum it up: I'd prefer to merge #3152 + #3154 + 6d7104c backport (+Q_EMIT) and then this PR should only add optimizations for addresses and dates.
The reason I re-implemented #3152 was that the
I've replaced my solution with a backport of 6d7104c and the
Yes it is still the major source of freezes, but it got a lot better. It's down from multiple seconds to slightly less then a second on my machine.
I would prefer to not merge #3154 as it is a hack. It's unlikely, but it can lead to stale data shown in the UI. I would prefer to fix the actual bottlenecks. I've added another commit on top which puts the label into the TransactionRecord and updates it when the address book changes. This avoids the expensive lookups done in EDIT: I was referring to #3142 being a hack, but I actually meant #3154 |
codablock
force-pushed
the
pr_guilockups
branch
from
ef626c1 to
1111329
Compare
…ilterAcceptsRow The QDateTime::operator< calls inside TransactionFilterProxy::filterAcceptsRow turned out to be the slowest part in the UI when many TXs are inside the wallet. DateRoleInt allows us to request the plain seconds since epoch which we then use to compare against dateFrom/dateTo, which are also both stored as seconds since epoch now.
This one avoids converting from string to CBitcoinAddress and calling .Get() on the result.
…cord This avoids frequent and slow conversion
This avoids unnecessary conversions
We already do this through updateTransaction(), which is also called when an IS lock is received for one of our own TXs.
…is possible lockedByChainLocks can never get back to false, so no need to re-check it. Same with lockedByInstantSend, except when a ChainLock overrides it.
Instead of looking it up in data()
codablock
force-pushed
the
pr_guilockups
branch
from
94389e6 to
7aa074f
Compare
|
Fiexed #3155 (comment) and rebased on develop to bring in latest Gitlab fixes |
* qt: Fix labels in transaction list The issue was introduced in #3682 * qt: Always use labels from TransactionStatus for transaction list Missed this in #3155 * Update src/qt/transactiontablemodel.cpp Co-authored-by: dustinface <[email protected]> Co-authored-by: dustinface <[email protected]>
* qt: Fix labels in transaction list The issue was introduced in dashpay#3682 * qt: Always use labels from TransactionStatus for transaction list Missed this in dashpay#3155 * Update src/qt/transactiontablemodel.cpp Co-authored-by: dustinface <[email protected]> Co-authored-by: dustinface <[email protected]>
, bitcoin#20591, bitcoin#23596, bitcoin#22868, bitcoin#24225 (wallet backports: part 3) 6958a5b merge bitcoin#24225: Add sanity checks to DiscourageFeeSniping (Kittywhiskers Van Gogh) 63dc779 merge bitcoin#22868: Call load handlers without cs_wallet locked (Kittywhiskers Van Gogh) 698e01e merge bitcoin#23596: fix `wallet_transactiontime_rescan.py --descriptors` and add to test runner (Kittywhiskers Van Gogh) 72322ce merge bitcoin#20591: fix ComputeTimeSmart function during rescanning process (Kittywhiskers Van Gogh) 99c9880 merge bitcoin#22941: inline functions `{Read,Write}OrderPos` (Kittywhiskers Van Gogh) e2ae504 merge bitcoin#25148: Remove `NO_THREAD_SAFETY_ANALYSIS` from non-test/benchmarking code (Kittywhiskers Van Gogh) f637516 merge bitcoin#22100: Clean up new wallet spend, receive files added bitcoin#21207 (Kittywhiskers Van Gogh) 9a5dc62 partial bitcoin#22100: Clean up new wallet spend, receive files added bitcoin#21207 (Kittywhiskers Van Gogh) 258cdc2 refactor: move and rename `GetBudgetSystemCollateralTX()` to `spend.cpp` (Kittywhiskers Van Gogh) e03fdff refactor: move ISLock and ChainLock wtx check to CWallet (Kittywhiskers Van Gogh) 460c187 refactor: move some CoinJoin-specific logic out of CWalletTx and CWallet (Kittywhiskers Van Gogh) ed12dab move-only: section CoinJoin-specific code to separate source file (Kittywhiskers Van Gogh) bc0a1b7 refactor: remove unused `CWalletTx::GetWallet()` (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * `CWalletTx::GetWallet()` was introduced in [dash#3155](#3155) ([commit](3b44576)) and its use was curtailed in [dash#3682](#3682) ([commit](155324e)) but it has lingered around. As [bitcoin#22100](bitcoin#22100) will get rid of `CWalletTx::pwallet` and it is unused, it is removed. * As [bitcoin#22100](bitcoin#22100) has a substantial diff, it has been split into two commits for ease of review, the former centers around `wallet/receive.cpp` and the latter around `wallet/spend.cpp`. The former has additional changes that are supplanted/removed in the latter to ensure they can be independently validated. To compare the diff against upstream, squashing the two halves may be desirable. * As the goal of the backport to remove the circular dependencies between `wallet.cpp` and both `spend.cpp` and `receive.cpp`, Dash-specific code that contributes to this dependency had to be moved. To this effect, CoinJoin-specific code has been moved to its own source file as it relies on code in both `spend.cpp` and `receive.cpp`. `GetBudgetSystemCollateralTX()` was moved to `spend.cpp` as it used `CreateTransaction()` and given a better representative name. * ~~`CWalletTx::Is{ChainLocked, LockedByInstantSend}()` was moved to `CWallet` and renamed to match the convention of `CWallet::GetTxDepthInMainChain()` in preparation for [bitcoin#22100](bitcoin#22100). A drawback of the new implementation is that both calls (but especially `IsTxLockedByInstantSend()`) is more expensive as per-transaction result caching is no longer available.~~ Resolved by d29b5ee, thanks Udjin! * Portions of [bitcoin#25148](bitcoin#25148) were included in previous commits to ensure that `-Wthread-safety` is satisfied (e.g. annotations for `MakeWalletTxStatus()` and `WalletTxToJSON()` were needed when `IsTxLockedByInstantSend()` and `IsTxChainLocked()` were introduced) ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 6958a5b Tree-SHA512: 4b9702c5976643d63874f245b4543c068523c0c87cc8298f8fbf0826426088397167dc775fd34aa27ce7d8decc8bb592e3ae11c6342193d757d285fad1f8d010
3 participants
This is an alternative to #3152 and #3154
Profiling has shown that large wallets spend nearly all the time in
TransactionFilterProxy::filterAcceptsRow. The most expensive part was the datetime comparison as Qt internally does a conversion from local time to UTC before doing the actual comparison. Simply using seconds since epoch instead ofQDateTimesolves the issue.The second most time was spent in the
index.data(TransactionTableModel::LabelRole)which performed conversion from a string into CBitcoinAddress and then into CTxDestination. I'm avoiding this now by also holding these types in TransactionRecord.A lot of time was also lost due to unnecessary updates of the TransactionRecord in regard to the IS lock status. This is fixed by only doing this when really necessary (NotifyTransactionChanged was called on this TX and the IS lock state is still false).
For more details, see individual commits.
There is still potential for optimization. I think that we can optimize the time spent in
filterAcceptsRowto nearly zero by introducing some cache. But this is for another PR.