Fix largest part of GUI lockups with large wallets #3155
Conversation
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] make sure transaction table entry gets updated after bump * Remove unnecessary tracking of IS lock count * Track lockedByChainLocks in TransactionRecord * Only update record when the TX was not ChainLocked before * Emit dataChanged for CT_UPDATED transactions * Use plain seconds since epoch comparison in TransactionFilterProxy::filterAcceptsRow 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. * Don't invoke updateConfirmations directly and let pollBalanceChanged handle it * Implement AddressTableModel::labelForDestination This one avoids converting from string to CBitcoinAddress and calling .Get() on the result. * Also store CBitcoinAddress object and CTxDestination in TransactionRecord This avoids frequent and slow conversion * Use labelForDestination when possible This avoids unnecessary conversions * Don't set fForceCheckBalanceChanged to true when IS lock is received We already do this through updateTransaction(), which is also called when an IS lock is received for one of our own TXs. * Only update lockedByChainLocks and lockedByInstantSend when a change 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. * Hold and update label in TransactionRecord Instead of looking it up in data() * Review suggestions * Use proper columns in dataChanged call in updateAddressBook
Missed this in dashpay#3155
Missed this in dashpay#3155
* 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]>
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.