gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged #18160

pull promag wants to merge 1 commits into bitcoin:master from promag:2020-02-avoid-getbalance changing 3 files +15 −14
  1. promag commented at 11:45 am on February 16, 2020: member

    Each 250ms the slot WalletModel::pollBalanceChanged is called which, at worst case, calls Wallet::GetBalance. This is a waste of resources since most of the time there aren’t new transactions or new blocks. Fix this by early checking if cache is dirty or not.

    The actual balance computation can still hang the GUI thread but that is tracked in #16874 and should be fixed with a solution similar to #17135.

  2. gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged 0933a37078
  3. promag requested review from ryanofsky on Feb 16, 2020
  4. promag requested review from hebasto on Feb 16, 2020
  5. promag requested review from jonasschnelli on Feb 16, 2020
  6. promag commented at 11:49 am on February 16, 2020: member
    @MarcoFalke considering #18123 this should be backported too (if change is correct).
  7. fanquake added the label GUI on Feb 16, 2020
  8. hebasto commented at 5:58 pm on February 16, 2020: member
    Concept ACK about not wasting resources.
  9. in src/qt/walletmodel.cpp:92 in 0933a37078 
     97-        if(transactionTableModel)
 98-            transactionTableModel->updateConfirmations();
 99-    }
100+    checkBalanceChanged(new_balances);
101+    if(transactionTableModel)
102+        transactionTableModel->updateConfirmations();
    hebasto commented at 6:29 pm on February 16, 2020:
    nit: While these lines are touched already, why not apply correct format style?
    promag commented at 9:30 pm on February 16, 2020:
    Forgot to mention to review with &w=1.
  10. hebasto approved
  11. hebasto commented at 6:30 pm on February 16, 2020: member

    ACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

    Early return from tryGetBalances() also minimizes holding of cs_wallet lock.

  12. DrahtBot commented at 7:24 pm on February 16, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #17954 (wallet: Remove calls to Chain::Lock methods by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  13. promag commented at 10:09 am on February 17, 2020: member
    Might fix #15015.
  14. jonasschnelli commented at 7:02 pm on February 20, 2020: contributor
    See also #10251 (could the discussion there be relevant here?)
  15. promag commented at 7:13 pm on February 20, 2020: member

    The goal here is to avoid unnecessary calls to GetBalances and to be an easy backport.

    Refactoring to make it asynchronous can also be done but I don’t think it invalidates this change.

  16. jonasschnelli approved
  17. jonasschnelli commented at 2:28 pm on February 21, 2020: contributor
    utACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37
  18. in src/interfaces/wallet.h:204 in 0933a37078 
    200@@ -201,8 +201,11 @@ class Wallet
201     //! Get balances.
202     virtual WalletBalances getBalances() = 0;
203 
204-    //! Get balances if possible without blocking.
205-    virtual bool tryGetBalances(WalletBalances& balances, int& num_blocks) = 0;
206+    //! Get balances if possible without waiting for chain and wallet locks.
    instagibbs commented at 5:10 pm on February 25, 2020:
    This should mention that it won’t attempt to get balances if height is same.
  19. instagibbs approved
  20. instagibbs commented at 5:14 pm on February 25, 2020: member

    ACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37

    Unrelated to this PR: Checking for staleness should be done via checking total chain work, not height changes. A re-org across a difficulty change threshold could result in balances not updating when they should.

  21. MarcoFalke commented at 5:38 pm on February 25, 2020: member
    Can someone explain to me how this is supposed to work? Why does the balance not change when the block height stays the same? Can’t we send out funds or receive them in the meantime (they might be untrusted, but still, the balance should change)?
  22. instagibbs commented at 6:03 pm on February 25, 2020: member
    @MarcoFalke look at updateTransaction which sets the “force” parameter. I’ll have to dig more to completely understand on my side
  23. MarcoFalke commented at 6:08 pm on February 25, 2020: member
    In that case it seems easier to just call it with the force parameter on every connected block and then remove the polling?
  24. promag commented at 6:15 pm on February 25, 2020: member
    The real deal should be async balance computation to not block the GUI thread. What you suggest is a bigger change and requires locks instead of try locks.
  25. ryanofsky commented at 10:53 pm on February 25, 2020: member

    I think this PR would be equivalent to adding:

    0if (!fForceCheckBalanceChanged && cachedNumBlocks == m_client_model.getNumBlocks()) return;

    to the top of the WalletModel::pollBalanceChanged() function after #17905.

    Assuming #17905 is a change we want to make anyway, keeping the changed detection in pollBalanceChanged() instead of tryGetBalances seems better because:

    1. It keeps tryGetBalances method the same and its usage simpler
    2. It avoids overhead of tryGetBalances IPC call (with #10102) in addition to avoiding the overhead of balance computation.
    3. Not adding a new tryGetBalances cached_num_blocks argument should leave one less thing that needs to be updated in #17993, which switches polling to check the chain tip instead of the chain height
  26. promag commented at 11:15 pm on February 25, 2020: member

    @ryanofsky If we call getBalances async when the tip changes or a wallet transaction is updated then we can drop tryGetBalances (it only exists because of polling).

    So this PR tunes tryGetBalances for polling, maybe it should rename it tryGetBalancesIfNeeded.

  27. ryanofsky approved
  28. ryanofsky commented at 0:43 am on February 26, 2020: member
    Code review ACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37, but I would prefer (not strongly) for #17905 to be merged first. This PR can be simpler if it is based on #17905, so tryGetBalances can just be left alone instead of changing into to a more complicated tryGetBalancesIfNeeded function, and then getting changed back later when we want to optimize it out.
  29. promag commented at 0:52 am on February 26, 2020: member
    @ryanofsky is the alternative easy to backport?
  30. ryanofsky commented at 1:45 am on February 26, 2020: member

    @ryanofsky is the alternative easy to backport?

    The suggested alternative makes this PR smaller and probably easier to backport, but it builds on #17905 which is bigger and does have minor conflicts with 0.18 and 0.19.

    If the goal is to backport #18160 without #17905, I’d suggest leaving this PR in its current form so it is easier to backport. If the goal is to backport both PRs, then adopting the suggested alternative won’t have an impact on backporting. The alternative would just make this PR simpler and let these changes be entirely contained in src/qt/ instead of requiring a complication to src/interfaces/ that will be unnecessary and confusing after #17905

    But again no strong preference on what order things are merged. I am just saying there will be less to clean up if #17905 is merged first and src/interfaces can be left alone.

  31. promag commented at 1:55 am on February 26, 2020: member
    Well we could tag this for backport to 0.19 and wait for progress on #17905 or 0.19.1.
  32. promag commented at 2:39 am on March 15, 2020: member
    IMO this should go into 0.20. @ryanofsky @jonasschnelli @laanwj
  33. luke-jr commented at 1:50 am on March 23, 2020: member
    @promag Too late for any non-fixes…
  34. promag commented at 9:37 am on March 23, 2020: member
    @luke-jr do you consider #15015 and related issues bugs?
  35. laanwj added this to the milestone 0.20.0 on Mar 26, 2020
  36. jonatack commented at 11:39 am on March 30, 2020: member

    ACK 0933a37078e based primarily on code review, despite a lot of manual testing with a large 177MB wallet.

    I did a fair amount of manually testing this PR versus master, back on forth, on debian and macOS, loading small wallets and a 177MB wallet in the GUI, and left a comment here: #15015 (comment). The results are variable: When all is snappy and well, I am not seeing a difference. When the GUI is laggy/scanning, this PR does seems better, but I don’t have much confidence given the variability. This ACK is mainly based on code review.

  37. promag commented at 2:28 pm on March 30, 2020: member

    Made a wallet with 34176 transactions. Then ran instruments before and after this PR for around 90 seconds.

    Before: Screenshot 2020-03-30 at 15 09 24

    After: Screenshot 2020-03-30 at 15 19 30

    So before 8.44 seconds (of 90s) in total was spent in CWallet::GetBalance. Note that after no significant CPU was spent.

  38. laanwj merged this on Mar 31, 2020
  39. laanwj closed this on Mar 31, 2020
  40. promag deleted the branch on Mar 31, 2020
  41. sidhujag referenced this in commit b857234f16 on Mar 31, 2020
  42. MarkLTZ referenced this in commit 7fdadc73fc on Apr 6, 2020
  43. promag referenced this in commit d3cfe0d6a4 on Apr 6, 2020
  44. MarkLTZ referenced this in commit 8436547baa on Apr 6, 2020
  45. ryanofsky referenced this in commit d04533c28e on Apr 10, 2020
  46. ryanofsky referenced this in commit e2849e109b on Apr 10, 2020
  47. ryanofsky referenced this in commit 9fe489b76f on Apr 10, 2020
  48. ryanofsky referenced this in commit d9aa420e17 on Apr 10, 2020
  49. ryanofsky referenced this in commit 8928ebd6eb on Apr 10, 2020
  50. ryanofsky referenced this in commit 6dbc52bae9 on Apr 10, 2020
  51. ryanofsky referenced this in commit f3ecfb553f on Apr 15, 2020
  52. ryanofsky referenced this in commit abc7847de4 on Apr 15, 2020
  53. ryanofsky referenced this in commit 26c1dc9c2d on Apr 15, 2020
  54. ryanofsky referenced this in commit 605c3f19be on Apr 15, 2020
  55. ryanofsky referenced this in commit ae38954238 on Apr 15, 2020
  56. ryanofsky referenced this in commit e3f3e8efc1 on Apr 15, 2020
  57. ryanofsky referenced this in commit cf333aba15 on Apr 25, 2020
  58. ryanofsky referenced this in commit 6de7f9efc1 on Apr 25, 2020
  59. ryanofsky referenced this in commit bf0a510981 on May 1, 2020
  60. ryanofsky referenced this in commit d3a56be77a on May 1, 2020
  61. ryanofsky referenced this in commit ae9ee3c8fc on May 1, 2020
  62. ryanofsky referenced this in commit 24854ec8aa on May 1, 2020
  63. ryanofsky referenced this in commit 803fb13624 on May 1, 2020
  64. ryanofsky referenced this in commit a4bc125fec on May 1, 2020
  65. ryanofsky referenced this in commit 09939485d3 on May 8, 2020
  66. ryanofsky referenced this in commit 6759a0c81e on May 8, 2020
  67. jonasschnelli referenced this in commit a587f85853 on May 20, 2020
  68. sidhujag referenced this in commit 381c70f27b on May 20, 2020
  69. fanquake referenced this in commit 30a28146ac on Jun 9, 2020
  70. MarcoFalke referenced this in commit 28a9df7d76 on Aug 11, 2020
  71. janus referenced this in commit 525dd34517 on Nov 15, 2020
  72. janus referenced this in commit f3968d34cd on Nov 15, 2020
  73. deadalnix referenced this in commit 3ba6c6af6e on Jan 13, 2021
  74. DrahtBot locked this on Feb 15, 2022


promag hebasto DrahtBot jonasschnelli instagibbs MarcoFalke ryanofsky luke-jr jonatack

Labels
GUI

Milestone
0.20.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me