Add balances cache / GUI: use a signal instead of a poll thread #10251

jonasschnelli commented at 2:43 PM on April 21, 2017: contributor

There are two major parts in this PR.

A) wallet: introduce a per-balance-type cache

Additionally to the per CWalletTx debit, etc. caches, this adds an atomic<CAmount> cache for each balance type (available, immature, watchonly, etc.). If the balance has been cached, no lock will be acquired when calling Get*Balance(). As always with caches, the problematic parts is where to invalidate it (that is why there are some calls to MarkBalancesDirty()).

B) Signal for balance changes

This PR exposes a new wallet signal that will signal the GUI when a balance change had happened (instead of the 250ms polling timer).

jonasschnelli added the label GUI on Apr 21, 2017

jonasschnelli added the label Wallet on Apr 21, 2017

in src/wallet/wallet.cpp:1879 in 69b751bd36 outdated

1917 |  
1918 |  CAmount CWallet::GetBalance() const
1919 |  {
1920 | -    CAmount nTotal = 0;
1921 | +    if (!fBalancesDirty) {
1922 | +        return nBalanceCache;

ryanofsky commented at 3:07 PM on April 21, 2017:

I think think you need to acquire cs_wallet lock here, otherwise this could return a partial sum if GetBalance got called from two threads at the same time and one thread returned nBalanceCache while the other thread was in the middle of the nBalanceCache += loop.

Alternately, you could bring back the nTotal local variable and just set nBalanceCache = nTotal atomically.

jonasschnelli commented at 3:56 PM on April 21, 2017:

Oh.. good point. I'll re-add the temp variable then.

in src/wallet/wallet.cpp:1924 in 69b751bd36 outdated

1920 | -    CAmount nTotal = 0;
1921 | +    if (!fBalancesDirty) {
1922 | +        return nBalanceCache;
1923 | +    }
1924 | +
1925 | +    nBalanceCache = 0;

ryanofsky commented at 3:10 PM on April 21, 2017:

I think you need to move this line after the LOCK2(cs_main, cs_wallet); line. Otherwise if this method is called from two different threads simultaneously, this line could zero out a balance which is in the middle of being added up by the other thread.

Or alternately, delete this line and and set nBalanceCache = nTotal atomically after the loop.

laanwj commented at 3:11 PM on April 21, 2017: member

Concept ACK, nice!

in src/wallet/wallet.h:920 in 69b751bd36 outdated

 915 | +    mutable std::atomic<CAmount> nUnconfirmedWatchOnlyBalanceCache;
 916 | +    mutable std::atomic<CAmount> nImmatureWatchOnlyBalanceCache;
 917 | +    std::atomic<bool> fBalancesDirty;
 918 | +
 919 | +    /** Notification for balance changes */
 920 | +    boost::signals2::signal<void ()> BalancesDidChange;

ryanofsky commented at 3:21 PM on April 21, 2017:

Maybe call it BalancesChanged instead of BalancesDidChange to be consistent with the naming of other signals (StatusChanged, AddressBookChanged, etc).

jonasschnelli commented at 3:59 PM on April 21, 2017:

Yes. I can do that. I kinda like the signal naming convention from apple. XYWillChange, XYDidChange, etc.: because it allows the listener to know (without reading to much code) if the event was synchronous or has triggered a process with a later update. But lets not overdo it here,.. will change therefore.

jonasschnelli force-pushed on Apr 21, 2017

jonasschnelli commented at 4:12 PM on April 21, 2017: contributor

Force pushed fixes for issues found by @ryanofsky.

in src/wallet/wallet.h:783 in b18f6765c5 outdated

 779 | @@ -780,6 +780,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
 780 |          nRelockTime = 0;
 781 |          fAbortRescan = false;
 782 |          fScanningWallet = false;
 783 | +        fBalancesDirty = true;

ryanofsky commented at 4:09 PM on May 1, 2017:

In commit "Add atomic cache for balances"

The whole caching portion of this PR actually seems broken because fBalancesDirty is initialized to true but never set to false anywhere. I think this can be fixed, but maybe simplest thing to do would be to drop the caching, and just keep the signalling part of this PR.

jonasschnelli commented at 7:19 AM on May 4, 2017:

Oh. Yes indeed. I now switched to a cache-invalidation-atomic per balance type and set it to false once the cache has been built.

in src/qt/walletmodel.cpp:169 in e63ab9be56 outdated

 135 | @@ -137,12 +136,6 @@ void WalletModel::updateBalance()
 136 |      }
 137 |  }
 138 |  
 139 | -void WalletModel::updateTransaction()

ryanofsky commented at 5:05 PM on May 1, 2017:

In commit "[Qt] remove unused polling code"

I think it would be good to squash this commit into "[Qt] use the BalancesChanged signal instead of a 250ms poll timer." It's confusing to see code that checks the fForceCheckBalanceChanged variable and some code that sets it removed, while other code that sets it sticks around.

jonasschnelli commented at 7:19 AM on May 4, 2017:

Good point. Squashed those two commits together.

ryanofsky commented at 5:13 PM on May 1, 2017: member

See other comments, but it looks like the caching here is broken. The Qt changes seem fine but I think would be easier to understand as 1 commit instead of 3.

jonasschnelli force-pushed on May 4, 2017

jonasschnelli commented at 7:20 AM on May 4, 2017: contributor

Force push fixed @ryanofsky points.

in src/wallet/wallet.cpp:1939 in 28637b6254 outdated

1934 | @@ -1914,13 +1935,18 @@ CAmount CWallet::GetBalance() const
1935 |              if (pcoin->IsTrusted())
1936 |                  nTotal += pcoin->GetAvailableCredit();
1937 |          }
1938 | +        nBalanceCache = nTotal;
1939 | +        fBalancesDirty = false;

ryanofsky commented at 3:22 PM on May 4, 2017:

In commit "Add atomic cache for balances"

Maybe rename fBalancesDirty to fBalanceDirty since this only applies to the balance returned from CWallet::GetBalance and not the other balances.

ryanofsky commented at 3:22 PM on May 4, 2017: member

utACK a6903788aa105bd26dc6bd5a2784db5109467848

jonasschnelli force-pushed on May 4, 2017

jonasschnelli commented at 6:42 PM on May 4, 2017: contributor

Force push fixed @ryanofsky's nit (and also added the "other balances" atomics to SetNull()).

ryanofsky commented at 3:14 PM on May 5, 2017: member

ACK 84904a9d8ac5e98558b366bc69338d4ad07dc2f7.

Changes since last review were those Jonas mentioned. Seeing the missing initializations in SetNull() was a little disconcerting, because I would have expected tests to catch errors they would cause. Actually one test did fail (wallet_tests/rescan) but I guess I didn't notice it.

jonasschnelli force-pushed on Jun 14, 2017

jonasschnelli commented at 6:38 AM on June 14, 2017: contributor

Needed a trivial rebase (BOOST_FOREACH -> for change).

in src/wallet/wallet.cpp:816 in 51a7a33141 outdated

 818 | -        for (std::pair<const uint256, CWalletTx>& item : mapWallet)
 819 | -            item.second.MarkDirty();
 820 | -    }
 821 | +    MarkBalancesDirty();
 822 | +
 823 | +    LOCK(cs_wallet);

kallewoof commented at 6:58 AM on June 14, 2017:

Am I mistaken when I think that the lock on cs_wallet will actually happen before the call to MarkBalancesDirty() due to scope? If so I would swap these lines for clarity.

jonasschnelli commented at 7:26 AM on June 14, 2017:

I guess I should move the LOCK() up to the top. Because its RAII, I think it doesn't matter in this case (no extra block). The lock should take affect when we enter the function/scope (before MarkBalancesDirty()). But maybe I'm wrong.

kallewoof commented at 7:01 AM on June 14, 2017: member

utACK 9ca1140a19d3d39b86d7a6d6dc7ed907fcea2a88 (my QT skills are limited though)

sipa commented at 7:18 AM on June 14, 2017: member

@kallewoof No, the lock on cs_wallet will happen after MarkBalancesDirty

sipa commented at 7:27 AM on June 14, 2017: member

But maybe I'm wrong

You're wrong.

Add atomic cache for balances 8688671484

[Qt] use the BalancesChanged signal instead of a 250ms poll timer

remove unused polling code

1e93408654

[Qt] rename checkBalanceChanged to updateBalance 187b3eea4a

jonasschnelli force-pushed on Jun 14, 2017

jonasschnelli commented at 7:32 AM on June 14, 2017: contributor

But maybe I'm wrong

You're wrong.

Okay. Then I'll better be quite. Moved up the LOCK().

ryanofsky commented at 7:18 PM on June 14, 2017: member

utACK 187b3eea4a9da6e589099289632ce0425e269e37. Only change since last review was acquiring wallet lock before the one markdirty call.

laanwj commented at 7:06 AM on June 15, 2017: member

I'm trying to understand whether this will introduce a hang in the GUI thread in some cases.

Right now, the recurrent poll timer (not thread! it's a qt timer which runs in the GUI thread) updates the balance if it's possible to get the locks using TRY_LOCK. Then it will get them, once, and process all balances.

After this change the locks are no longer TRY, so the BalancesChanged signal will always cause the GUI thread to take the wallet locks (through CWallet::get*Balance()). Not once, but up to once per type of balance, if they all turn out to be dirty (which is likely). Taking the cs_main and cs_wallet lock up to 6 times can take ages when the cs_main lock is contended: during initial sync, catching up. This all happens in the GUI thread (at the receiving end of updateBalance signal).

Maybe I'm misunderstanding the code, but if I'm right about this, I'm not convinced that this is an improvement in user experience.

jonasschnelli commented at 12:05 PM on June 16, 2017: contributor

Some of the current heavy "freezes" are happening because of the balance poll thread... it seems to be a significant CPU eater. Right now, we TRY_LOCK every 250ms (which often LOCKS successfully and then iterates (multiple times) through the complete mapWallet).

jonasschnelli commented at 12:10 PM on June 16, 2017: contributor

Here the complete profiling (master):

It seems like that the pollBalanceThread took 50% of the complete CPU time for the measured run. (30 seconds, opening debug window and 10-20 times sendtoaddress)

jonasschnelli commented at 12:16 PM on June 16, 2017: contributor

I testes again with this PR and the responsiveness is much better. @laanwj points make sense to me.

Could it be, that the TRY_LOCK every 250ms will result in getting the LOCK very often and then do the heavy balance calculation on the main thread because QTimer runs on the Main/GUI thread?

ryanofsky commented at 12:20 PM on June 16, 2017: member

Could it be, that the TRY_LOCK every 250ms will result in getting the LOCK very often and then do the heavy balance calculation on the main thread because QTimer runs on the Main/GUI thread?

That's what I would expect. Have you thought about keeping the cache but bringing back the polling and try lock to address laanwj's point?

jonasschnelli force-pushed on Jun 16, 2017

jonasschnelli commented at 1:05 PM on June 16, 2017: contributor

@ryanofsky I profiled this PR with only the first commit (atomic caches, keep the polling). The results are better, but not as good as with the signal/non-polling approach:

Test run: Startup, call sendtoaddress(getnewaddress(), 1) 20 times manually

Master with only the first commit (pure atomic caches):

The polling thread gets measured (through significants) in my profiler with ~33% (very high IMO) <img width="1396" alt="bildschirmfoto 2017-06-16 um 14 58 42" src="https://user-images.githubusercontent.com/178464/27227684-c2640fda-52a4-11e7-9396-b111d1c28e13.png">

Master with this PR

The balance updates seems no longer be significant: <img width="1396" alt="bildschirmfoto 2017-06-16 um 15 04 14" src="https://user-images.githubusercontent.com/178464/27227760-169d1c18-52a5-11e7-8d3a-f359c0f2b21b.png">

ryanofsky commented at 6:36 PM on June 21, 2017: member

Unclear to me if @laanwj's concern about losing the try lock is sufficiently addressed with the new testing and discussion at https://botbot.me/freenode/bitcoin-core-dev/msg/87346236/. If it is, maybe this PR is close to ready for merge (so far two utACKs from kallewoof and me). If not, it seems like there is some advantage (50% -> 30% poll thread cpu decrease) and no downside to just merging the first commit for now?

jonasschnelli commented at 6:38 PM on June 21, 2017: contributor

I guess it would be great if someone with (g)perf experience on Linux/Ubuntu and maybe on Windows could benchmark this PR against master.

ryanofsky commented at 7:54 PM on June 21, 2017: member

Linux and windows performance tests seem like they would be a good sanity check on the numbers you collected (50% cpu usage spent on balance computation on master, 33% with caching commit, and ~0% with full pr), but is there some reason to think this behavior would be dependent on the platform?

I guess the main thing I'm not clear on is whether the results you collected (assuming they do hold up on other platforms) address laanwj's concern above, or if there is some kind of different testing that needs to be done, or some other safeguard that should be added to prevent the possibility of new hangs.

promag commented at 1:19 AM on July 20, 2017: member

@jonasschnelli if still relevant I can do it.

jonasschnelli commented at 6:57 AM on July 20, 2017: contributor

@promag: A benchmark would be very good to have. Ideally you compare master against this PR and against only the first commit in this PR (3 versions). Thanks!

jonasschnelli commented at 4:05 AM on October 5, 2017: contributor

Closing for now...

jonasschnelli closed this on Oct 5, 2017

DrahtBot locked this on Sep 8, 2021