Qt: Only call tryGetBalances in pollBalanceChanged if the result will be used. #13612

pull tecnovert wants to merge 2 commits into bitcoin:master from particl:bitcoin changing 3 files +22 −18
  1. tecnovert commented at 6:57 pm on July 7, 2018: contributor
    getBalances takes significant processing if the wallet has many transactions.
  2. Avoid calling tryGetBalances in pollBalanceChanged if the result won't be used.
    getBalances takes significant processing if the wallet has many transactions.
    e7ad1768fe
  3. sipa commented at 1:32 am on July 8, 2018: member
    Concept ACK, seems reasonable if it’s an issue.
  4. fanquake added the label GUI on Jul 8, 2018
  5. fanquake added the label Wallet on Jul 8, 2018
  6. fanquake requested review from jonasschnelli on Jul 8, 2018
  7. MarcoFalke commented at 6:48 am on July 8, 2018: member

    Could do an early return to avoid wrapping the whole function scope in two sets of braces and indenting it by 4 spaces

    On Sat, Jul 7, 2018 at 2:33 PM, Pieter Wuille notifications@github.com wrote:

    Concept ACK, seems reasonable if it’s an issue.

    — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/13612#issuecomment-403254845, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv7wlo1WfWRJHWaMjiwiIGZ2r5nfSks5uEWFPgaJpZM4VGdUl .

  8. tecnovert force-pushed on Jul 8, 2018
  9. Rearranged so TRY_LOCK is called first.
    The LOCK in getNumBlocks shouldn't be run before the TRY_LOCK in
    tryGetBalances
    The neatest fix seems to be to move the height check into tryGetBalances.
    899d8f68a0
  10. tecnovert force-pushed on Jul 8, 2018
  11. tecnovert commented at 2:11 pm on July 8, 2018: contributor

    It takes a fair amount of blocks and transactions before the issue shows up. CPU usage scales with both the chain height and number of transactions.

    gprof results show most of the time is taken by all the GetDepthInMainChain calls from IsTrusted

    Testing on a regtest node with 10k blocks and 10k txns htop shows: +-6 CPU% before the changes and +-0.7 CPU% after

    20k blocks and 10k txns: +-6.5 CPU% before the changes and +-0.7 CPU% after

    40k blocks and 39.6k txns: +-20 CPU% before the changes and +-0.7 CPU% after

  12. promag commented at 9:53 pm on July 8, 2018: member

    Concept ACK.

    A greater improvement would be to avoid pollBalanceChanged each 250ms. Will test.

  13. practicalswift commented at 1:13 pm on July 9, 2018: contributor

    Concept ACK

    Nice first-time contribution @tecnovert!

  14. jonasschnelli commented at 8:38 pm on July 10, 2018: contributor
    The way how to GUI (and the wallet) calculates the balance has significant performance impacts. I tried in the past to enhance this situation (but failed, see #10251). Concept ACK, will test and perf-compare soon.
  15. DrahtBot commented at 8:15 pm on August 3, 2018: 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:

    • #14437 (Refactor: Start to separate wallet from node by ryanofsky)
    • #10973 (Refactor: separate wallet from node 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.

  16. DrahtBot commented at 9:00 pm on September 7, 2018: member
  17. DrahtBot closed this on Sep 7, 2018

  18. DrahtBot reopened this on Sep 7, 2018

  19. DrahtBot added the label Needs rebase on Nov 9, 2018
  20. DrahtBot commented at 3:16 pm on November 9, 2018: member
  21. in src/interfaces/wallet.cpp:348 in 899d8f68a0
    344@@ -345,16 +345,24 @@ class WalletImpl : public Wallet
    345         }
    346         return result;
    347     }
    348-    bool tryGetBalances(WalletBalances& balances, int& num_blocks) override
    349+    bool tryGetBalances(WalletBalances& balances, bool skip_height_check, int cached_blocks, int& num_blocks) override
    


    meshcollider commented at 7:42 am on November 10, 2018:
    I feel like this bool name is a little confusing, could be ambiguous on exactly what it is skipping. Perhaps ignore_cached or force_check_balance?
  22. meshcollider commented at 11:17 am on November 14, 2018: contributor
  23. cryptozeny referenced this in commit b6f0a9d6c1 on Feb 8, 2019
  24. cryptozeny referenced this in commit 2a96f22c92 on Feb 8, 2019
  25. cryptozeny referenced this in commit 50403a93c6 on Feb 8, 2019
  26. cryptozeny referenced this in commit ea78562840 on Feb 8, 2019
  27. cryptozeny referenced this in commit fff78b9b6f on Feb 8, 2019
  28. cryptozeny referenced this in commit 72436c90b2 on Feb 8, 2019
  29. fanquake added the label Up for grabs on Aug 4, 2019
  30. fanquake closed this on Aug 4, 2019

  31. laanwj removed the label Needs rebase on Oct 24, 2019
  32. jarolrod commented at 9:28 pm on March 25, 2021: member
    @fanquake This can have the Up For Grabs label removed. A similar fix was done in #18160, then reverted as part of a different fix in https://github.com/bitcoin/bitcoin/pull/18587
  33. fanquake removed the label Up for grabs on Mar 25, 2021
  34. DrahtBot locked this on Aug 16, 2022

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-10-31 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me